detect/var: Restrict var usage to single buffer #13584
Conversation
Issue: 1412 When in strict mode, issue an error and refuse to load the rule if variables produced from a different buffer are used with a separate buffer. When not in strict mode (default), issue a warning and load the rule. Only consider sig matches with compatible ids/lists.
Issue 1412
Add mention of byte_{extract,math,test,jump} variable usage
and buffer scope and include how the command line option
strict-rule-keywords affects validation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13584 +/- ##
==========================================
+ Coverage 83.55% 83.60% +0.04%
==========================================
Files 1011 1012 +1
Lines 275039 275072 +33
==========================================
+ Hits 229804 229962 +158
+ Misses 45235 45110 -125
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline = 26847 |
| return true; | ||
| } | ||
|
|
||
| SCLogWarning("Using byte variable from a different buffer may produce indeterminate " |
There was a problem hiding this comment.
can we move this check into a validate callback so that the sid is available in a warning?
There was a problem hiding this comment.
I think that Philippe's comment here also make sense: https://github.com/OISF/suricata/pull/13564/files/664e61a2678b90d25e3f27937059a2f4817aaece#r2183567368
(apologies for the very late reply)
|
Do we want this for 8.0.1 or for 9 ? This is a behavior change... |
|
|
||
| DetectByteIndexType idx; | ||
| if (!DetectByteRetrieveSMVar(name, s, -1, &idx)) { | ||
| if (!DetectByteRetrieveSMVar(name, s, false, -1, &idx, 1)) { |
There was a problem hiding this comment.
I'll modify this to use a sentinel value since the line number is meant to indicate the offset into the rule file(s).
| SCLogDebug("Retrieved SM for \"%s\"", arg); | ||
| return sm; | ||
| } | ||
| if (DetectByteMathSMNameMatch(sm, arg)) { |
There was a problem hiding this comment.
Still #13564 (comment)
This change looks suspicious
Did you test a rule with 2 buffers, each buffer doing a byte_extract+ byte_math ?
I am afraid that with this change, the second buffer will get their found_list from the first buffer when it uses to get the right sm_list == list in current code
Do you have a test for it ?
There was a problem hiding this comment.
This rule is handled correctly -- is this what you were thinking?
alert ip any any -> any any (msg:"multi buffer"; ipv4.hdr;byte_extract:1,0,header; byte_test:1,!=,header,1;tcp.hdr; byte_extract:1,0,header2, relative;byte_test:1,=,header2,1;sid: 1;)
There was a problem hiding this comment.
So, buffers are ok because they run only the first loop of `DetectByteExtractRetrieveSMVar``
The second loop (where you removed the check list==sm_list) is run for DETECT_SM_LIST_PMATCH and DETECT_SM_LIST_BASE64_DATA
So, this allows signature like
alert http any any -> any any (msg:"multi buffer2"; content: "toto"; byte_extract:1,0,header; http.user_agent; base64_decode; base64_data;byte_test:1,=,header,1;sid: 2;) to be accepted when it should not
|
Continued in #13622 |
Continuation of #13564
Issue: 1412
Extend the checks added for 7549 to include buffers.
Only consider sig matches with compatible ids/lists.
Link to ticket: https://redmine.openinfosecfoundation.org/issues/1412
Describe changes:
buffersinit dataUpdates:
strict-rule-keywordsProvide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=OISF/suricata-verify#2576
SU_REPO=
SU_BRANCH=