detect: log app-layer metadata in alert with single tx #12166
Conversation
Ticket: 7199 When there is a single live transaction, we cannot pick a wrong transaction to log, even if the rule does not use app-layer keywords.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12166 +/- ##
==========================================
+ Coverage 49.81% 49.83% +0.02%
==========================================
Files 909 909
Lines 257904 257904
==========================================
+ Hits 128467 128522 +55
+ Misses 129437 129382 -55
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 23584 |
| txid = AppLayerParserGetTransactionInspectId(pflow->alparser, dir); | ||
| if ((alert_flags & PACKET_ALERT_FLAG_STREAM_MATCH) || | ||
| (s->alproto != ALPROTO_UNKNOWN && pflow->proto == IPPROTO_UDP) || | ||
| AppLayerParserGetTxCnt(pflow, pflow->alstate) == txid + 1) { |
There was a problem hiding this comment.
This is not what I had in mind. The idea was that for a protocol like tls, where we have only one tx max, we use that tx. However, in other protocols the "live" tx may not necessarily be relevant? There are protocols like smb, nfs that have transactions with different life times. I'm not sure how reliable it would be to pick the last. Before accepting a patch on that I would like to see a per protocol analysis of how we can be sure that this condition provides us with a relevant tx. We've decided it's worse to log the wrong one, than to log none.
There was a problem hiding this comment.
Oh.
I understand that you say that only one tx is not enough, what is important is to know if every byte of the stream belong to the transaction...
Even for TLS, this is not obvious, as the encrypted bytes do not belong to the transaction, and the inspect id logic should work but not obvious to me...
On top of my head :
- HTTP1 : every byte is in a transaction ✅
- HTTP2 : every byte is in a transaction ✅
- DNS : every byte is in a transaction ✅
- TLS : only the first bytes before encryption are in a transaction
⚠️ - SMTP : first commands are not in a transaction 🔴
- SSH : seems to be like TLS
⚠️ even if I am not sure if we want to make the plaintext records part evolve... - SMB, NFS : I do not know ❓
- many others...
Should we have a dirty patch to allow only certain app-layer protocols ?
There was a problem hiding this comment.
Another problematic case :
signature is like alert ip any any -> any any (sid: 1; flow.age: >1; flow: to_server; )
- 3 way handshake
- first HTTP2 request
- acked
- after one second, second HTTP2 request begins with a packet with the start of big HTTP2 frame :
- parser returns
AppLayerResult::incomplete, does not create tx - signature triggers
- the only transaction present is the first one (AppLayerParserGetTxCnt==1) but the first request had all its data before flow.age was 1 second old, so should not be logged
- parser returns
|
Something next in #12168 |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7199
May also solve https://redmine.openinfosecfoundation.org/issues/7406 and https://redmine.openinfosecfoundation.org/issues/7350
Describe changes:
SV_BRANCH=OISF/suricata-verify#2141
Is this the right way to solve most of the cases as I remember discussing with someone ?
#12161 with better check and comment for one live transaction