Skip to content
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.

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

detect/csum: rm interaction btw stream setting/csum #12440

Closed
wants to merge 2 commits into from

Conversation

jlucovsky
Copy link
Contributor

Continuation of #12432

Issue: 7467

Stream checksum validation no longer has a side effect of setting PKT_IGNORE_CHECKSUM and thus, no longer affects csum keyword checks.

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

Describe changes:

  • During stream checksum validation checks, no longer set PKT_IGNORE_CHECKSUM

Updates:

  • Add a note to the upgrade document describing the behavioral change.

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

  • To use an LibHTP, 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#2241
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

Issue: 7467

Stream checksum validation no longer has a side effect of setting
PKT_IGNORE_CHECKSUM and thus, no longer affects csum keyword checks.
Describe the change of behavior between the stream.checksum-validation
setting and checksum-based rule keywords.
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.62%. Comparing base (95e8427) to head (36b5963).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12440      +/-   ##
==========================================
- Coverage   80.63%   80.62%   -0.01%     
==========================================
  Files         920      920              
  Lines      258704   258700       -4     
==========================================
- Hits       208595   208582      -13     
- Misses      50109    50118       +9     
Flag Coverage Δ
fuzzcorpus 56.81% <ø> (-0.01%) ⬇️
livemode 19.40% <ø> (+<0.01%) ⬆️
pcap 44.27% <ø> (-0.07%) ⬇️
suricata-verify 63.28% <ø> (+0.01%) ⬆️
unittests 58.52% <ø> (+<0.01%) ⬆️

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

Comment on lines +85 to +91
- The configuration setting controlling stream checksum checks no longer affects
checksum keyword validation. Previously, when ``stream.checksum-validation``
was set to ``no``, the checksum keywords (e.g., ``ipv4-csum``, ``tcpv4-csum``, etc)
would either match or not match according to the value used with the checksum keyword.
Previous behavior would return a match when ``ipv4-csum: valid`` was specified and
not match if ``ipv4-csum: invalid`` was used. With 8.0, a match will occur based on the
computed checksum and the value (``valid`` or ``invalid``) agree.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure I understand this statement.

In 7, if I disable checksum validation, do the keywords always treat the checksum as OK? And now in 8 even if checksum validation is disabled, they will match on the actual checksum regardless if checksum-validation is disable or not at the stream level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 7, if checksum validation is disabled, the csum rule keywords match based on the value used with the keyword. eg., tcpv4-csum: invalid wont't match; tcpv4-csum: valid matches

In 8, checksum-validation no longer affects the csum rule keywords. The csum rule keywords trigger if the checksum value (valid or invalid) match the value used with the rule keyword, e.g, ipv4-csum: valid (match if checksum is valid) or ipv4-csum: invalid match only when checksum is invalid)

Copy link
Member

Choose a reason for hiding this comment

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

I find this bit confusing:

In 7, if checksum validation is disabled, the csum rule keywords match based on the value used with the keyword.

Maybe something like?

In 7, if checksum validation is disabled, the rule keywords checking for checksum will always consider it valid. Meaning tcpv3-csum: invalid will never match.

In 8, checksum-validation no longer affects the csum rule keywords. ipv4-csum: valid only match if the checksum is valid, even if engine checksum validations have been disabled.

Or something like that.. In particular, this is the bit I find confusing:

The csum rule keywords trigger if the checksum value (valid or invalid) match the value used with the rule keyword

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24313

@jlucovsky
Copy link
Contributor Author

Continued in #12479

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