Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

detect/dataset: delay set operation after signature full match #11600

Closed
wants to merge 2 commits into from
Closed

detect/dataset: delay set operation after signature full match #11600

wants to merge 2 commits into from

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#2000

Side note: the limitation described for flowvar in https://redmine.openinfosecfoundation.org/issues/7197 likely also applies here to dataset

@regit @catenacyber
detect/dataset: delay set operation after signature full match
07dff4f 
The set operation of dataset keyword was done even if signature
did not fully match.

This patch changes the behavior of the dataset keyword to do a
match and a post match for the set operation. In the match, the
buffer data that needs to end up in the set is captured and in
post match the dataset is updated (if ever the signature is fully
matching).

Ticket: OISF#5576
@catenacyber catenacyber mentioned this pull request Aug 1, 2024
Closed
3 tasks
@catenacyber catenacyber marked this pull request as draft August 1, 2024 20:52
@catenacyber
Copy link
Contributor Author

Side note: the limitation described for flowvar in https://redmine.openinfosecfoundation.org/issues/7197 likely also applies here to dataset

Actually, this is a bigger problem indeed because unlike flowvar, DetectThreadCtxGetKeywordThreadCtx do not reset the data after each detection run, and the pointer to the buffer can have become dangling in case of some realloc (or meaningless anyways)

@codecov Codecov
Copy link

codecov bot commented Aug 1, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.31373% with 8 lines in your changes missing coverage. Please review.

Project coverage is 70.35%. Comparing base (42e5e55) to head (03e0617).

❗ There is a different number of reports uploaded between BASE (42e5e55) and HEAD (03e0617). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (42e5e55) HEAD (03e0617)
suricata-verify 1 0
fuzzcorpus 1 0
Additional details and impacted files 
@@             Coverage Diff             @@
##           master   #11600       +/-   ##
===========================================
- Coverage   82.52%   70.35%   -12.17%     
===========================================
  Files         923      923               
  Lines      248808   248649      -159     
===========================================
- Hits       205317   174944    -30373     
- Misses      43491    73705    +30214
Flag Coverage Δ
fuzzcorpus ?
livemode 18.71% <68.62%> (-0.07%) ⬇️
pcap 44.07% <17.64%> (-0.07%) ⬇️
suricata-verify ?
unittests 59.07% <15.68%> (-0.02%) ⬇️

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

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on build_asan.

Pipeline 21818

@catenacyber catenacyber force-pushed the dataset-set-postmatch-5576-v3 branch from 33163a6 to d5286fd Compare August 2, 2024 09:15
@catenacyber
Copy link
Contributor Author

Could we somehow delay the dataset check as the last operation before postmatch ?..

@catenacyber catenacyber force-pushed the dataset-set-postmatch-5576-v3 branch from d5286fd to 03e0617 Compare August 2, 2024 14:29
@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPW2_single_alerts_cmp.

ERROR: QA failed on SURI_TLPW2_autofp_alerts_cmp.

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

ERROR: QA failed on IPS_AFP_drop_chk.

field baseline test %
IPS_AFP_stats_chk
.ips.accepted 367733210 342623210 93.17%
.ips.blocked 747360 25857360 3459.83%
.ips.drop_reason.flow_drop 680400 25736400 3782.54%
.ips.drop_reason.rules 66960 120960 180.65%
.flow.end.state.established 550799 604799 109.8%
.flow.end.state.closed 1048672 994672 94.85%
.flow.end.tcp_state.established 169560 223560 131.85%
.flow.end.tcp_state.closed 1048672 994672 94.85%
.detect.alert 185760 293760 158.14%
TREX_GENERIC_stats_chk
.detect.alert 86688 129368 149.23%

Pipeline 21849

@inashivb inashivb self-requested a review August 12, 2024 12:17
@catenacyber catenacyber mentioned this pull request Aug 12, 2024
Closed
@catenacyber
Copy link
Contributor Author

Continued in #11623

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

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

@inashivb inashivb Awaiting requested review from inashivb

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 @regit