Skip to content

detect/byte_jump: Support bitmask value #14675

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.

Sign up for GitHub

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

Jump to bottom
Closed
jlucovsky wants to merge 2 commits into from
Closed

detect/byte_jump: Support bitmask value #14675

jlucovsky wants to merge 2 commits into from

Conversation

@jlucovsky
Copy link
Contributor

@jlucovsky jlucovsky commented Jan 22, 2026

Continuation of #14666

Add support for the bitmask value to byte_jump

Snort compatibility says:
- The bitmask value is applied to the extracted value
- The result of the bitmask operation is to be right-shifted by the
  number of trailing 0's in the bitmask value.

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

This question remains open

Describe changes:

  • Clarify how the bitmask value is used in the documentation
  • Add parsing support for bitmask option
  • Apply bitmask value/shift count as jump value is calculated.

Updates:

  • Range check bitmask value: [1, MAX_UINT32]
  • Removed Cargo.lock.in
  • Fix ASAN issue discovered during fuzzing.
  • Rebase and added s-v test
  • s-v test update -- add test case for 0-value bitmask
  • Moved the application of the bitmask to match Snort. Bitmask value now applied prior to multiplier

Provide values to any of the below to override the defaults.

  • To use a Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=OISF/suricata-verify#2880
SU_REPO=
SU_BRANCH=

@jlucovsky
detect/byte_jump: Support bitmask value
0aafe00 
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
@jlucovsky
doc/byte_jump: Clarify bitmask operation
a827ec2 
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.
@jlucovsky jlucovsky mentioned this pull request Jan 22, 2026
Closed
@codecov
Copy link

codecov bot commented Jan 22, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.12%. Comparing base (c333b28) to head (a827ec2).
⚠️ Report is 11 commits behind head on main.

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
Flag Coverage Δ
fuzzcorpus 60.20% <48.78%> (+0.02%) ⬆️
livemode 18.75% <31.70%> (+0.04%) ⬆️
pcap 44.59% <48.78%> (-0.01%) ⬇️
suricata-verify 65.30% <95.12%> (+0.01%) ⬆️
unittests 59.28% <89.10%> (+0.01%) ⬆️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@suricata-qa
Copy link

suricata-qa commented Jan 23, 2026

Information: QA ran without warnings.

Pipeline = 29259

@catenacyber
Copy link
Contributor

catenacyber commented Jan 23, 2026

This question remains open

Which question ?

catenacyber
catenacyber reviewed Jan 23, 2026
View reviewed changes
src/detect-bytejump.h
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*/
Copy link
Contributor

@catenacyber catenacyber Jan 23, 2026

Choose a reason for hiding this comment

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

u8 should be enough as it is < 32 ;-)

Copy link
Contributor Author

@jlucovsky jlucovsky Jan 27, 2026

Choose a reason for hiding this comment

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

Will change.

catenacyber
catenacyber approved these changes Jan 23, 2026
View reviewed changes
Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

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

catenacyber
catenacyber reviewed Jan 23, 2026
View reviewed changes
src/detect-bytejump.c
data->bitmask_value);
val &= data->bitmask_value;
if (val && data->bitmask_shift_count) {
val = val >> data->bitmask_shift_count;
Copy link
Contributor

@catenacyber catenacyber Jan 23, 2026

Choose a reason for hiding this comment

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

This is wrong, not the value should be shifted, but bitmask_value

Copy link
Contributor Author

@jlucovsky jlucovsky Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@catenacyber catenacyber Jan 29, 2026

Choose a reason for hiding this comment

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

I leave this for others to review as this shift does not make any sense to me

catenacyber
catenacyber requested changes Jan 23, 2026
View reviewed changes
Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

See comment about shift

@jlucovsky
Copy link
Contributor Author

jlucovsky commented Jan 27, 2026

Member

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.

@jlucovsky jlucovsky mentioned this pull request Jan 27, 2026
Open
@jlucovsky
Copy link
Contributor Author

jlucovsky commented Jan 27, 2026

Continued in #14701

@jlucovsky jlucovsky closed this Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@catenacyber catenacyber catenacyber requested changes

@jufajardini jufajardini Awaiting requested review from jufajardini jufajardini is a code owner

@victorjulien victorjulien Awaiting requested review from victorjulien victorjulien is a code owner

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jlucovsky @suricata-qa @catenacyber