detect/flowbits: use strtok_r for parsing

Fixes underlying parsing issues by keeping stricter argument checks.

Redmine Bug: 5154

diff --git a/src/detect-flowbits.c b/src/detect-flowbits.c
index 4753ee9d43d1e3cfb7ea19bacfcd4b955837c6a0..d8680220a65bfa737ee3936fbb25cb4e23a7b313 100644 (file)
--- a/src/detect-flowbits.c
+++ b/src/detect-flowbits.c
@@ -212,118 +212,132 @@ int DetectFlowbitMatch (DetectEngineThreadCtx *det_ctx, Packet *p,
     return 0;
 }
 
-static int DetectFlowbitParse(const char *str, char *cmd, int cmd_len, char *name,
-    int name_len)
+static int DetectFlowbitParse(
+        DetectEngineCtx *de_ctx, const char *rawstr, DetectFlowbitsData **cdout)
 {
-    const int max_substrings = 30;
-    int count, rc;
-    int ov[max_substrings];
-
-    count = DetectParsePcreExec(&parse_regex, str, 0, 0, ov, max_substrings);
-    if (count != 2 && count != 3) {
-        SCLogError(SC_ERR_PCRE_MATCH,
-            "\"%s\" is not a valid setting for flowbits.", str);
-        return 0;
-    }
-
-    rc = pcre_copy_substring((char *)str, ov, max_substrings, 1, cmd, cmd_len);
-    if (rc < 0) {
-        SCLogError(SC_ERR_PCRE_GET_SUBSTRING, "pcre_copy_substring failed");
-        return 0;
-    }
-
-    if (count == 3) {
-        rc = pcre_copy_substring((char *)str, ov, max_substrings, 2, name,
-            name_len);
-        if (rc < 0) {
-            SCLogError(SC_ERR_PCRE_GET_SUBSTRING, "pcre_copy_substring failed");
-            return 0;
+    bool cmd_set = false;
+    bool name_set = false;
+    uint8_t cmd = 0;
+    char name[256] = "";
+    char *val = NULL;
+    DetectFlowbitsData *cd = NULL;
+    char copy[strlen(rawstr) + 1];
+    strlcpy(copy, rawstr, sizeof(copy));
+    char *context = NULL;
+    char *token = strtok_r(copy, ",", &context);
+    while (token != NULL) {
+        while (*token != '\0' && isblank(*token)) {
+            token++;
         }
-
-        /* Trim trailing whitespace. */
-        while (strlen(name) > 0 && isblank(name[strlen(name) - 1])) {
-            name[strlen(name) - 1] = '\0';
+        if (strchr(token, '|') == NULL) {
+            val = strchr(token, ' ');
+            if (val != NULL) {
+                *val++ = '\0';
+                while (*val != '\0' && isblank(*val)) {
+                    val++;
+                }
+            } else {
+                SCLogDebug("val %s", token);
+            }
         }
+        if (strlen(token) == 0) {
+            return -1;
+        }
+        if (strcmp(token, "noalert") == 0 && !cmd_set) {
+            if (strtok_r(NULL, ",", &context) != NULL) {
+                return -1;
+            }
+            if (val && strlen(val) != 0) {
+                return -1;
+            }
+            *cdout = NULL;
+            return 0;
+        }
+        if (!cmd_set) {
+            if (val && strlen(val) != 0) {
+                return -1;
+            }
+            if (strcmp(token, "set") == 0) {
+                cmd = DETECT_FLOWBITS_CMD_SET;
+            } else if (strcmp(token, "isset") == 0) {
+                cmd = DETECT_FLOWBITS_CMD_ISSET;
+            } else if (strcmp(token, "unset") == 0) {
+                cmd = DETECT_FLOWBITS_CMD_UNSET;
+            } else if (strcmp(token, "isnotset") == 0) {
+                cmd = DETECT_FLOWBITS_CMD_ISNOTSET;
+            } else if (strcmp(token, "toggle") == 0) {
+                cmd = DETECT_FLOWBITS_CMD_TOGGLE;
+            } else {
+                SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid flowbits cmd: %s", token);
+                return -1;
+            }
+            cmd_set = true;
+        } else if (!name_set) {
+            if (val && strlen(val) != 0) {
+                return -1;
+            }
 
-        if (strchr(name, '|') == NULL) {
-            /* Validate name, spaces are not allowed. */
-            for (size_t i = 0; i < strlen(name); i++) {
-                if (isblank(name[i])) {
-                    SCLogError(SC_ERR_INVALID_SIGNATURE,
-                        "spaces not allowed in flowbit names");
-                    return 0;
+            if (strchr(token, '|') == NULL) {
+                /* Validate name, spaces are not allowed. */
+                for (size_t i = 0; i < strlen(token); i++) {
+                    if (isblank(token[i])) {
+                        SCLogError(SC_ERR_INVALID_SIGNATURE, "spaces not allowed in flowbit names");
+                        return 0;
+                    }
                 }
             }
+            strlcpy(name, token, sizeof(name));
+            name_set = true;
+        } else {
+            SCLogError(SC_ERR_INVALID_SIGNATURE, "Invalid flowbits keyword: %s", token);
+            return -1;
         }
+        token = strtok_r(NULL, ",", &context);
     }
-
-    return 1;
-}
-
-int DetectFlowbitSetup (DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
-{
-    DetectFlowbitsData *cd = NULL;
-    SigMatch *sm = NULL;
-    uint8_t fb_cmd = 0;
-    char fb_cmd_str[16] = "", fb_name[256] = "";
-
-    if (!DetectFlowbitParse(rawstr, fb_cmd_str, sizeof(fb_cmd_str), fb_name,
-            sizeof(fb_name))) {
+    if (cmd_set && !name_set) {
         return -1;
     }
 
-    if (strcmp(fb_cmd_str,"noalert") == 0) {
-        fb_cmd = DETECT_FLOWBITS_CMD_NOALERT;
-    } else if (strcmp(fb_cmd_str,"isset") == 0) {
-        fb_cmd = DETECT_FLOWBITS_CMD_ISSET;
-    } else if (strcmp(fb_cmd_str,"isnotset") == 0) {
-        fb_cmd = DETECT_FLOWBITS_CMD_ISNOTSET;
-    } else if (strcmp(fb_cmd_str,"set") == 0) {
-        fb_cmd = DETECT_FLOWBITS_CMD_SET;
-    } else if (strcmp(fb_cmd_str,"unset") == 0) {
-        fb_cmd = DETECT_FLOWBITS_CMD_UNSET;
-    } else if (strcmp(fb_cmd_str,"toggle") == 0) {
-        fb_cmd = DETECT_FLOWBITS_CMD_TOGGLE;
-    } else {
-        SCLogError(SC_ERR_UNKNOWN_VALUE, "ERROR: flowbits action \"%s\" is not supported.", fb_cmd_str);
-        goto error;
-    }
-
-    switch (fb_cmd) {
-        case DETECT_FLOWBITS_CMD_NOALERT:
-            if (strlen(fb_name) != 0)
-                goto error;
-            s->flags |= SIG_FLAG_NOALERT;
-            return 0;
-        case DETECT_FLOWBITS_CMD_ISNOTSET:
-        case DETECT_FLOWBITS_CMD_ISSET:
-        case DETECT_FLOWBITS_CMD_SET:
-        case DETECT_FLOWBITS_CMD_UNSET:
-        case DETECT_FLOWBITS_CMD_TOGGLE:
-        default:
-            if (strlen(fb_name) == 0)
-                goto error;
-            break;
-    }
-
     cd = SCCalloc(1, sizeof(DetectFlowbitsData));
     if (unlikely(cd == NULL))
         goto error;
-    if (strchr(fb_name, '|') != NULL) {
-        int retval = FlowbitOrAddData(de_ctx, cd, fb_name);
+    if (strchr(name, '|') != NULL) {
+        int retval = FlowbitOrAddData(de_ctx, cd, name);
         if (retval == -1) {
             goto error;
         }
-        cd->cmd = fb_cmd;
+        cd->cmd = cmd;
     } else {
-        cd->idx = VarNameStoreSetupAdd(fb_name, VAR_TYPE_FLOW_BIT);
+        cd->idx = VarNameStoreSetupAdd(name, VAR_TYPE_FLOW_BIT);
         de_ctx->max_fb_id = MAX(cd->idx, de_ctx->max_fb_id);
-        cd->cmd = fb_cmd;
+        cd->cmd = cmd;
         cd->or_list_size = 0;
         cd->or_list = NULL;
-        SCLogDebug("idx %" PRIu32 ", cmd %s, name %s",
-            cd->idx, fb_cmd_str, strlen(fb_name) ? fb_name : "(none)");
+        SCLogDebug(
+                "idx %" PRIu32 ", cmd %d, name %s", cd->idx, cmd, strlen(name) ? name : "(none)");
+    }
+    *cdout = cd;
+
+    return 0;
+error:
+    if (cd != NULL)
+        DetectFlowbitFree(de_ctx, cd);
+    return -1;
+}
+
+int DetectFlowbitSetup(DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
+{
+    DetectFlowbitsData *cd = NULL;
+    SigMatch *sm = NULL;
+
+    int result = DetectFlowbitParse(de_ctx, rawstr, &cd);
+    if (result < 0) {
+        return -1;
+    } else if (result == 0 && cd == NULL) {
+        s->flags |= SIG_FLAG_NOALERT;
+        return 0;
     }
+
     /* Okay so far so good, lets get this into a SigMatch
      * and put it in the Signature. */
     sm = SigMatchAlloc();
@@ -333,8 +347,8 @@ int DetectFlowbitSetup (DetectEngineCtx *de_ctx, Signature *s, const char *rawst
     sm->type = DETECT_FLOWBITS;
     sm->ctx = (SigMatchCtx *)cd;
 
-    switch (fb_cmd) {
-        /* case DETECT_FLOWBITS_CMD_NOALERT can't happen here */
+    switch (cd->cmd) {
+            /* case DETECT_FLOWBITS_CMD_NOALERT can't happen here */
 
         case DETECT_FLOWBITS_CMD_ISNOTSET:
         case DETECT_FLOWBITS_CMD_ISSET:
@@ -744,41 +758,40 @@ static void DetectFlowbitsAnalyzeDump(const DetectEngineCtx *de_ctx,
 
 static int FlowBitsTestParse01(void)
 {
-    char command[16] = "", name[16] = "";
-
-    /* Single argument version. */
-    FAIL_IF(!DetectFlowbitParse("noalert", command, sizeof(command), name,
-            sizeof(name)));
-    FAIL_IF(strcmp(command, "noalert") != 0);
-
-    /* No leading or trailing spaces. */
-    FAIL_IF(!DetectFlowbitParse("set,flowbit", command, sizeof(command), name,
-            sizeof(name)));
-    FAIL_IF(strcmp(command, "set") != 0);
-    FAIL_IF(strcmp(name, "flowbit") != 0);
-
-    /* Leading space. */
-    FAIL_IF(!DetectFlowbitParse("set, flowbit", command, sizeof(command), name,
-            sizeof(name)));
-    FAIL_IF(strcmp(command, "set") != 0);
-    FAIL_IF(strcmp(name, "flowbit") != 0);
-
-    /* Trailing space. */
-    FAIL_IF(!DetectFlowbitParse("set,flowbit ", command, sizeof(command), name,
-            sizeof(name)));
-    FAIL_IF(strcmp(command, "set") != 0);
-    FAIL_IF(strcmp(name, "flowbit") != 0);
-
-    /* Leading and trailing space. */
-    FAIL_IF(!DetectFlowbitParse("set, flowbit ", command, sizeof(command), name,
-            sizeof(name)));
-    FAIL_IF(strcmp(command, "set") != 0);
-    FAIL_IF(strcmp(name, "flowbit") != 0);
-
-    /* Spaces are not allowed in the name. */
-    FAIL_IF(DetectFlowbitParse("set,namewith space", command, sizeof(command),
-            name, sizeof(name)));
+    DetectEngineCtx *de_ctx = NULL;
+    de_ctx = DetectEngineCtxInit();
+    FAIL_IF_NULL(de_ctx);
+    de_ctx->flags |= DE_QUIET;
+    DetectFlowbitsData *cd = NULL;
+
+#define BAD_INPUT(str) FAIL_IF_NOT(DetectFlowbitParse(de_ctx, (str), &cd) == -1);
+
+    BAD_INPUT("alert");
+    BAD_INPUT("n0alert");
+    BAD_INPUT("nOalert");
+    BAD_INPUT("set,namewith space");
+    BAD_INPUT("issset,typo");
+    BAD_INPUT("isset,a b | c");
+
+#undef BAD_INPUT
 
+#define GOOD_INPUT(str, command)                                                                   \
+    FAIL_IF_NOT(DetectFlowbitParse(de_ctx, (str), &cd) == 0);                                      \
+    FAIL_IF_NULL(cd);                                                                              \
+    FAIL_IF_NOT(cd->cmd == (command));                                                             \
+    DetectFlowbitFree(NULL, cd);                                                                   \
+    cd = NULL;
+
+    GOOD_INPUT("unset, flowbit ", DETECT_FLOWBITS_CMD_UNSET);
+    GOOD_INPUT("set,flowbit", DETECT_FLOWBITS_CMD_SET);
+    GOOD_INPUT("isset, flowbit", DETECT_FLOWBITS_CMD_ISSET);
+    GOOD_INPUT("isnotset,flowbit ", DETECT_FLOWBITS_CMD_ISNOTSET);
+    GOOD_INPUT("toggle, flowbit ", DETECT_FLOWBITS_CMD_TOGGLE);
+    GOOD_INPUT("isset, fb1|fb2", DETECT_FLOWBITS_CMD_ISSET);
+
+#undef GOOD_INPUT
+
+    DetectEngineCtxFree(de_ctx);
     PASS;
 }