detect/bytemath: bound shift counts - #16067
Conversation
Zero the result when a byte_math right shift count reaches 64, the width of the uint64_t being shifted, so the operation no longer depends on behavior C11 6.5.7p3 leaves undefined. The left shift case has done this since 473ca6d; the right shift case was left unguarded. DetectByteMathDoMatch() shifted by whatever count it was handed. On x86_64 the hardware masks the count to its low six bits, so a count of 64 became a shift of 0 and returned the extracted value unchanged instead of 0. That value is stored in det_ctx->byte_values[] and feeds any byte_test, isdataat, or content offset later in the signature, so the signature's verdict follows from an arithmetic result the standard does not define. The count reaches the shift from the wire. When byte_math names a variable for rvalue, DetectEngineContentInspectionInternal() reads it out of det_ctx->byte_values[] at detect-engine-content-inspection.c:614, where a preceding byte_extract stored bytes taken from the payload, so one payload byte of 0x40 sets the count to 64. Issue: 8845
Fail rule load when byte_math pairs << or >> with a literal rvalue of 64 or more. rvalue was bounded only to u32::MAX, so a rule shifting by 100 loaded and then produced 0 for every packet it inspected, spending detection work on a comparison whose outcome was settled before the first packet arrived. A variable rvalue still loads. Its value comes from a byte_extract on the packet and is not known until the rule runs, so the guard in DetectByteMathDoMatch() stays the only check covering that path. DetectByteMathParse() reports every SCByteMathParse() failure as "invalid bytemath values", so the rejected rule is named by the "error parsing signature" line that follows rather than by the reason the parser gave. Issue: 8845
Record that << and >> yield 0 when rvalue is 64 or more, beside the existing note about division by zero. A shift count can come from a byte_extract variable, so the rule text alone does not say which counts land in that case. Issue: 8845
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16067 +/- ##
==========================================
- Coverage 83.02% 82.99% -0.04%
==========================================
Files 1002 1002
Lines 276951 276999 +48
==========================================
- Hits 229933 229886 -47
- Misses 47018 47113 +95
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 = 33203 |
catenacyber
left a comment
There was a problem hiding this comment.
Thanks for the work,
CI : ✅
Git ID set : looks fine for me
CLA : you already contributed
Doc update : cool
Redmine ticket : ok
Tests : cool
Dependencies added: none
Code : good
Commits segmentation : I would squash the doc one but you know it already
Commit messages : 🟡 a bit too verbose to my taste
|
Followed this over from #16046 — the shift bound and the new One small thing that didn't carry over, non-blocking since this is already approved: The docs line still reads
but after this PR a literal
Also worth a word in |
|
Merged in #16141, thanks! |
Continuation of #16046
Link to ticket: https://redmine.openinfosecfoundation.org/issues/8845
Describe changes:
Verification: clean build with --enable-warnings --enable-rust-strict
--enable-unittests; C unit tests and cargo test detect::byte_math pass; both new suricata-verify tests pass and both fail on an unfixed build.
Note the second commit fails rules that load today (byte_math with a literal
shift count of 64 or more). That suits 9.0.0-beta1. It is deliberately a
separate commit from the fix so the backport question can be answered on its
own -- main-8.0.x still has the unguarded shift at src/detect-bytemath.c:191.
Updates:
Provide values to any of the below to override the defaults.
SV_BRANCH=OISF/suricata-verify#3288