detect/byte_jump: Support bitmask value#14675
Conversation
Issue: 6693 Add bitmask support to byte_jump - Parse - Calculate shift count - Apply to value before applying multiplier Snort: See https://github.com/chenkc/snort2.9/blob/master/snort-2.9.11.1/src/detection-plugins/sp_byte_jump.c#L780
Issue: 6693 Clarify how the bitmask value is used for byte_jump Snort compatibility says: - The bitmask value is applied to the extracted value before the multiplier is applied. - The result of the bitmask operation is to be right shifted by the number of trailing 0's in the bitmask value.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14675 +/- ##
=======================================
Coverage 82.11% 82.12%
=======================================
Files 1011 1011
Lines 262812 262899 +87
=======================================
+ Hits 215812 215898 +86
- Misses 47000 47001 +1
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 = 29259 |
Which question ? |
| int32_t post_offset; /**< Offset to adjust post-jump */ | ||
| uint16_t multiplier; /**< Multiplier for nbytes (multiplier n)*/ | ||
| uint32_t bitmask_value; /**< bitmask value */ | ||
| uint32_t bitmask_shift_count; /**< bitmask shift value*/ |
There was a problem hiding this comment.
u8 should be enough as it is < 32 ;-)
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
Thanks for the work,
CI : ✅
Commits segmentation : I would squash them, but ok...
Commit messages : good
Git ID set : looks fine for me
CLA : you signed it
Doc update : good (reads always strange to implement an already documented feature)
Redmine ticket : ok
Rustfmt : no rust 😢
Tests : Not sure I understand, see SV comments
Dependencies added: none
Code : good
| data->bitmask_value); | ||
| val &= data->bitmask_value; | ||
| if (val && data->bitmask_shift_count) { | ||
| val = val >> data->bitmask_shift_count; |
There was a problem hiding this comment.
This is wrong, not the value should be shifted, but bitmask_value
There was a problem hiding this comment.
The value val is being right shifted by the according to the bitmask_shift_count calculated during setup. During setup, the bitmask value is right shifted to count the number of trailing 0 bits.
There was a problem hiding this comment.
I leave this for others to review as this shift does not make any sense to me
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
See comment about shift
The question was "when to apply the bitmask" -- snort does it prior to the multiplier so i've moved bitmask application to occur before the multiplier is applied. |
|
Continued in #14701 |
3 participants
Continuation of #14666
Add support for the bitmask value to byte_jump
Link to ticket: https://redmine.openinfosecfoundation.org/issues/6693
This question remains open
Describe changes:
bitmaskvalue is used in the documentationbitmaskoptionUpdates:
Provide 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#2880
SU_REPO=
SU_BRANCH=