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

next/710/70x/20250210/v1 #12549

Closed

Conversation

catenacyber and others added 7 commits February 10, 2025 07:31
To optimize detection, and logging, to avoid going through
all the live transactions when only a few were modified.

Two boolean fields are added to the tx data: updated_tc and ts
The app-layer parsers are now responsible to set these when
needed, and the logging and detection uses them to skip
transactions that were not updated.

There may some more optimization remaining by when we set
both updated_tc and updated_ts in functions returning
a mutable transaction, by checking if all the callers
are called in one direction only (request or response)

Ticket: 7087
(cherry picked from commit b02557a)

Conflicts:
	rust/src/applayer.rs
	rust/src/enip/enip.rs
	rust/src/ldap/ldap.rs
	src/app-layer-smtp.c
Ticket: 7495

We want to finish also if we tested all the expected protocols
in mask, or if we tested even more.

There can be one more protocol coming from pe0, which can be
the protocol already found in the other direction.

(cherry picked from commit b5094b0)
cherry-picked from commit 8ff80cb
Accepts valid characters as defined in RFC3261.

cherry-picked from commit 7e993d5
The `is_version_char` function incorrectly allowed characters that are not
part of the valid SIP version "SIP/2.0".

For instance, 'HTTP/1.1' was mistakenly accepted as a valid SIP version,
although it's not.

This commit fixes the issue by updating the condition to strictly
check for the correct version string.

cherry-picked from commit 69f841c
Issue: 7467

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

(cherry picked from commit 758da98)
Describe the change of behavior between the stream.checksum-validation
setting and checksum-based rule keywords.

(cherry picked from commit cfbf8fd)
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 98.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (2be430e) to head (ee093f6).

Additional details and impacted files
@@              Coverage Diff               @@
##           main-7.0.x   #12549      +/-   ##
==============================================
+ Coverage       83.13%   83.24%   +0.10%     
==============================================
  Files             922      922              
  Lines          261187   261304     +117     
==============================================
+ Hits           217147   217527     +380     
+ Misses          44040    43777     -263     
Flag Coverage Δ
fuzzcorpus 64.26% <97.61%> (+0.29%) ⬆️
suricata-verify 63.50% <88.09%> (+0.02%) ⬆️
unittests 62.36% <41.11%> (-0.01%) ⬇️

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

Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

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

Consistent w the approved PRs.
What does

Conflicts:
	rust/src/applayer.rs
	rust/src/enip/enip.rs
	rust/src/ldap/ldap.rs
	src/app-layer-smtp.c

in the message of commit 6556104 mean though?

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.error.tls.parser 1145 1206 105.33%

Pipeline 24672

@victorjulien
Copy link
Member Author

Consistent w the approved PRs. What does

Conflicts:
	rust/src/applayer.rs
	rust/src/enip/enip.rs
	rust/src/ldap/ldap.rs
	src/app-layer-smtp.c

in the message of commit 6556104 mean though?

I agree this looks like something that doesn't belong there.

@jasonish
Copy link
Member

Consistent w the approved PRs. What does

Conflicts:
	rust/src/applayer.rs
	rust/src/enip/enip.rs
	rust/src/ldap/ldap.rs
	src/app-layer-smtp.c

in the message of commit 6556104 mean though?

I agree this looks like something that doesn't belong there.

Missed in initial review, even though conflicts were resolved. Would be nice to not have it, a bit misleading.

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

Needs commit re-word.

@victorjulien
Copy link
Member Author

Rejected after additional review.

@victorjulien victorjulien deleted the next/710/70x/20250210/v1 branch February 10, 2025 17:49
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.

7 participants