From: Alexander Gozman Date: Fri, 4 Mar 2016 13:18:46 +0000 (+0300) Subject: BUG #1689: fix stack overflow when parsing variables X-Git-Tag: suricata-3.0.1RC1~76 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=69785f186c227c44a958d3684500b5da5e1a6749;p=thirdparty%2Fsuricata.git 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 31e6a7ab81..764af6a85f 100644 --- 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 1c9c0228f7..2585e6bdbe 100644 --- 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 e4e9689b35..40d4f7f31e 100644 --- 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 0cd8c3528b..dc284725ff 100644 --- 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__ */