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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions doc/userguide/upgrade.rst
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ Major changes
- Unknown requirements in the ``requires`` keyword will now be treated
as unmet requirements, causing the rule to not be loaded. See
:ref:`keyword_requires`.
- 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.
Comment on lines +85 to +91
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



Removals
~~~~~~~~
Expand Down
4 changes: 0 additions & 4 deletions src/stream-tcp.c
Original file line number Diff line number Diff line change
Expand Up @@ -5958,11 +5958,7 @@ TmEcode StreamTcp (ThreadVars *tv, Packet *p, void *data, PacketQueueNoLock *pq)
StatsIncr(tv, stt->counter_tcp_invalid_checksum);
return TM_ECODE_OK;
}
} else {
p->flags |= PKT_IGNORE_CHECKSUM;
}
} else {
p->flags |= PKT_IGNORE_CHECKSUM; //TODO check that this is set at creation
}
AppLayerProfilingReset(stt->ra_ctx->app_tctx);

Expand Down
Loading