rate_filter: by_rule fixed triggering algorithm

Fixes issue #2258

Program was triggering rate_filter by_rule earlier than needed
and generally behaved like a threshold.

diff --git a/src/detect-engine-threshold.c b/src/detect-engine-threshold.c
index efd95138977e917ce4ff5ade59728cc0322877c2..81613f3d33c4a33ec41365b4e3b8b221d3c6644c 100644 (file)
--- a/src/detect-engine-threshold.c
+++ b/src/detect-engine-threshold.c
@@ -227,13 +227,13 @@ DetectThresholdEntryAlloc(const DetectThresholdData *td, Packet *p,
     if (unlikely(ste == NULL)) {
         SCReturnPtr(NULL, "DetectThresholdEntry");
     }
+    memset(ste, 0, sizeof(*ste));
 
     ste->sid = sid;
     ste->gid = gid;
 
     ste->track = td->track;
     ste->seconds = td->seconds;
-    ste->tv_timeout = 0;
 
     SCReturnPtr(ste, "DetectThresholdEntry");
 }
@@ -614,50 +614,32 @@ static int ThresholdHandlePacketRule(DetectEngineCtx *de_ctx, Packet *p,
 {
     int ret = 0;
 
-    if (td->type != TYPE_RATE)
-        return 1;
-
     DetectThresholdEntry* lookup_tsh = (DetectThresholdEntry *)de_ctx->ths_ctx.th_entry[s->num];
-    if (lookup_tsh != NULL) {
-        /* Check if we have a timeout enabled, if so,
-         * we still matching (and enabling the new_action) */
-        if ( (p->ts.tv_sec - lookup_tsh->tv_timeout) > td->timeout) {
-            /* Ok, we are done, timeout reached */
-            lookup_tsh->tv_timeout = 0;
-        } else {
-            /* Already matching */
-            /* Take the action to perform */
-            RateFilterSetAction(p, pa, td->new_action);
-            ret = 1;
-        }
+    SCLogDebug("by_rule lookup_tsh %p num %u", lookup_tsh, s->num);
 
-        /* Update the matching state with the timeout interval */
-        if ( (p->ts.tv_sec - lookup_tsh->tv_sec1) < td->seconds) {
-            lookup_tsh->current_count++;
-            if (lookup_tsh->current_count >= td->count) {
-                /* Then we must enable the new action by setting a
-                 * timeout */
-                lookup_tsh->tv_timeout = p->ts.tv_sec;
-                /* Take the action to perform */
+    switch (td->type) {
+        case TYPE_RATE:
+        {
+            ret = 1;
+            if (lookup_tsh && IsThresholdReached(lookup_tsh, td, p->ts.tv_sec)) {
                 RateFilterSetAction(p, pa, td->new_action);
-                ret = 1;
             }
-        } else {
-            lookup_tsh->tv_sec1 = p->ts.tv_sec;
-            lookup_tsh->current_count = 1;
-        }
-    } else {
-        if (td->count == 1) {
-            ret = 1;
-        }
-
-        DetectThresholdEntry *e = DetectThresholdEntryAlloc(td, p, s->id, s->gid);
-        if (e != NULL) {
-            e->current_count = 1;
-            e->tv_sec1 = p->ts.tv_sec;
-            e->tv_timeout = 0;
+            else if (!lookup_tsh) {
+                DetectThresholdEntry *e = DetectThresholdEntryAlloc(td, p, s->id, s->gid);
+                if (e != NULL) {
+                    e->current_count = 1;
+                    e->tv_sec1 = p->ts.tv_sec;
+                    e->tv_timeout = 0;
 
-            de_ctx->ths_ctx.th_entry[s->num] = e;
+                    de_ctx->ths_ctx.th_entry[s->num] = e;
+                }
+            }
+            break;
+        }
+        default:
+        {
+            SCLogError(SC_ERR_INVALID_VALUE, "type %d is not supported", td->type);
+            break;
         }
     }
 

diff --git a/src/util-threshold-config.c b/src/util-threshold-config.c
index 370a9e476de18e69dbb431a9f1d554b71452cfba..d8ad7c70c74fa77a1d1a334ad1b4f883d0c27a72 100644 (file)
--- a/src/util-threshold-config.c
+++ b/src/util-threshold-config.c
@@ -1276,8 +1276,7 @@ static FILE *SCThresholdConfGenerateValidDummyFD07(void)
 }
 
 /**
- * \brief Creates a dummy threshold file, with all valid options, but
- *        with splitted rules (multiline), for testing purposes.
+ * \brief Creates a dummy threshold file, for testing rate_filter, track by_rule
  *
  * \retval fd Pointer to file descriptor.
  */
@@ -1285,8 +1284,7 @@ static FILE *SCThresholdConfGenerateValidDummyFD08(void)
 {
     FILE *fd = NULL;
     const char *buffer =
-        "rate_filter gen_id 1, sig_id 10, track by_rule, count 3, seconds 3, new_action drop, timeout 10\n"
-        "rate_filter gen_id 1, sig_id 11, track by_src, count 3, seconds 1, new_action drop, timeout 5\n";
+        "rate_filter gen_id 1, sig_id 10, track by_rule, count 3, seconds 3, new_action drop, timeout 10\n";
 
     fd = SCFmemopen((void *)buffer, strlen(buffer), "r");
     if (fd == NULL)
@@ -1741,7 +1739,11 @@ static int SCThresholdConfTest10(void)
     memset (&ts, 0, sizeof(struct timeval));
     TimeGet(&ts);
 
-    Packet *p1 = UTHBuildPacket((uint8_t*)"lalala", 6, IPPROTO_TCP);
+    /* Create two different packets falling to the same rule, and
+    *  because count:3, we should drop on match #4.
+    */
+    Packet *p1 = UTHBuildPacketSrcDst((uint8_t*)"lalala", 6, IPPROTO_TCP,
+            "172.26.0.2", "172.26.0.11");
     FAIL_IF_NULL(p1);
     Packet *p2 = UTHBuildPacketSrcDst((uint8_t*)"lalala", 6, IPPROTO_TCP,
             "172.26.0.1", "172.26.0.10");
@@ -1767,26 +1769,44 @@ static int SCThresholdConfTest10(void)
     SigGroupBuild(de_ctx);
     DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx);
     TimeGet(&p1->ts);
+    p2->ts = p1->ts;
 
+    /* All should be alerted, none dropped */
     SigMatchSignatures(&th_v, de_ctx, det_ctx, p1);
-    FAIL_IF(PacketAlertCheck(p1, 10));
+    FAIL_IF(PACKET_TEST_ACTION(p1, ACTION_DROP));
+    FAIL_IF(PacketAlertCheck(p1, 10) != 1);
+    p1->action = 0;
     SigMatchSignatures(&th_v, de_ctx, det_ctx, p2);
-    FAIL_IF(PacketAlertCheck(p2, 10));
-
+    FAIL_IF(PACKET_TEST_ACTION(p2, ACTION_DROP));
+    FAIL_IF(PacketAlertCheck(p2, 10) != 1);
+    p2->action = 0;
     SigMatchSignatures(&th_v, de_ctx, det_ctx, p1);
-    FAIL_IF_NOT(PacketAlertCheck(p1, 10) == 1);
+    FAIL_IF(PACKET_TEST_ACTION(p1, ACTION_DROP));
+    FAIL_IF(PacketAlertCheck(p1, 10) != 1);
+    p1->action = 0;
+
+    /* Match #4 should be dropped*/
+    SigMatchSignatures(&th_v, de_ctx, det_ctx, p2);
+    FAIL_IF_NOT(PACKET_TEST_ACTION(p2, ACTION_DROP));
+    FAIL_IF(PacketAlertCheck(p2, 10) != 1);
+    p2->action = 0;
 
     TimeSetIncrementTime(2);
     TimeGet(&p1->ts);
 
-    SigMatchSignatures(&th_v, de_ctx, det_ctx, p2);
-    FAIL_IF_NOT(PacketAlertCheck(p2, 10) == 1);
+    /* Still dropped because timeout not expired */
+    SigMatchSignatures(&th_v, de_ctx, det_ctx, p1);
+    FAIL_IF_NOT(PACKET_TEST_ACTION(p1, ACTION_DROP));
+    FAIL_IF(PacketAlertCheck(p1, 10) != 1);
+    p1->action = 0;
 
     TimeSetIncrementTime(10);
     TimeGet(&p1->ts);
 
+    /* Not dropped because timeout expired */
     SigMatchSignatures(&th_v, de_ctx, det_ctx, p1);
-    FAIL_IF_NOT(PacketAlertCheck(p1, 10) == 0);
+    FAIL_IF(PACKET_TEST_ACTION(p1, ACTION_DROP));
+    FAIL_IF(PacketAlertCheck(p1, 10) != 1);
 
     /* Ensure that a Threshold entry was installed at the sig */
     FAIL_IF_NULL(de_ctx->ths_ctx.th_entry[s->num]);