-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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/dataset: fix dataset set logic #8123
Conversation
The set operation of dataset keyword was not done only signature when there is a full match. This was not correct with regards to expectation. 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
Setting it as draft to do more testing and analysis. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #8123 +/- ##
==========================================
- Coverage 81.67% 81.61% -0.06%
==========================================
Files 962 962
Lines 276781 276833 +52
==========================================
- Hits 226052 225939 -113
- Misses 50729 50894 +165
Flags with carried forward coverage won't be shown. Click here to find out more. |
I wonder if this will work if there are more than one dataset set in a rule. |
Can you add a SV test for this case? |
WARNING:
Pipeline 10318 |
Should it be just a second or third dataset to be set and make sure that those are also not updated if the rule does not match? I did a test with just adding more datasets that would also set the data but to a different setname and it was correct. But maybe we can think of even more complex rules with different data to be set in different sets. |
Yeah lets start with that. If you see obvious other scenarios we can add tests for those as well. |
OISF/suricata-verify#959 S-V for this PR |
WARNING:
Pipeline 10318 |
ERROR: ERROR: QA failed on TREX_GENERIC_stats_chk. Pipeline 10318 |
@victorjulien do you need help with reviewing this ? |
ERROR: ERROR: QA failed on TREX_GENERIC_stats_chk. Pipeline 10318 |
} DetectDatasetData; | ||
|
||
typedef struct DetectDatasetMatchData_ { | ||
const uint8_t *data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just have a pointer to InspectionBuffer
here, or even just an index into the array of buffers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DetectEngineContentInspectionInternal uses const uint8_t *buffer, const uint32_t buffer_len
and not an InspectionBuffer
ERROR: ERROR: QA failed on TREX_GENERIC_stats_chk. Pipeline 10318 |
@regit will you work on this again ? |
ERROR: ERROR: QA failed on TREX_GENERIC_stats_chk. Pipeline 10318 |
I will take this over |
Continued in #11600 |
Update of #8114.
The set operation of dataset keyword was not done only signature when there is a full match. This was not correct with regards to expectation.
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: #5576
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5576
Describe changes:
suricata-verify-pr: 959