detect: investigation on single-pkt flows inspection (5180) - v2 #14704
Conversation
During initialization, the engine reports how many rules were loaded, as well as which types. Pkt-only or stream-pkt rules would cause a "hole" in such stats, as they're not counted.
Commit 497394e removed inspection of app-proto txs for packets without an established TCP connection. But this check meant that the first packet seen in a session pick mid-stream could go without inspection (previous bug 5510 seemed to point towards this behavior, too). If the flow has more packets, the stream would be inspected as part of the upcoming packets and this would go unnoticed. In a single-packet flow, however, the inspection for the packed would be skipped. Although this might not affect alerts -- as they could be processed as part of the flow timeout logic, the actual traffic could be evaded in IPS, in case of a drop rule. From the above, the most visible scenario is when there is only one packet on the flow, as then the engine doesn't have "more time" to pick-up real-packets to inspect for that given flow. But certain tests show that this can also happen for more than one packet scenarios: there will be one less drop event, or traffic from a packet that should have been already dropped will be logged. This led to the possibility of a real packet not being blocked, in IPS, or matched against rules, as the corresponding portion of the stream was only inspected later, as part of the stream/flow-timeout logic. Marking the packet as having a stream established if midstream is enabled fixes those without exposing midstream logic to the detection engine. Related to Bug OISF#5510 As part of Bug OISF#5180
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14704 +/- ##
==========================================
- Coverage 82.13% 82.11% -0.02%
==========================================
Files 1011 1011
Lines 262925 262939 +14
==========================================
- Hits 215954 215918 -36
- Misses 46971 47021 +50
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline = 29305 |
| "TCP_ESTABLISHED", ssn); | ||
| /* Since we're picking this session mid-stream, update the packet flag, so that detection | ||
| * will inspect it */ | ||
| p->flags |= PKT_STREAM_EST; |
There was a problem hiding this comment.
I think this makes sense.
Wonder if we could do it in more generic way, like:
diff --git a/src/stream-tcp.c b/src/stream-tcp.c
index f7171fff2b..e2e257c4cc 100644
--- a/src/stream-tcp.c
+++ b/src/stream-tcp.c
@@ -5804,10 +5804,9 @@ int StreamTcpPacket (ThreadVars *tv, Packet *p, StreamTcpThread *stt,
skip:
StreamTcpPacketCheckPostRst(ssn, p);
-
- if (ssn->state >= TCP_ESTABLISHED) {
- p->flags |= PKT_STREAM_EST;
- }
+ }
+ if (ssn && ssn->state >= TCP_ESTABLISHED) {
+ p->flags |= PKT_STREAM_EST;
}
if (ssn != NULL) {There was a problem hiding this comment.
This impacts the tests, must understand why and looks like the most correct results.
There was a problem hiding this comment.
I should mention I didn't test this :)
There was a problem hiding this comment.
If my understanding is correct -- that we would have this instead of what the PR suggests --, it doesn't to work, because, again, if I understood correctly, this is in an else that's called after checking for the session's existence. As we will call StreamTcpPacketStateNone after the first check, this means that we don't set PKT_STREAM_FLAG for the first packet, and thus still skip the first packet seen for the flow, in midstream session, with this approach.
3 participants
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5180
Previous PR: #14677
Describe changes:
Provide values to any of the below to override the defaults.
SV_BRANCH=OISF/suricata-verify#2888