From: Victor Julien <victor@inliniac.net>
Date: Tue, 28 Sep 2021 10:28:54 +0000 (+0200)
Subject: detect: fix FNs in case of too many prefilter engines
X-Git-Tag: suricata-5.0.8~27
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1a4288ff2be7f1d461de7153b7ee014bcaca3a29;p=thirdparty%2Fsuricata.git

detect: fix FNs in case of too many prefilter engines

Fix FNs in case of too many prefilter engines. A transaction was tracking
which engines have run using a u64 bit array. The engines 'local_id' was
used to set and check this bit. However the bit checking code didn't
handle int types correctly, leading to an incorrect left shift result of
a u32 to a u64 bit value.

This commit addresses that by fixing the int handling.

This is only a partial fix however. It's not hard to craft a ruleset that
exceeds the 63-bit space available. A more complete fix is in:

932cf0b6a6ad ("detect: track prefilter by progress, not engine")

However this seems like a too high risk change for a backport into
5.0.

This patch does issue a warning if the condition is detected at start
up, and `-T` does error out on it.

Bug: #4688.
---

diff --git a/src/detect-engine-prefilter.c b/src/detect-engine-prefilter.c
index f0f3e68b7b..98477243ec 100644
--- a/src/detect-engine-prefilter.c
+++ b/src/detect-engine-prefilter.c
@@ -108,7 +108,7 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx,
         if (engine->tx_min_progress > tx->tx_progress)
             goto next;
         if (tx->tx_progress > engine->tx_min_progress) {
-            if (tx->prefilter_flags & (1<<(engine->local_id))) {
+            if (tx->prefilter_flags & BIT_U64(engine->local_id)) {
                 goto next;
             }
         }
@@ -119,7 +119,7 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx,
         PREFILTER_PROFILING_END(det_ctx, engine->gid);
 
         if (tx->tx_progress > engine->tx_min_progress) {
-            tx->prefilter_flags |= (1<<(engine->local_id));
+            tx->prefilter_flags |= BIT_U64(engine->local_id);
         }
     next:
         if (engine->is_last)
@@ -447,6 +447,17 @@ void PrefilterSetupRuleGroup(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
             e++;
         }
         SCLogDebug("sgh %p max local_id %u", sgh, local_id);
+
+        /* max assigned id can be 62 (63 is reserved) */
+        if (local_id >= 63) {
+            if (de_ctx->failure_fatal) {
+                FatalError(SC_ERR_DETECT_PREPARE, "max number of prefilter engines exceeded (%u >= 62). "
+                        "Risk of False Negatives. See ticket #4688.", local_id - 1);
+            } else {
+                SCLogWarning(SC_ERR_DETECT_PREPARE, "max number of prefilter engines exceeded (%u >= 62). "
+                        "Risk of False Negatives. See ticket #4688.", local_id - 1);
+            }
+        }
     }
 }