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

detect: log app-layer metadata in alert with single tx #12166

Closed
wants to merge 1 commit into from
Closed

detect: log app-layer metadata in alert with single tx #12166

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions src/detect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,15 +813,16 @@ static inline void DetectRulePacketRules(
DetectRunPostMatch(tv, det_ctx, p, s);

uint64_t txid = PACKET_ALERT_NOTX;
if ((alert_flags & PACKET_ALERT_FLAG_STREAM_MATCH) ||
(s->alproto != ALPROTO_UNKNOWN && pflow->proto == IPPROTO_UDP)) {
// if there is a stream match (TCP), or
// a UDP specific app-layer signature,
// try to use the good tx for the packet direction
if (pflow->alstate) {
uint8_t dir =
(p->flowflags & FLOW_PKT_TOCLIENT) ? STREAM_TOCLIENT : STREAM_TOSERVER;
txid = AppLayerParserGetTransactionInspectId(pflow->alparser, dir);
if (pflow && pflow->alstate) {
uint8_t dir = (p->flowflags & FLOW_PKT_TOCLIENT) ? STREAM_TOCLIENT : STREAM_TOSERVER;
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// if there is a stream match (TCP), or
// a UDP specific app-layer signature,
// or only one live transaction
// try to use the good tx for the packet direction
void *tx_ptr =
AppLayerParserGetTx(pflow->proto, pflow->alproto, pflow->alstate, txid);
AppLayerTxData *txd =
Expand Down
Loading