From: Juliana Fajardini <jufajardini@gmail.com>
Date: Thu, 17 Jun 2021 16:18:09 +0000 (+0100)
Subject: detect: fix bug where rule without sid is accepted
X-Git-Tag: suricata-7.0.0-beta1~1566
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a15fada7271d125ed3b4d9f29d72ff10ff652ff5;p=thirdparty%2Fsuricata.git

detect: fix bug where rule without sid is accepted

Before, if Suricata parsed a rule without a 'sid' option, instead of
failing that rule, the rule was parsed and attributed a sid 0.
Changes to:
detect-parse:
- add logic to filter out rules without sid;
- change unittest which didn't have a sid, but used to pass.
detect-sid: add unittest for rules without sid or with sid: 0
---

diff --git a/src/detect-parse.c b/src/detect-parse.c
index 8c1bd2a87d..13d5480616 100644
--- a/src/detect-parse.c
+++ b/src/detect-parse.c
@@ -1889,6 +1889,10 @@ static int SigValidate(DetectEngineCtx *de_ctx, Signature *s)
             SCReturnInt(0);
         }
     }
+    if (s->id == 0) {
+        SCLogError(SC_ERR_INVALID_SIGNATURE, "Signature missing required value \"sid\".");
+        SCReturnInt(0);
+    }
     SCReturnInt(1);
 }
 
@@ -2807,13 +2811,15 @@ static int SigParseTest11(void)
 
     Signature *s = NULL;
 
-    s = DetectEngineAppendSig(de_ctx, "drop tcp any any -> any 80 (msg:\"Snort_Inline is blocking the http link\";) ");
+    s = DetectEngineAppendSig(de_ctx,
+            "drop tcp any any -> any 80 (msg:\"Snort_Inline is blocking the http link\"; sid:1;) ");
     if (s == NULL) {
         printf("sig 1 didn't parse: ");
         goto end;
     }
 
-    s = DetectEngineAppendSig(de_ctx, "drop tcp any any -> any 80 (msg:\"Snort_Inline is blocking the http link\"; sid:1;)            ");
+    s = DetectEngineAppendSig(de_ctx, "drop tcp any any -> any 80 (msg:\"Snort_Inline is blocking "
+                                      "the http link\"; sid:2;)            ");
     if (s == NULL) {
         printf("sig 2 didn't parse: ");
         goto end;
diff --git a/src/detect-sid.c b/src/detect-sid.c
index 979a663ccf..d598908ecf 100644
--- a/src/detect-sid.c
+++ b/src/detect-sid.c
@@ -119,6 +119,22 @@ static int SidTestParse03(void)
     PASS;
 }
 
+static int SidTestParse04(void)
+{
+    DetectEngineCtx *de_ctx = DetectEngineCtxInit();
+    FAIL_IF_NULL(de_ctx);
+
+    FAIL_IF_NOT_NULL(DetectEngineAppendSig(
+            de_ctx, "alert tcp any any -> any any (content:\"ABC\"; sid: 0;)"));
+
+    /* Let's also make sure that Suricata fails a rule which doesn't have a sid at all */
+    FAIL_IF_NOT_NULL(
+            DetectEngineAppendSig(de_ctx, "alert tcp any any -> any any (content:\"ABC\";)"));
+
+    DetectEngineCtxFree(de_ctx);
+    PASS;
+}
+
 /**
  * \brief Register DetectSid unit tests.
  */
@@ -127,5 +143,6 @@ static void DetectSidRegisterTests(void)
     UtRegisterTest("SidTestParse01", SidTestParse01);
     UtRegisterTest("SidTestParse02", SidTestParse02);
     UtRegisterTest("SidTestParse03", SidTestParse03);
+    UtRegisterTest("SidTestParse04", SidTestParse04);
 }
 #endif /* UNITTESTS */
\ No newline at end of file