Detect log alert tx one 7449 v1

doc/userguide/configuration/suricata-yaml.rst

@@ -660,6 +660,7 @@ The detection-engine builds internal groups of signatures. Suricata loads signat
     sgh-mpm-context: auto
     inspection-recursion-limit: 3000
     stream-tx-log-limit: 4
+    guess-applayer-tx: no
 
 At all of these options, you can add (or change) a value. Most
 signatures have the adjustment to focus on one direction, meaning
@@ -694,11 +695,17 @@ complicated issues. It could end up in an 'endless loop' due to a bug,
 meaning it will repeat its actions over and over again. With the
 option inspection-recursion-limit you can limit this action.
 
-The stream-tx-log-limit defines the maximum number of times a
+The ``stream-tx-log-limit`` defines the maximum number of times a
 transaction will get logged for rules without app-layer keywords.
 This is meant to avoid logging the same data an arbitrary number
 of times.
 
+The ``guess-applayer-tx`` option controls whether the engine will try to guess
+and tie a transaction to a given alert if the matching signature doesn't have
+app-layer keywords. If enabled, AND ONLY ONE LIVE TRANSACTION EXISTS, that
+transaction's data will be added to the alert metadata. Note that this may not
+be the expected data, from an analyst's perspective.
+
 *Example 4	Detection-engine grouping tree*
 
 .. image:: suricata-yaml/grouping_tree.png

doc/userguide/output/eve/eve-json-output.rst

@@ -79,11 +79,11 @@ can be used to force the detect engine to tie a transaction
 to an alert.
 This transaction is not guaranteed to be the relevant one,
 depending on your use case and how you define relevant here.
-If there are multiple live transactions, none will get
-picked up.
-The alert event will have ``"tx_guessed": true`` to recognize
-these alerts.
-
+**WARNING: If there are multiple live transactions, none will get
+picked up.** This is to reduce the chances of logging unrelated data, and may
+lead to alerts being logged without metadata, in some cases.
+The alert event will have ``tx_guessed: true`` to recognize
+such alerts.
 
 Metadata::
 

doc/userguide/upgrade.rst

@@ -101,8 +101,11 @@ Logging changes
   sometimes logged with a dash instead of an underscore.
 - Application layer metadata is logged with alerts by default **only for rules that
   use application layer keywords**. For other rules, the configuration parameter
-  ``detect.guess-applayer-tx`` can be used to force the detect engine to find a
-  transaction, which is not guaranteed to be the one you expect.
+  ``detect.guess-applayer-tx`` can be used to force the detect engine to guess a
+  transaction, which is not guaranteed to be the one you expect. **In this case,
+  the engine will NOT log any transaction metadata if there is more than one
+  live transaction, to reduce the chances of logging unrelated data.** This may
+  lead to what looks like a regression in behavior, but it is a considered choice.
 
 Upgrading 6.0 to 7.0
 --------------------

src/detect.c

@@ -725,6 +725,28 @@ static inline void DetectRunPrefilterPkt(
 #endif
 }
 
+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 +843,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

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.