From de9413c654f6c3059fa21ba3df0f649435515543 Mon Sep 17 00:00:00 2001
From: Philippe Antoine <pantoine@oisf.net>
Date: Fri, 31 May 2024 14:06:55 +0200
Subject: [PATCH] detect: safety for app-layer logging of stream-only rules

If a stream-only rule matches, and we find a tx where we
want to log the app-layer data, store into the tx data that
we already logged, so that we do not log again the app-layer metadata

Ticket: 7085
---
 doc/userguide/configuration/suricata-yaml.rst |  6 ++++++
 rust/src/applayer.rs                          |  4 ++++
 src/detect-engine.c                           | 11 +++++++++++
 src/detect.c                                  | 11 +++++++++--
 src/detect.h                                  |  3 +++
 suricata.yaml.in                              |  2 ++
 6 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/doc/userguide/configuration/suricata-yaml.rst b/doc/userguide/configuration/suricata-yaml.rst
index b348a49fc2..ce7cd76a85 100644
--- a/doc/userguide/configuration/suricata-yaml.rst
+++ b/doc/userguide/configuration/suricata-yaml.rst
@@ -659,6 +659,7 @@ The detection-engine builds internal groups of signatures. Suricata loads signat
       toserver-groups: 25
     sgh-mpm-context: auto
     inspection-recursion-limit: 3000
+    stream-tx-log-limit: 4
 
 At all of these options, you can add (or change) a value. Most
 signatures have the adjustment to focus on one direction, meaning
@@ -693,6 +694,11 @@ 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
+transaction will get logged for a stream-only rule match.
+This is meant to avoid logging the same data an arbitrary number
+of times.
+
 *Example 4	Detection-engine grouping tree*
 
 .. image:: suricata-yaml/grouping_tree.png
diff --git a/rust/src/applayer.rs b/rust/src/applayer.rs
index 92bab1d82f..f863546bbe 100644
--- a/rust/src/applayer.rs
+++ b/rust/src/applayer.rs
@@ -114,6 +114,8 @@ pub struct AppLayerTxData {
     /// STREAM_TOCLIENT: file tx , files only in toclient dir
     /// STREAM_TOSERVER|STREAM_TOCLIENT: files possible in both dirs
     pub file_tx: u8,
+    /// Number of times this tx data has already been logged for one stream match
+    pub stream_logged: u8,
 
     /// detection engine flags for use by detection engine
     detect_flags_ts: u64,
@@ -152,6 +154,7 @@ impl AppLayerTxData {
             files_stored: 0,
             file_flags: 0,
             file_tx: 0,
+            stream_logged: 0,
             detect_flags_ts: 0,
             detect_flags_tc: 0,
             de_state: std::ptr::null_mut(),
@@ -174,6 +177,7 @@ impl AppLayerTxData {
             files_stored: 0,
             file_flags: 0,
             file_tx: 0,
+            stream_logged: 0,
             detect_flags_ts,
             detect_flags_tc,
             de_state: std::ptr::null_mut(),
diff --git a/src/detect-engine.c b/src/detect-engine.c
index c4e79682f1..58b5c9967c 100644
--- a/src/detect-engine.c
+++ b/src/detect-engine.c
@@ -2904,6 +2904,17 @@ static int DetectEngineCtxLoadConf(DetectEngineCtx *de_ctx)
     SCLogDebug("de_ctx->inspection_recursion_limit: %d",
                de_ctx->inspection_recursion_limit);
 
+    // default value is 4
+    de_ctx->stream_tx_log_limit = 4;
+    if (ConfGetInt("detect.stream-tx-log-limit", &value) == 1) {
+        if (value >= 0 && value <= UINT8_MAX) {
+            de_ctx->stream_tx_log_limit = (uint8_t)value;
+        } else {
+            SCLogWarning("Invalid value for detect-engine.stream-tx-log-limit: must be between 0 "
+                         "and 255, will default to 4");
+        }
+    }
+
     /* parse port grouping whitelisting settings */
 
     const char *ports = NULL;
diff --git a/src/detect.c b/src/detect.c
index 70612516fb..03fa843706 100644
--- a/src/detect.c
+++ b/src/detect.c
@@ -25,7 +25,6 @@
 
 #include "suricata-common.h"
 #include "suricata.h"
-#include "conf.h"
 
 #include "decode.h"
 #include "packet.h"
@@ -823,7 +822,15 @@ static inline void DetectRulePacketRules(
                 uint8_t dir =
                         (p->flowflags & FLOW_PKT_TOCLIENT) ? STREAM_TOCLIENT : STREAM_TOSERVER;
                 txid = AppLayerParserGetTransactionInspectId(pflow->alparser, dir);
-                alert_flags |= PACKET_ALERT_FLAG_TX;
+                void *tx_ptr =
+                        AppLayerParserGetTx(pflow->proto, pflow->alproto, pflow->alstate, txid);
+                AppLayerTxData *txd =
+                        tx_ptr ? AppLayerParserGetTxData(pflow->proto, pflow->alproto, tx_ptr)
+                               : NULL;
+                if (txd && txd->stream_logged < de_ctx->stream_tx_log_limit) {
+                    alert_flags |= PACKET_ALERT_FLAG_TX;
+                    txd->stream_logged++;
+                }
             }
         }
         AlertQueueAppend(det_ctx, s, p, txid, alert_flags);
diff --git a/src/detect.h b/src/detect.h
index 4df2dfa3d6..cf8f4335b1 100644
--- a/src/detect.h
+++ b/src/detect.h
@@ -884,6 +884,9 @@ typedef struct DetectEngineCtx_ {
     /* maximum recursion depth for content inspection */
     int inspection_recursion_limit;
 
+    /* maximum number of times a tx will get logged for a stream-only rule match */
+    uint8_t stream_tx_log_limit;
+
     /* registration id for per thread ctx for the filemagic/file.magic keywords */
     int filemagic_thread_ctx_id;
 
diff --git a/suricata.yaml.in b/suricata.yaml.in
index 2e5f722c36..3e73623c4b 100644
--- a/suricata.yaml.in
+++ b/suricata.yaml.in
@@ -1686,6 +1686,8 @@ detect:
     toserver-groups: 25
   sgh-mpm-context: auto
   inspection-recursion-limit: 3000
+  # maximum number of times a tx will get logged for a stream-only rule match
+  # stream-tx-log-limit: 4
   # 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