BUG #1689: fix stack overflow when parsing variables

Suricata crashed when variable (either address or port)
referred to itself or if one created a looped chain of
variables. For instance:

HOME_NET:     "!$EXTERNAL_NET"
EXTERNAL_NET: "!$HOME_NET"

Or:

Var1: "$Var2"
Var2: "$Var3"
Var3: "$Var1"

diff --git a/src/detect-engine-address.c b/src/detect-engine-address.c
index 31e6a7ab81b7e7ff7c791e15e4504a928a41b5fa..764af6a85f5a7fd6c93a50fb92f7359ff01d2aac 100644 (file)
--- a/src/detect-engine-address.c
+++ b/src/detect-engine-address.c
@@ -45,6 +45,7 @@
 
 #include "util-debug.h"
 #include "util-print.h"
+#include "util-var.h"
 
 /* prototypes */
 void DetectAddressPrint(DetectAddress *);
@@ -915,7 +916,7 @@ error:
  */
 static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
         DetectAddressHead *gh, DetectAddressHead *ghn,
-        char *s, int negate)
+        char *s, int negate, ResolvedVariablesList *var_list)
 {
     size_t x = 0;
     size_t u = 0;
@@ -926,6 +927,12 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
     char *rule_var_address = NULL;
     char *temp_rule_var_address = NULL;
 
+    if (AddVariableToResolveList(var_list, s) == -1) {
+        SCLogError(SC_ERR_INVALID_YAML_CONF_ENTRY, "Found a loop in a address "
+                   "groups declaration. This is likely a misconfiguration.");
+        goto error;
+    }
+
     SCLogDebug("s %s negate %s", s, negate ? "true" : "false");
 
     for (u = 0, x = 0; u < size && x < sizeof(address); u++) {
@@ -956,7 +963,7 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                     /* normal block */
                     SCLogDebug("normal block");
 
-                    if (DetectAddressParse2(de_ctx, gh, ghn, address, (negate + n_set) % 2) < 0)
+                    if (DetectAddressParse2(de_ctx, gh, ghn, address, (negate + n_set) % 2, var_list) < 0)
                         goto error;
                 } else {
                     /* negated block
@@ -969,7 +976,7 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                     DetectAddressHead tmp_gh = { NULL, NULL, NULL };
                     DetectAddressHead tmp_ghn = { NULL, NULL, NULL };
 
-                    if (DetectAddressParse2(de_ctx, &tmp_gh, &tmp_ghn, address, 0) < 0)
+                    if (DetectAddressParse2(de_ctx, &tmp_gh, &tmp_ghn, address, 0, var_list) < 0)
                         goto error;
 
                     DetectAddress *tmp_ad;
@@ -1037,6 +1044,7 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                                                         SC_RULE_VARS_ADDRESS_GROUPS);
                 if (rule_var_address == NULL)
                     goto error;
+
                 if (strlen(rule_var_address) == 0) {
                     SCLogError(SC_ERR_INVALID_SIGNATURE, "variable %s resolved "
                             "to nothing. This is likely a misconfiguration. "
@@ -1044,6 +1052,7 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                             "\"!$HOME_NET\" instead of !$HOME_NET. See issue #295.", s);
                     goto error;
                 }
+
                 SCLogDebug("rule_var_address %s", rule_var_address);
                 temp_rule_var_address = rule_var_address;
                 if ((negate + n_set) % 2) {
@@ -1053,8 +1062,12 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                     snprintf(temp_rule_var_address, strlen(rule_var_address) + 3,
                              "[%s]", rule_var_address);
                 }
-                DetectAddressParse2(de_ctx, gh, ghn, temp_rule_var_address,
-                                    (negate + n_set) % 2);
+
+
+                if (DetectAddressParse2(de_ctx, gh, ghn, temp_rule_var_address,
+                                    (negate + n_set) % 2, var_list) < 0)
+                    goto error;
+
                 d_set = 0;
                 n_set = 0;
                 if (temp_rule_var_address != rule_var_address)
@@ -1089,6 +1102,7 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                                                         SC_RULE_VARS_ADDRESS_GROUPS);
                 if (rule_var_address == NULL)
                     goto error;
+
                 if (strlen(rule_var_address) == 0) {
                     SCLogError(SC_ERR_INVALID_SIGNATURE, "variable %s resolved "
                             "to nothing. This is likely a misconfiguration. "
@@ -1096,6 +1110,7 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                             "\"!$HOME_NET\" instead of !$HOME_NET. See issue #295.", s);
                     goto error;
                 }
+
                 SCLogDebug("rule_var_address %s", rule_var_address);
                 temp_rule_var_address = rule_var_address;
                 if ((negate + n_set) % 2) {
@@ -1105,8 +1120,9 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
                     snprintf(temp_rule_var_address, strlen(rule_var_address) + 3,
                             "[%s]", rule_var_address);
                 }
+
                 if (DetectAddressParse2(de_ctx, gh, ghn, temp_rule_var_address,
-                                    (negate + n_set) % 2) < 0) {
+                                    (negate + n_set) % 2, var_list) < 0) {
                     SCLogDebug("DetectAddressParse2 hates us");
                     goto error;
                 }
@@ -1146,6 +1162,7 @@ static int DetectAddressParse2(const DetectEngineCtx *de_ctx,
     return 0;
 
 error:
+
     return -1;
 }
 
@@ -1368,6 +1385,8 @@ int DetectAddressTestConfVars(void)
 {
     SCLogDebug("Testing address conf vars for any misconfigured values");
 
+    ResolvedVariablesList var_list = TAILQ_HEAD_INITIALIZER(var_list);
+
     ConfNode *address_vars_node = ConfGetNode("vars.address-groups");
     if (address_vars_node == NULL) {
         return 0;
@@ -1394,7 +1413,10 @@ int DetectAddressTestConfVars(void)
             goto error;
         }
 
-        int r = DetectAddressParse2(NULL, gh, ghn, seq_node->val, /* start with negate no */0);
+        int r = DetectAddressParse2(NULL, gh, ghn, seq_node->val, /* start with negate no */0, &var_list);
+
+        CleanVariableResolveList(&var_list);
+
         if (r < 0) {
             SCLogError(SC_ERR_INVALID_YAML_CONF_ENTRY,
                         "failed to parse address var \"%s\" with value \"%s\". "
@@ -1402,6 +1424,8 @@ int DetectAddressTestConfVars(void)
             goto error;
         }
 
+        CleanVariableResolveList(&var_list);
+
         if (DetectAddressIsCompleteIPSpace(ghn)) {
             SCLogError(SC_ERR_INVALID_YAML_CONF_ENTRY,
                        "address var - \"%s\" has the complete IP space negated "
@@ -1453,7 +1477,7 @@ int DetectAddressParse(const DetectEngineCtx *de_ctx,
         goto error;
     }
 
-    r = DetectAddressParse2(de_ctx, gh, ghn, str, /* start with negate no */0);
+    r = DetectAddressParse2(de_ctx, gh, ghn, str, /* start with negate no */0, NULL);
     if (r < 0) {
         SCLogDebug("DetectAddressParse2 returned %d", r);
         goto error;

diff --git a/src/detect-engine-port.c b/src/detect-engine-port.c
index 1c9c0228f7c115d1a93fa333553fb95c05c97239..2585e6bdbef5e4c1cc629f3afd497d60029f5b1e 100644 (file)
--- a/src/detect-engine-port.c
+++ b/src/detect-engine-port.c
@@ -51,6 +51,7 @@
 #include "pkt-var.h"
 #include "host.h"
 #include "util-profiling.h"
+#include "util-var.h"
 
 static int DetectPortCutNot(DetectPort *, DetectPort **);
 static int DetectPortCut(DetectEngineCtx *, DetectPort *, DetectPort *,
@@ -1025,7 +1026,7 @@ error:
  */
 static int DetectPortParseDo(const DetectEngineCtx *de_ctx,
                              DetectPort **head, DetectPort **nhead,
-                             char *s, int negate)
+                             char *s, int negate, ResolvedVariablesList *var_list)
 {
     size_t u = 0;
     size_t x = 0;
@@ -1037,6 +1038,12 @@ static int DetectPortParseDo(const DetectEngineCtx *de_ctx,
     char *rule_var_port = NULL;
     int r = 0;
 
+    if (AddVariableToResolveList(var_list, s) == -1) {
+        SCLogError(SC_ERR_INVALID_YAML_CONF_ENTRY, "Found a loop in a port "
+                   "groups declaration. This is likely a misconfiguration.");
+        goto error;
+    }
+
     SCLogDebug("head %p, *head %p, negate %d", head, *head, negate);
 
     for (u = 0, x = 0; u < size && x < sizeof(address); u++) {
@@ -1065,7 +1072,7 @@ static int DetectPortParseDo(const DetectEngineCtx *de_ctx,
                 SCLogDebug("Parsed port from DetectPortParseDo - %s", address);
                 x = 0;
 
-                r = DetectPortParseDo(de_ctx, head, nhead, address, negate? negate: n_set);
+                r = DetectPortParseDo(de_ctx, head, nhead, address, negate? negate: n_set, var_list);
                 if (r == -1)
                     goto error;
 
@@ -1103,7 +1110,7 @@ static int DetectPortParseDo(const DetectEngineCtx *de_ctx,
                     temp_rule_var_port = alloc_rule_var_port;
                 }
                 r = DetectPortParseDo(de_ctx, head, nhead, temp_rule_var_port,
-                                  (negate + n_set) % 2);//negate? negate: n_set);
+                                  (negate + n_set) % 2, var_list);//negate? negate: n_set);
                 if (r == -1)
                     goto error;
 
@@ -1163,7 +1170,7 @@ static int DetectPortParseDo(const DetectEngineCtx *de_ctx,
                     temp_rule_var_port = alloc_rule_var_port;
                 }
                 r = DetectPortParseDo(de_ctx, head, nhead, temp_rule_var_port,
-                                  (negate + n_set) % 2);
+                                  (negate + n_set) % 2, var_list);
                 if (r == -1)
                     goto error;
 
@@ -1337,6 +1344,8 @@ int DetectPortTestConfVars(void)
 {
     SCLogDebug("Testing port conf vars for any misconfigured values");
 
+    ResolvedVariablesList var_list = TAILQ_HEAD_INITIALIZER(var_list);
+
     ConfNode *port_vars_node = ConfGetNode("vars.port-groups");
     if (port_vars_node == NULL) {
         return 0;
@@ -1361,9 +1370,15 @@ int DetectPortTestConfVars(void)
             goto error;
         }
 
-        int r = DetectPortParseDo(NULL, &gh, &ghn, seq_node->val, /* start with negate no */0);
+        int r = DetectPortParseDo(NULL, &gh, &ghn, seq_node->val, /* start with negate no */0, &var_list);
+
+        CleanVariableResolveList(&var_list);
+
         if (r < 0) {
             DetectPortCleanupList(gh);
+            SCLogError(SC_ERR_INVALID_YAML_CONF_ENTRY,
+                        "failed to parse port var \"%s\" with value \"%s\". "
+                        "Please check it's syntax", seq_node->name, seq_node->val);
             goto error;
         }
 
@@ -1409,7 +1424,7 @@ int DetectPortParse(const DetectEngineCtx *de_ctx,
     /* negate port list */
     DetectPort *nhead = NULL;
 
-    r = DetectPortParseDo(de_ctx, head, &nhead, str,/* start with negate no */0);
+    r = DetectPortParseDo(de_ctx, head, &nhead, str,/* start with negate no */0, NULL);
     if (r < 0)
         goto error;
 
@@ -2786,7 +2801,7 @@ static int PortTestMatchDoubleNegation(void)
     int result = 0;
     DetectPort *head = NULL, *nhead = NULL;
 
-    if (DetectPortParseDo(NULL, &head, &nhead, "![!80]", 0) == -1)
+    if (DetectPortParseDo(NULL, &head, &nhead, "![!80]", 0, NULL) == -1)
         return result;
 
     result = (head != NULL);

diff --git a/src/util-var.c b/src/util-var.c
index e4e9689b35d9b04bdffcc600dace8e0b9aa265b5..40d4f7f31e830b00ac0185ea901610232298ebd1 100644 (file)
--- a/src/util-var.c
+++ b/src/util-var.c
@@ -128,3 +128,43 @@ void GenericVarRemove(GenericVar **list, GenericVar *gv)
         listgv = listgv->next;
     }
 }
+
+// Checks if a variable is already in a resolve list and if it's not, adds it.
+int AddVariableToResolveList(ResolvedVariablesList *list, char *var)
+{
+    ResolvedVariable *p_item;
+
+    if (list == NULL || var == NULL)
+        return 0;
+
+    TAILQ_FOREACH(p_item, list, next) {
+        if (!strcmp(p_item->var_name, var)) {
+            return -1;
+        }
+    }
+
+    p_item = SCMalloc(sizeof(ResolvedVariable));
+
+    if (unlikely(p_item == NULL)) {
+        return -1;
+    }
+
+    strlcpy(p_item->var_name, var, sizeof(p_item->var_name) - 1);
+    TAILQ_INSERT_TAIL(list, p_item, next);
+
+    return 0;
+}
+
+void CleanVariableResolveList(ResolvedVariablesList *var_list)
+{
+    if (var_list == NULL) {
+        return;
+    }
+
+    ResolvedVariable *p_item;
+
+    while ((p_item = TAILQ_FIRST(var_list))) {
+        TAILQ_REMOVE(var_list, p_item, next);
+        SCFree(p_item);
+    }
+}

diff --git a/src/util-var.h b/src/util-var.h
index 0cd8c3528b1f8192ce030d24564bb50cead4b579..dc284725ffa250bd26484e36771f7967267df550 100644 (file)
--- a/src/util-var.h
+++ b/src/util-var.h
@@ -57,9 +57,21 @@ typedef struct XBit_ {
     uint32_t expire;
 } XBit;
 
+// A list of variables we try to resolve while parsing configuration file.
+// Helps to detect recursive declarations.
+typedef struct ResolvedVariable_ {
+    char var_name[256];
+    TAILQ_ENTRY(ResolvedVariable_) next;
+} ResolvedVariable;
+
+typedef TAILQ_HEAD(, ResolvedVariable_) ResolvedVariablesList;
+
 void GenericVarFree(GenericVar *);
 void GenericVarAppend(GenericVar **, GenericVar *);
 void GenericVarRemove(GenericVar **, GenericVar *);
 
+int AddVariableToResolveList(ResolvedVariablesList *list, char *var);
+void CleanVariableResolveList(ResolvedVariablesList *var_list);
+
 #endif /* __UTIL_VAR_H__ */