thresholds: Fix buffer overflow in threshold context

th_entry is resized using ThresholdHashRealloc() every time a rule with
a threshold using by_rule tracking is added. The problem is that this is
done before the rules are reordered, so occasionally a rule with by_rule
tracking gets a higher signature number (after reordering) than the
number of th_entries allocated, causing Suricata to crash.

This commit fixes this by allocating th_entries after all the rules are
loaded and reordered.

Backtrace from core dump:

  Program terminated with signal SIGSEGV, Segmentation fault.

  #0  0x000000000051b381 in ThresholdHandlePacket (p=p@entry=0x7fb0080f3960, lookup_tsh=0x51, new_tsh=new_tsh@entry=0x7fb016c316e0, td=td@entry=0x14adedf0, sid=9800979, gid=1, pa=0x7fb0080f3b18)
      at detect-engine-threshold.c:415
  415>----                if (TIMEVAL_DIFF_SEC(p->ts, lookup_tsh->tv1) < td->seconds) {

Bug #4503.

(cherry picked from commit 2a326421aa29154ebfaada3888974a634feb5f56)

diff --git a/src/detect-engine-build.c b/src/detect-engine-build.c
index 784337b0997d5b6a8149bbe467c4f51b821d74c4..ecd21f868429275ae60e1ceb4e0ef9918cd91f76 100644 (file)
--- a/src/detect-engine-build.c
+++ b/src/detect-engine-build.c
@@ -28,6 +28,7 @@
 #include "detect-engine-port.h"
 #include "detect-engine-prefilter.h"
 #include "detect-engine-proto.h"
+#include "detect-engine-threshold.h"
 
 #include "detect-dsize.h"
 #include "detect-tcp-flags.h"
@@ -1944,6 +1945,8 @@ int SigGroupBuild(DetectEngineCtx *de_ctx)
     SCProfilingRuleInitCounters(de_ctx);
 #endif
 
+    ThresholdHashAllocate(de_ctx);
+
     if (!DetectEngineMultiTenantEnabled()) {
         VarNameStoreActivateStaging();
     }

diff --git a/src/detect-engine-threshold.c b/src/detect-engine-threshold.c
index d98931de95a3fe3ccc475df72d978a4713dc1eac..355e1dc35dcb37045ae9b59ec0f7406865ba5f23 100644 (file)
--- a/src/detect-engine-threshold.c
+++ b/src/detect-engine-threshold.c
@@ -651,27 +651,71 @@ void ThresholdHashInit(DetectEngineCtx *de_ctx)
 }
 
 /**
- * \brief Realloc threshold context hash tables
+ * \brief Allocate threshold context hash tables
  *
  * \param de_ctx Detection Context
  */
