From: Philippe Antoine <pantoine@oisf.net>
Date: Wed, 11 Dec 2024 13:50:49 +0000 (+0100)
Subject: detect: improve tx_id guessing for unidirectional protocols
X-Git-Tag: suricata-8.0.0-beta1~617
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bdcb59373858afd08f67525b2bb5f55876340b78;p=thirdparty%2Fsuricata.git

detect: improve tx_id guessing for unidirectional protocols

So we get:
1. request arrives - buffered due to not ackd
2. response arrives, acks request - request is now parsed, response isn't
3. ack for response, response parsed. Then detect runs for request,
generates alert. We now have 2 txs. txid will be 0 from AppLayerParserGetTransactionInspectId

But txid 1 is unidirectional in the other way, so we can use txid 0
metadata for logging

Ticket: 7449
---

diff --git a/src/detect.c b/src/detect.c
index ce107a52bc..b96d96d93a 100644
--- a/src/detect.c
+++ b/src/detect.c
@@ -725,6 +725,38 @@ static inline void DetectRunPrefilterPkt(
 #endif
 }
 
+/** \internal
+ *  \brief check if the tx whose id is given is the only one
+ *  live transaction for the flow in the given direction
+ *
+ *  \param f flow
+ *  \param txid transaction id
+ *  \param dir direction
+ *
+ *  \retval bool true if we are sure this tx is the only one live in said direction
+ */
+static bool IsOnlyTxInDirection(Flow *f, uint64_t txid, uint8_t dir)
+{
+    uint64_t tx_cnt = AppLayerParserGetTxCnt(f, f->alstate);
+    if (tx_cnt == txid + 1) {
+        // only live tx
+        return true;
+    }
+    if (tx_cnt == txid + 2) {
+        // 2 live txs, one after us
+        void *tx = AppLayerParserGetTx(f->proto, f->alproto, f->alstate, txid + 1);
+        if (tx) {
+            AppLayerTxData *txd = AppLayerParserGetTxData(f->proto, f->alproto, tx);
+            // test if the other tx is unidirectional in the other way
+            if (txd &&
+                    (AppLayerParserGetTxDetectFlags(txd, dir) & APP_LAYER_TX_SKIP_INSPECT_FLAG)) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
 static inline void DetectRulePacketRules(
     ThreadVars * const tv,
     DetectEngineCtx * const de_ctx,
@@ -821,8 +853,7 @@ static inline void DetectRulePacketRules(
             uint8_t dir = (p->flowflags & FLOW_PKT_TOCLIENT) ? STREAM_TOCLIENT : STREAM_TOSERVER;
             txid = AppLayerParserGetTransactionInspectId(pflow->alparser, dir);
             if ((s->alproto != ALPROTO_UNKNOWN && pflow->proto == IPPROTO_UDP) ||
-                    (de_ctx->guess_applayer &&
-                            AppLayerParserGetTxCnt(pflow, pflow->alstate) == txid + 1)) {
+                    (de_ctx->guess_applayer && IsOnlyTxInDirection(pflow, txid, dir))) {
                 // if there is a UDP specific app-layer signature,
                 // or only one live transaction
                 // try to use the good tx for the packet direction
diff --git a/suricata.yaml.in b/suricata.yaml.in
index 0c71090cb3..4bc9e87aa2 100644
--- a/suricata.yaml.in
+++ b/suricata.yaml.in
@@ -1705,10 +1705,10 @@ detect:
   inspection-recursion-limit: 3000
   # maximum number of times a tx will get logged for rules without app-layer keywords
   # stream-tx-log-limit: 4
-  # try to tie an app-layer transaction for rules without app-layer keywords
-  # if there is only one live transaction for the flow
-  # allows to log app-layer metadata in alert
-  # but the transaction may not be the relevant one.
+  # Try to guess an app-layer transaction for rules without app-layer keywords,
+  # ONLY IF there is just one live transaction for the flow.
+  # This allows logging app-layer metadata in alert - the transaction may not
+  # be the relevant one for the alert.
   # guess-applayer-tx: no
   # If set to yes, the loading of signatures will be made after the capture
   # is started. This will limit the downtime in IPS mode.