From 10ea60a237cf41ddd10f7a887e2824b4b8e1c419 Mon Sep 17 00:00:00 2001
From: Gianni Tedesco <gianni@scaramanga.co.uk>
Date: Sun, 13 Dec 2020 23:54:13 +0900
Subject: [PATCH] detect: Validate that NOOPT options don't have optvals

Without this, a simple typo between : and ; is able to hide actual bugs
in rules.

I discovered 2 bugs in ET open ruleset this way.
---
 src/detect-parse.c       | 10 ++++++++--
 src/tests/detect-parse.c | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/src/detect-parse.c b/src/detect-parse.c
index 3f33f360e8..59517b5591 100644
--- a/src/detect-parse.c
+++ b/src/detect-parse.c
@@ -704,8 +704,14 @@ static int SigParseOptions(DetectEngineCtx *de_ctx, Signature *s, char *optstr,
 
     if (!(st->flags & (SIGMATCH_NOOPT|SIGMATCH_OPTIONAL_OPT))) {
         if (optvalue == NULL || strlen(optvalue) == 0) {
-            SCLogError(SC_ERR_INVALID_SIGNATURE, "invalid formatting or malformed option to %s keyword: \'%s\'",
-                    optname, optstr);
+            SCLogError(SC_ERR_INVALID_SIGNATURE,
+                    "invalid formatting or malformed option to %s keyword: '%s'", optname, optstr);
+            goto error;
+        }
+    } else if (st->flags & SIGMATCH_NOOPT) {
+        if (optvalue && strlen(optvalue)) {
+            SCLogError(SC_ERR_INVALID_SIGNATURE, "unexpected option to %s keyword: '%s'", optname,
+                    optstr);
             goto error;
         }
     }
diff --git a/src/tests/detect-parse.c b/src/tests/detect-parse.c
index b7fb452b38..d807c838f8 100644
--- a/src/tests/detect-parse.c
+++ b/src/tests/detect-parse.c
@@ -38,6 +38,23 @@ static int DetectParseTest01 (void)
     PASS;
 }
 
+/**
+ * \test DetectParseTestNoOpt  is a regression test to make sure that we reject
+ * any signature where a NOOPT rule option is given a value. This can hide rule
+ * errors which make other options disappear, eg: foo: bar: baz; where "foo" is
+ * the NOOPT option, we will end up with a signature which is missing "bar".
+ */
+
+static int DetectParseTestNoOpt(void)
+{
+    DetectEngineCtx *de_ctx = DetectEngineCtxInit();
+    FAIL_IF(DetectEngineAppendSig(de_ctx,
+                    "alert http any any -> any any (msg:\"sid 1 version 0\"; "
+                    "content:\"dummy1\"; endswith: reference: ref; sid:1;)") != NULL);
+    DetectEngineCtxFree(de_ctx);
+
+    PASS;
+}
 
 /**
  * \brief this function registers unit tests for DetectParse
@@ -45,4 +62,5 @@ static int DetectParseTest01 (void)
 void DetectParseRegisterTests(void)
 {
     UtRegisterTest("DetectParseTest01", DetectParseTest01);
+    UtRegisterTest("DetectParseTestNoOpt", DetectParseTestNoOpt);
 }