Skip to content

Conversation

jlucovsky
Copy link
Contributor

Continuation of #13622

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:

  • Extend buffer/variable checks to buffers init data
  • In strict-mode, do not load rules using variables from different buffers.
  • When not in strict mode, issue warning for rules that use variables from different buffers.

Updates:

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#2576
SU_REPO=
SU_BRANCH=

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.
Copy link

codecov bot commented Aug 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.73%. Comparing base (b93a277) to head (07f6c00).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13716      +/-   ##
==========================================
+ Coverage   82.99%   83.73%   +0.74%     
==========================================
  Files        1001     1011      +10     
  Lines      272978   275100    +2122     
==========================================
+ Hits       226556   230355    +3799     
+ Misses      46422    44745    -1677     
Flag Coverage Δ
fuzzcorpus 62.90% <96.15%> (+0.02%) ⬆️
livemode 18.99% <0.00%> (?)
pcap 44.70% <38.46%> (+0.01%) ⬆️
suricata-verify 65.08% <94.87%> (+<0.01%) ⬆️
unittests 59.18% <74.35%> (+<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

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.uptime 648 626 96.6%

Pipeline = 27152

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

minor things :P

Comment on lines +180 to +182
- If a byte keyword (such as ``byte_extract`` or ``byte_math``, etc) is used with
a variable, and that variable usage is with a buffer other than the one used
to create the variable, a warning is printed and the rule is loaded. The
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we clearly indicate the Suricata version, here?

*
* \param arg The name of the variable being sought
* \param s The signature to check for the variable
* \param strict Match if and only iff the list sought and the list found equal.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
* \param strict Match if and only iff the list sought and the list found equal.
* \param strict Match if and only iff the list sought and the list found are equal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

* \param s Pointer the signature to look in.
*
* \retval A pointer to the SigMatch if found, otherwise NULL.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe update the function description, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@jlucovsky
Copy link
Contributor Author

Continued in #13720

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