-void ThresholdHashRealloc(DetectEngineCtx *de_ctx)
+void ThresholdHashAllocate(DetectEngineCtx *de_ctx)
 {
-    /* Return if we are already big enough */
-    uint32_t num = de_ctx->signum + 1;
-    if (num <= de_ctx->ths_ctx.th_size)
-        return;
+    Signature *s = de_ctx->sig_list;
+    bool has_by_rule_tracking = false;
+    const DetectThresholdData *td = NULL;
+    const SigMatchData *smd;
+
+    /* Find the signature with the highest signature number that is using
+       thresholding with by_rule tracking. */
+    uint32_t highest_signum = 0;
+    while (s != NULL) {
+        if (s->sm_arrays[DETECT_SM_LIST_SUPPRESS] != NULL) {
+            smd = NULL;
+            do {
+                td = SigGetThresholdTypeIter(s, &smd, DETECT_SM_LIST_SUPPRESS);
+                if (td == NULL) {
+                    continue;
+                }
+                if (td->track != TRACK_RULE) {
+                    continue;
+                }
+                if (s->num >= highest_signum) {
+                    highest_signum = s->num;
+                    has_by_rule_tracking = true;
+                }
+            } while (smd != NULL);
+        }
 
-    void *ptmp = SCRealloc(de_ctx->ths_ctx.th_entry, num * sizeof(DetectThresholdEntry *));
-    if (ptmp == NULL) {
-        SCLogWarning(SC_ERR_MEM_ALLOC, "Error allocating memory for rule thresholds"
-                " (tried to allocate %"PRIu32" th_entrys for rule tracking)", num);
-    } else {
-        de_ctx->ths_ctx.th_entry = ptmp;
-        for (uint32_t i = de_ctx->ths_ctx.th_size; i < num; ++i) {
-            de_ctx->ths_ctx.th_entry[i] = NULL;
+        if (s->sm_arrays[DETECT_SM_LIST_THRESHOLD] != NULL) {
+            smd = NULL;
+            do {
+                td = SigGetThresholdTypeIter(s, &smd, DETECT_SM_LIST_THRESHOLD);
+                if (td == NULL) {
+                    continue;
+                }
+                if (td->track != TRACK_RULE) {
+                    continue;
+                }
+                if (s->num >= highest_signum) {
+                    highest_signum = s->num;
+                    has_by_rule_tracking = true;
+                }
+            } while (smd != NULL);
         }
-        de_ctx->ths_ctx.th_size = num;
+
+        s = s->next;
+    }
+
+    /* Skip allocating if by_rule tracking is not used */
+    if (has_by_rule_tracking == false) {
+        return;
+    }
+
+    de_ctx->ths_ctx.th_size = highest_signum + 1;
+    de_ctx->ths_ctx.th_entry = SCCalloc(de_ctx->ths_ctx.th_size, sizeof(DetectThresholdEntry *));
+    if (de_ctx->ths_ctx.th_entry == NULL) {
+        FatalError(SC_ERR_MEM_ALLOC,
+                "Error allocating memory for rule "
+                "thresholds (tried to allocate %" PRIu32 " th_entrys for "
+                "rule tracking)",
+                de_ctx->ths_ctx.th_size);
     }
 }
 

diff --git a/src/detect-engine-threshold.h b/src/detect-engine-threshold.h
index e3115b873f11e760bc89b63e6d21676609bb643e..d94d8ec6ad01a0b43b4542ca53c0bd6df879aa20 100644 (file)
--- a/src/detect-engine-threshold.h
+++ b/src/detect-engine-threshold.h
@@ -43,7 +43,7 @@ int PacketAlertThreshold(DetectEngineCtx *, DetectEngineThreadCtx *,
         const Signature *, PacketAlert *);
 
 void ThresholdHashInit(DetectEngineCtx *);
-void ThresholdHashRealloc(DetectEngineCtx *);
+void ThresholdHashAllocate(DetectEngineCtx *);
 void ThresholdContextDestroy(DetectEngineCtx *);
 
 int ThresholdHostTimeoutCheck(Host *, struct timeval *);

diff --git a/src/detect-threshold.c b/src/detect-threshold.c
index 3abe8d263cef4e15c2593704697de14571bcd2a5..baf8aea550b1b2d25daffe169d2c00e79a1971af 100644 (file)
--- a/src/detect-threshold.c
+++ b/src/detect-threshold.c
@@ -251,9 +251,6 @@ static int DetectThresholdSetup(DetectEngineCtx *de_ctx, Signature *s, const cha
     if (de == NULL)
         goto error;
 
-    if (de->track == TRACK_RULE)
-        ThresholdHashRealloc(de_ctx);
-
     sm = SigMatchAlloc();
     if (sm == NULL)
         goto error;
@@ -1604,7 +1601,6 @@ static int DetectThresholdTestSig13(void)
 
     SigGroupBuild(de_ctx);
     DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx);
-    ThresholdHashRealloc(de_ctx);
 
     /* should alert twice */
     SigMatchSignatures(&th_v, de_ctx, det_ctx, p);

diff --git a/src/util-threshold-config.c b/src/util-threshold-config.c
index 9d4bfb177e3eb6faa05102906d481a5200c2b489..104ccd663aa6472f20f0df1f6c8c64a5ade190dd 100644 (file)
--- a/src/util-threshold-config.c
+++ b/src/util-threshold-config.c
@@ -487,9 +487,6 @@ static int SetupThresholdRule(DetectEngineCtx *de_ctx, uint32_t id, uint32_t gid
                 sm->type = DETECT_THRESHOLD;
             sm->ctx = (void *)de;
 
-            if (parsed_track == TRACK_RULE) {
-                ThresholdHashRealloc(de_ctx);
-            }
             SigMatchAppendSMToList(s, sm, DETECT_SM_LIST_THRESHOLD);
         }
 
@@ -530,9 +527,6 @@ static int SetupThresholdRule(DetectEngineCtx *de_ctx, uint32_t id, uint32_t gid
                     sm->type = DETECT_THRESHOLD;
                 sm->ctx = (void *)de;
 
-                if (parsed_track == TRACK_RULE) {
-                    ThresholdHashRealloc(de_ctx);
-                }
                 SigMatchAppendSMToList(s, sm, DETECT_SM_LIST_THRESHOLD);
             }
         }
@@ -604,10 +598,6 @@ static int SetupThresholdRule(DetectEngineCtx *de_ctx, uint32_t id, uint32_t gid
                 sm->type = DETECT_THRESHOLD;
             sm->ctx = (void *)de;
 
-            if (parsed_track == TRACK_RULE) {
-                ThresholdHashRealloc(de_ctx);
-            }
-
             SigMatchAppendSMToList(s, sm, DETECT_SM_LIST_THRESHOLD);
         }
     }