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.

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

Dataset set postmatch 5576 v13 #12126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Dataset set postmatch 5576 v13 #12126

wants to merge 3 commits into from
+268 −60

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • detect/dataset: delay set operation after signature full match

SV_BRANCH=OISF/suricata-verify#2093

#12057 rebased after merge of #12056 which was included here

The design is :

  • detect: postmatch can run AppLayerTxMatch callbacks (in its own commit)
  • usage of delay_postmatch: buffers using dataset/set are put at the tail of inspections whatever their progress
  • usage of DETECT_ENGINE_INSPECT_SIG_MATCH_MORE_BUF: dataset may return this new case to tell that ok we match on a multi-buffer, but we still want to try all occurrences of a multi-buffer (instead of returning a simple early match)

How do you like the git history/commits ?

@catenacyber
ci/live: test for sid 1 existence, not in last position
8467a41
@catenacyber
detect: postmatch can run AppLayerTxMatch callbacks
c29fa0d 
Will be useful for dataset, when it needs to find a transaction
buffer again.
@catenacyber
detect/dataset: delay set operation after signature full match
9edb512 
The set operation of dataset keyword was done even if signature
did not fully match, which is not the expected behavior.
We want dataset to behave like flowbits for instance.

This patch changes the behavior of the dataset keyword to do a
match and a post match for the set operation.
The postmatch retrieves the data, using the list identifier
associated to the buffer for this signature.

This avoids to store the buffer(s), when we do not have a
dedicated storage (per signature and per tx) that can own
and clean arbitrary buffers over multiple packets, in
the case the transaction spans over multiple packets
with different tx progresses for instance.
If detection runs on one packet, the InspectionBuffer are
cached and fast to get.
The most expensive case if for multi buffers, where we
need to run detection again, to see which occurences
match all payload keywords and should be added in the dataset.

Ticket: OISF#5576
@catenacyber catenacyber mentioned this pull request Nov 18, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Nov 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.23529% with 20 lines in your changes missing coverage. Please review.

Project coverage is 83.11%. Comparing base (5d766df) to head (9edb512).

Additional details and impacted files 
@@             Coverage Diff             @@
##           master   #12126       +/-   ##
===========================================
+ Coverage   62.68%   83.11%   +20.43%     
===========================================
  Files         840      909       +69     
  Lines      153669   258014   +104345     
===========================================
+ Hits        96323   214445   +118122     
+ Misses      57346    43569    -13777
Flag Coverage Δ
fuzzcorpus 60.95% <77.05%> (?)
livemode 19.50% <43.52%> (?)
pcap 44.36% <18.82%> (?)
suricata-verify 62.72% <88.23%> (+0.04%) ⬆️
unittests 59.23% <18.23%> (?)

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

---- 🚨 Try these New Features:

@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp 40 42 105.0%

Pipeline 23411

@ct0br0
Copy link

ct0br0 commented Nov 18, 2024

interesting, #12084 seems to have a similar dce bump 40->42

@catenacyber
Copy link
Contributor Author

interesting, #12084 seems to have a similar dce bump 40->42

Let's retry QA then

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

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

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@catenacyber @suricata-qa @ct0br0