From a19fe14f234efff3892c910b21c19dec9bda10ec Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 21 Oct 2024 15:24:50 +0200 Subject: [PATCH 1/4] detect: don't run pkt sigs on ffr pkts Last packet from the TLS TCP session moves TCP state to CLOSED. This flags the app-layer with APP_LAYER_PARSER_EOF_TS or APP_LAYER_PARSER_EOF_TC depending on the direction of the final packet. This flag will just have been set in a single direction. This leads to the last packet updating the inspect id in that packets direction. At the end of the TLS session a pseudo packet is created, because: - flow has ended - inspected tx id == 0, for at least one direction - total txs is 1 Then a packet rule matches: ``` alert tcp any any -> any 443 (flow: to_server; \ flowbits:isset,tls_error; \ sid:09901033; rev:1; \ msg:"Allow TLS error handling (outgoing packet)"; ) ``` The `SIG_MASK_REQUIRE_REAL_PKT` is not preventing the match, as the `flowbits` keyword doesn't set it. To avoid this match. This patch skips signatures of the `SIG_TYPE_PKT` for flow end packets. Ticket: #7318. (cherry picked from commit 0e4faba79ae7e9fbe36815956cfd44af6d7f9378) --- src/detect.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/detect.c b/src/detect.c index ac1f13906e8d..e02977bd0112 100644 --- a/src/detect.c +++ b/src/detect.c @@ -782,6 +782,10 @@ static inline void DetectRulePacketRules( goto next; // handle sig in DetectRunFrame } + /* skip pkt sigs for flow end packets */ + if ((p->flags & PKT_PSEUDO_STREAM_END) != 0 && s->type == SIG_TYPE_PKT) + goto next; + /* don't run mask check for stateful rules. * There we depend on prefilter */ if ((s->mask & scratch->pkt_mask) != s->mask) { From 6a9facebbea40724f9dabed789aa5485fe182647 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Tue, 26 Nov 2024 21:44:45 +0100 Subject: [PATCH 2/4] detect: log app-layer metadata in alert with single tx Ticket: 7199 Uses a config parameter detect.guess-applayer-tx to enable this behavior (off by default) This feature is requested for use cases with signatures not using app-layer keywords but still targetting application layer transactions, such as pass/drop rule combination, or lua usage. This overrides the previous behavior of checking if the signature has a content match, by checking if there is only one live transaction, in addition to the config parameter being set. (cherry picked from commit f2c37763149f16da17326cb313750052e5a2117d) --- doc/userguide/output/eve/eve-json-output.rst | 15 ++++++++++++++ doc/userguide/upgrade.rst | 4 ++++ etc/schema.json | 4 ++++ src/decode.h | 2 ++ src/detect-engine.c | 9 ++++++++- src/detect.c | 21 +++++++++++--------- src/detect.h | 3 +++ src/output-json-alert.c | 3 +++ suricata.yaml.in | 5 +++++ 9 files changed, 56 insertions(+), 10 deletions(-) diff --git a/doc/userguide/output/eve/eve-json-output.rst b/doc/userguide/output/eve/eve-json-output.rst index 364a80418d39..f6ff28c8ae46 100644 --- a/doc/userguide/output/eve/eve-json-output.rst +++ b/doc/userguide/output/eve/eve-json-output.rst @@ -62,6 +62,21 @@ Alerts are event records for rule matches. They can be amended with metadata, such as the application layer record (HTTP, DNS, etc) an alert was generated for, and elements of the rule. +The alert is amended with application layer metadata for signatures +using application layer keywords. It is also the case for protocols +over UDP as each single packet is expected to contain a PDU. + +For other signatures, the option ``guess-applayer-tx`` +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. + + Metadata:: - alert: diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst index 09c18d96b08b..816d7e22db21 100644 --- a/doc/userguide/upgrade.rst +++ b/doc/userguide/upgrade.rst @@ -59,6 +59,10 @@ Upgrading to 7.0.8 7.0 releases. It will not be provided in Suricata 8. Please fix any rules that depend on this behavior. +- 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. Upgrading 6.0 to 7.0 -------------------- diff --git a/etc/schema.json b/etc/schema.json index 8683bb4816bf..488cf3d511e7 100644 --- a/etc/schema.json +++ b/etc/schema.json @@ -112,6 +112,10 @@ "tx_id": { "type": "integer" }, + "tx_guessed": { + "description": "the signature that triggered this alert didn't tie to a transaction, so the transaction (and metadata) logged is a forced estimation and may not be the one you expect", + "type": "boolean" + }, "files": { "type": "array", "minItems": 1, diff --git a/src/decode.h b/src/decode.h index da41726e806b..103e42c6fc1d 100644 --- a/src/decode.h +++ b/src/decode.h @@ -281,6 +281,8 @@ typedef struct PacketAlert_ { #define PACKET_ALERT_RATE_FILTER_MODIFIED 0x10 /** alert is in a frame, frame_id set */ #define PACKET_ALERT_FLAG_FRAME 0x20 +/** alert in a tx was forced */ +#define PACKET_ALERT_FLAG_TX_GUESSED 0x040 extern uint16_t packet_alert_max; #define PACKET_ALERT_MAX 15 diff --git a/src/detect-engine.c b/src/detect-engine.c index f1c47b0fa356..a7b6ee1f26c4 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -2922,7 +2922,14 @@ static int DetectEngineCtxLoadConf(DetectEngineCtx *de_ctx) SCLogDebug("de_ctx->inspection_recursion_limit: %d", de_ctx->inspection_recursion_limit); - /* parse port grouping whitelisting settings */ + int guess_applayer = 0; + if ((ConfGetBool("detect.guess-applayer-tx", &guess_applayer)) == 1) { + if (guess_applayer == 1) { + de_ctx->guess_applayer = true; + } + } + + /* parse port grouping priority settings */ const char *ports = NULL; (void)ConfGet("detect.grouping.tcp-whitelist", &ports); diff --git a/src/detect.c b/src/detect.c index e02977bd0112..f3cf0e0ba373 100644 --- a/src/detect.c +++ b/src/detect.c @@ -818,16 +818,19 @@ 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 ((s->alproto != ALPROTO_UNKNOWN && pflow->proto == IPPROTO_UDP) || + (de_ctx->guess_applayer && + AppLayerParserGetTxCnt(pflow, pflow->alstate) == txid + 1)) { + // if there is a UDP specific app-layer signature, + // or only one live transaction + // try to use the good tx for the packet direction alert_flags |= PACKET_ALERT_FLAG_TX; + if (pflow->proto != IPPROTO_UDP) { + alert_flags |= PACKET_ALERT_FLAG_TX_GUESSED; + } } } AlertQueueAppend(det_ctx, s, p, txid, alert_flags); diff --git a/src/detect.h b/src/detect.h index f147d0fda8e7..2379d3253829 100644 --- a/src/detect.h +++ b/src/detect.h @@ -885,6 +885,9 @@ typedef struct DetectEngineCtx_ { /* maximum recursion depth for content inspection */ int inspection_recursion_limit; + /* force app-layer tx finding for alerts with signatures not having app-layer keywords */ + bool guess_applayer; + /* registration id for per thread ctx for the filemagic/file.magic keywords */ int filemagic_thread_ctx_id; diff --git a/src/output-json-alert.c b/src/output-json-alert.c index 4005c48800c2..72127e5d0f14 100644 --- a/src/output-json-alert.c +++ b/src/output-json-alert.c @@ -378,6 +378,9 @@ void AlertJsonHeader(void *ctx, const Packet *p, const PacketAlert *pa, JsonBuil if (pa->flags & PACKET_ALERT_FLAG_TX) { jb_set_uint(js, "tx_id", pa->tx_id); } + if (pa->flags & PACKET_ALERT_FLAG_TX_GUESSED) { + jb_set_bool(js, "tx_guessed", true); + } jb_open_object(js, "alert"); diff --git a/suricata.yaml.in b/suricata.yaml.in index ba9005dc3e4c..504bfd0b66b8 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -1677,6 +1677,11 @@ detect: toserver-groups: 25 sgh-mpm-context: auto inspection-recursion-limit: 3000 + # 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. + # 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. #delayed-detect: yes From cea8a86b22ad1d22795e7bb89ead184178eda9dd Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 11 Dec 2024 12:04:34 +0100 Subject: [PATCH 3/4] detect: re-add app-layer to alerts on stream matches The `guess-applayer-tx` work also removed the stream match condition for adding app-layer metadata to alerts. This is a behavior change that is not desired at this point, so this commit reverts that part of the changes. We keep the exising logging of app-layer metadata if the match was in the stream. --- src/detect.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/detect.c b/src/detect.c index f3cf0e0ba373..82aaafcb94e1 100644 --- a/src/detect.c +++ b/src/detect.c @@ -822,6 +822,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) || + (alert_flags & PACKET_ALERT_FLAG_STREAM_MATCH) || (de_ctx->guess_applayer && AppLayerParserGetTxCnt(pflow, pflow->alstate) == txid + 1)) { // if there is a UDP specific app-layer signature, From 58f97bfd020a6d74272eb0eb20ca3fa64a30e74c Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Wed, 11 Dec 2024 15:11:29 +0100 Subject: [PATCH 4/4] doc: improve documentation about guess-applayer-tx Ticket: 7199 --- doc/userguide/configuration/suricata-yaml.rst | 7 +++++++ doc/userguide/output/eve/eve-json-output.rst | 10 +++++----- doc/userguide/upgrade.rst | 7 +++++-- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/doc/userguide/configuration/suricata-yaml.rst b/doc/userguide/configuration/suricata-yaml.rst index 7bcabdb77fed..6956af836437 100644 --- a/doc/userguide/configuration/suricata-yaml.rst +++ b/doc/userguide/configuration/suricata-yaml.rst @@ -632,6 +632,7 @@ The detection-engine builds internal groups of signatures. Suricata loads signat toserver-groups: 25 sgh-mpm-context: auto inspection-recursion-limit: 3000 + 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 @@ -666,6 +667,12 @@ 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 ``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 f6ff28c8ae46..d9462952e1bf 100644 --- a/doc/userguide/output/eve/eve-json-output.rst +++ b/doc/userguide/output/eve/eve-json-output.rst @@ -71,11 +71,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 816d7e22db21..6da52adb1f88 100644 --- a/doc/userguide/upgrade.rst +++ b/doc/userguide/upgrade.rst @@ -61,8 +61,11 @@ Upgrading to 7.0.8 behavior. - 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 --------------------