detect: count keywords for multi-buffer #13783
Conversation
Ticket: 5044
| "FtpDataStateValues", | ||
| "HTTP2TransactionState", | ||
| "DataRepType", | ||
| "DETECT_COUNT_INDEX", |
There was a problem hiding this comment.
The hack is to reuse InspectionMultiBufferGetDataPtr function prototype, with passing special index 0xFFFFFFFF
|
|
||
| /// Intermediary function used in detect-email.c to access data from the MimeStateSMTP structure | ||
| /// for array header fields. | ||
| /// The hname parameter determines which data will be returned. |
There was a problem hiding this comment.
TODO: remove this comment
| } | ||
|
|
||
| sm_list = s->init_data->list; | ||
| // TODO check it is a multi buffer and not a single buffer |
There was a problem hiding this comment.
And add tests about impossible rules like http.uri; count:1; and raw tcp content: "abc"; count: 1;
| return false; | ||
| } | ||
|
|
||
| if (idx == DETECT_COUNT_INDEX) { |
There was a problem hiding this comment.
Need to do that for all the keywords listed in #13752
| SCDetectHelperKeywordRegister(&kw); | ||
| g_mime_email_received_buffer_id = SCDetectHelperMultiBufferMpmRegister("email.received", | ||
| "MIME EMAIL RECEIVED", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailReceivedData); | ||
| g_mime_email_received_buffer_id = SCDetectHelperMultiBufferProgressMpmRegister("email.received", |
There was a problem hiding this comment.
That is an interesting point :
- now we run detection on email.received multi-buffer even if the progress is not complete yet, that allows early detection
- But with
count, I think we only want the finalcountonce the progress is far enough
There was a problem hiding this comment.
now we run detection on email.received multi-buffer even if the progress is not complete yet, that allows early detection
If the progress is not complete, do we still have the entire buffer needed to match?
There was a problem hiding this comment.
once the progress is far enough
how do we know that?
There was a problem hiding this comment.
do we still have the entire buffer needed to match?
Depending on the packets, we have some of the instances of the multi-buffer.
You can have
- 1 packet with 0 email.received
- another packet after with 1 email.received
- a final packet with 3 email.received
Does that reply to your question ?
how do we know that?
We can access the tx progress if we need...
There was a problem hiding this comment.
Depending on the packets, we have some of the instances of the multi-buffer.
So, will the inspection be run every time a packet comes?
We can access the tx progress if we need
yes, but, could you please tell how do we define that the progress is "far enough"?
There was a problem hiding this comment.
For example, let's take HTTP which has the progress
- request_line = 0
- headers = 1
- body = 2
And we want to count the number of http headers
We have the following packets
- half of request line : progress is request_line = 0
- end of request line : progress is now headers = 1, headers count is 0
- first header : progress is headers = 1, headers count is 1
- 2 last headers : progress is now body = 2, headers count is 3
- end of request with its body
In this case, progress is "far enough" when it is 2
So, will the inspection be run every time a packet comes?
inspection does not run on first packet (progress not far enough) and neither on last packet (once progress is above, we only run once)
but it runs on the 3 packets in the middle
There was a problem hiding this comment.
Thanks a lot for explaining! 🙇🏽♀️ This sounds very good.
There was a problem hiding this comment.
Maybe we can rely on the tx hooks...
| DEBUG_VALIDATE_BUG_ON(1); | ||
| return false; | ||
| } | ||
| DetectU32Data *du32 = (DetectU32Data *)ctx; |
There was a problem hiding this comment.
Maybe we need to check the tx progress and allow only du32 modes "greater than" if progress means we still can get the count to grow
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13783 +/- ##
==========================================
- Coverage 83.72% 83.71% -0.01%
==========================================
Files 1011 1012 +1
Lines 275117 275178 +61
==========================================
+ Hits 230328 230379 +51
- Misses 44789 44799 +10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 27325 |
|
I would also like to support syntax |
|
Status : first make progress on #13823 |
|
Replaced by #13902 |
3 participants
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5044
Describe changes:
SV_BRANCH=OISF/suricata-verify#2634
Draft :
match_count, so we can test a dns request has 3 names, including 2 that match content "toto" see Dataset set postmatch 5576 v25 #13475 with DETECT_ENGINE_INSPECT_SIG_MATCH_MORE_BUFFeedback about design is welcome