Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark flow elephant/v2 #12123

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

inashivb
Copy link
Member

Link to ticket: https://redmine.openinfosecfoundation.org/issues/5647

SV_BRANCH=OISF/suricata-verify#2130

Previous PR: #11645

Changes since v1:

  • added a rule keyword besides allowing flow rate config in suricata.yaml

1. Add user defined elephant flow definition based on rate of bytes
easily configurable in suricata.yaml.
2. Add an elephant flow counter.

Feature 5647
This is to allow a way to match on the rate of the flow through rule
language. This serves as a trivial first step to a more elaborate path
to defining and detecting elephant flows.
@inashivb inashivb requested review from victorjulien and a team as code owners November 15, 2024 10:21
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 79.54545% with 9 lines in your changes missing coverage. Please review.

Project coverage is 83.14%. Comparing base (5d766df) to head (3d329d7).

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #12123       +/-   ##
===========================================
+ Coverage   62.68%   83.14%   +20.46%     
===========================================
  Files         840      910       +70     
  Lines      153669   257927   +104258     
===========================================
+ Hits        96323   214466   +118143     
+ Misses      57346    43461    -13885     
Flag Coverage Δ
fuzzcorpus 60.99% <43.18%> (?)
livemode 19.44% <50.00%> (?)
pcap 44.42% <56.81%> (?)
suricata-verify 62.69% <79.54%> (+0.01%) ⬆️
unittests 59.25% <20.45%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@inashivb
Copy link
Member Author

Need to find a way to keep time blocked data structures for flow management so this can be done on any interval. Victor suggested we could start w this trivial keyword though. Interested in hearing if it makes sense to others too.

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see this split between a PR for the flow.rate and the elephant marking.

Also please add user docs.

Other comments inline.


static int DetectFlowRateSetup(DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
{
uint64_t rate = atoll(rawstr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will need to use the StringParseUint64 or StringParseU64RangeCheck

@@ -499,6 +501,7 @@ typedef struct Flow_
uint64_t todstbytecnt;
uint64_t tosrcbytecnt;

bool elephant;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this affect the flow memory layout? (pahole)

return rate == expected_rate;
}

static int DetectFlowRateSetup(DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to think about how the arguments will express time later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that was a basic. idk why i don't have that 🤦🏽

#include "detect-parse.h"

static int DetectFlowRateMatch(
DetectEngineThreadCtx *det_ctx, Packet *p, const Signature *s, const SigMatchCtx *ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this to use the detect int handling code

uint64_t rate = (p->flow->tosrcbytecnt + p->flow->todstbytecnt) / age;

uint64_t expected_rate = (uint64_t)ctx;
return rate == expected_rate;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect. should'nt look for exact match but greater than equal to instead

@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp 40 42 105.0%

Pipeline 23324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants