From 27433df253fe3c03e3719d38110f80a9495fafd8 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Wed, 11 Dec 2024 15:11:29 +0100 Subject: [PATCH 1/2] doc: improve documentation about guess-applayer-tx Ticket: 7199 --- doc/userguide/configuration/suricata-yaml.rst | 9 ++++++++- doc/userguide/output/eve/eve-json-output.rst | 10 +++++----- doc/userguide/upgrade.rst | 7 +++++-- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/doc/userguide/configuration/suricata-yaml.rst b/doc/userguide/configuration/suricata-yaml.rst index 8400f92193a5..8004ecce9167 100644 --- a/doc/userguide/configuration/suricata-yaml.rst +++ b/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 diff --git a/doc/userguide/output/eve/eve-json-output.rst b/doc/userguide/output/eve/eve-json-output.rst index da80a3fd196b..580504d1f7ea 100644 --- a/doc/userguide/output/eve/eve-json-output.rst +++ b/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:: diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst index 2b16dd31a35c..4bf74b65284d 100644 --- a/doc/userguide/upgrade.rst +++ b/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 -------------------- From 1b9956741999325acaf52a4a0277a1042a25ff73 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Wed, 11 Dec 2024 14:50:49 +0100 Subject: [PATCH 2/2] 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 --- src/detect.c | 25 +++++++++++++++++++++++-- suricata.yaml.in | 8 ++++---- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/detect.c b/src/detect.c index ce107a52bc22..10823742636a 100644 --- a/src/detect.c +++ b/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 diff --git a/suricata.yaml.in b/suricata.yaml.in index 0c71090cb3bd..4bc9e87aa2af 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.