From dc70c18ddc9cca8913fa407a6e1448fb1f576872 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 20 Jul 2021 17:58:34 +0200 Subject: [PATCH] BUG/MEDIUM: cfgcond: limit recursion level in the condition expression parser Oss-fuzz reports in issue 36328 that we can recurse too far by passing extremely deep expressions to the ".if" parser. I thought we were still limited to the 1024 chars per line, that would be highly sufficient, but we don't have any limit now :-/ Let's just pass a maximum recursion counter to the recursive parsers. It's decremented for each call and the expression fails if it reaches zero. On the most complex paths it can add 3 levels per parenthesis, so with a limit of 1024, that's roughly 343 nested sub-expressions that are supported in the worst case. That's more than sufficient, for just a few kB of RAM. No backport is needed. --- include/haproxy/cfgcond.h | 6 ++--- include/haproxy/defaults.h | 5 ++++ src/cfgcond.c | 50 +++++++++++++++++++++++++++++--------- 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/include/haproxy/cfgcond.h b/include/haproxy/cfgcond.h index a1f86e6e4..3171f8192 100644 --- a/include/haproxy/cfgcond.h +++ b/include/haproxy/cfgcond.h @@ -26,15 +26,15 @@ #include const struct cond_pred_kw *cfg_lookup_cond_pred(const char *str); -int cfg_parse_cond_term(const char **text, struct cfg_cond_term **term, char **err, const char **errptr); +int cfg_parse_cond_term(const char **text, struct cfg_cond_term **term, char **err, const char **errptr, int maxdepth); int cfg_eval_cond_term(const struct cfg_cond_term *term, char **err); void cfg_free_cond_term(struct cfg_cond_term *term); -int cfg_parse_cond_and(const char **text, struct cfg_cond_and **expr, char **err, const char **errptr); +int cfg_parse_cond_and(const char **text, struct cfg_cond_and **expr, char **err, const char **errptr, int maxdepth); int cfg_eval_cond_and(struct cfg_cond_and *expr, char **err); void cfg_free_cond_and(struct cfg_cond_and *expr); -int cfg_parse_cond_expr(const char **text, struct cfg_cond_expr **expr, char **err, const char **errptr); +int cfg_parse_cond_expr(const char **text, struct cfg_cond_expr **expr, char **err, const char **errptr, int maxdepth); int cfg_eval_cond_expr(struct cfg_cond_expr *expr, char **err); void cfg_free_cond_expr(struct cfg_cond_expr *expr); diff --git a/include/haproxy/defaults.h b/include/haproxy/defaults.h index 3f6cbfd99..e07ff324f 100644 --- a/include/haproxy/defaults.h +++ b/include/haproxy/defaults.h @@ -106,6 +106,11 @@ // This should cover at least 5 + twice the # of data_types #define MAX_CLI_ARGS 64 +// max recursion levels in config condition evaluations +// (note that binary operators add one recursion level, and +// that parenthesis may add two). +#define MAX_CFG_RECURSION 1024 + // max # of matches per regexp #define MAX_MATCH 10 diff --git a/src/cfgcond.c b/src/cfgcond.c index c1259c0f4..50ba3dfab 100644 --- a/src/cfgcond.c +++ b/src/cfgcond.c @@ -68,9 +68,11 @@ void cfg_free_cond_term(struct cfg_cond_term *term) * untouched on failure. On success, the caller must free using * cfg_free_cond_term(). An error will be set in on error, and only * in this case. In this case the first bad character will be reported in - * . + * . corresponds to the maximum recursion depth permitted, + * it is decremented on each recursive call and the parsing will fail one + * reaching <= 0. */ -int cfg_parse_cond_term(const char **text, struct cfg_cond_term **term, char **err, const char **errptr) +int cfg_parse_cond_term(const char **text, struct cfg_cond_term **term, char **err, const char **errptr, int maxdepth) { struct cfg_cond_term *t; const char *in = *text; @@ -86,6 +88,10 @@ int cfg_parse_cond_term(const char **text, struct cfg_cond_term **term, char **e if (!*in) /* empty term does not parse */ return 0; + *term = NULL; + if (maxdepth <= 0) + goto fail0; + t = *term = calloc(1, sizeof(**term)); if (!t) { memprintf(err, "memory allocation error while parsing conditional expression '%s'", *text); @@ -117,7 +123,7 @@ int cfg_parse_cond_term(const char **text, struct cfg_cond_term **term, char **e t->args = NULL; do { in++; } while (*in == ' ' || *in == '\t'); - ret = cfg_parse_cond_expr(&in, &t->expr, err, errptr); + ret = cfg_parse_cond_expr(&in, &t->expr, err, errptr, maxdepth - 1); if (ret == -1) goto fail2; if (ret == 0) @@ -275,9 +281,11 @@ void cfg_free_cond_expr(struct cfg_cond_expr *expr) * on failure. On success, the caller will have to free all lower-level * allocated structs using cfg_free_cond_expr(). An error will be set in * on error, and only in this case. In this case the first bad - * character will be reported in . + * character will be reported in . corresponds to the + * maximum recursion depth permitted, it is decremented on each recursive + * call and the parsing will fail one reaching <= 0. */ -int cfg_parse_cond_and(const char **text, struct cfg_cond_and **expr, char **err, const char **errptr) +int cfg_parse_cond_and(const char **text, struct cfg_cond_and **expr, char **err, const char **errptr, int maxdepth) { struct cfg_cond_and *e; const char *in = *text; @@ -286,13 +294,21 @@ int cfg_parse_cond_and(const char **text, struct cfg_cond_and **expr, char **err if (!*in) /* empty expr does not parse */ return 0; + *expr = NULL; + if (maxdepth <= 0) { + memprintf(err, "unparsable conditional sub-expression '%s'", in); + if (errptr) + *errptr = in; + goto done; + } + e = *expr = calloc(1, sizeof(**expr)); if (!e) { memprintf(err, "memory allocation error while parsing conditional expression '%s'", *text); goto done; } - ret = cfg_parse_cond_term(&in, &e->left, err, errptr); + ret = cfg_parse_cond_term(&in, &e->left, err, errptr, maxdepth - 1); if (ret == -1) // parse error, error already reported goto done; @@ -320,7 +336,7 @@ int cfg_parse_cond_and(const char **text, struct cfg_cond_and **expr, char **err while (*in == ' ' || *in == '\t') in++; - ret = cfg_parse_cond_and(&in, &e->right, err, errptr); + ret = cfg_parse_cond_and(&in, &e->right, err, errptr, maxdepth - 1); if (ret > 0) *text = in; done: @@ -338,9 +354,11 @@ int cfg_parse_cond_and(const char **text, struct cfg_cond_and **expr, char **err * on failure. On success, the caller will have to free all lower-level * allocated structs using cfg_free_cond_expr(). An error will be set in * on error, and only in this case. In this case the first bad - * character will be reported in . + * character will be reported in . corresponds to the + * maximum recursion depth permitted, it is decremented on each recursive call + * and the parsing will fail one reaching <= 0. */ -int cfg_parse_cond_expr(const char **text, struct cfg_cond_expr **expr, char **err, const char **errptr) +int cfg_parse_cond_expr(const char **text, struct cfg_cond_expr **expr, char **err, const char **errptr, int maxdepth) { struct cfg_cond_expr *e; const char *in = *text; @@ -349,13 +367,21 @@ int cfg_parse_cond_expr(const char **text, struct cfg_cond_expr **expr, char **e if (!*in) /* empty expr does not parse */ return 0; + *expr = NULL; + if (maxdepth <= 0) { + memprintf(err, "unparsable conditional expression '%s'", in); + if (errptr) + *errptr = in; + goto done; + } + e = *expr = calloc(1, sizeof(**expr)); if (!e) { memprintf(err, "memory allocation error while parsing conditional expression '%s'", *text); goto done; } - ret = cfg_parse_cond_and(&in, &e->left, err, errptr); + ret = cfg_parse_cond_and(&in, &e->left, err, errptr, maxdepth - 1); if (ret == -1) // parse error, error already reported goto done; @@ -383,7 +409,7 @@ int cfg_parse_cond_expr(const char **text, struct cfg_cond_expr **expr, char **e while (*in == ' ' || *in == '\t') in++; - ret = cfg_parse_cond_expr(&in, &e->right, err, errptr); + ret = cfg_parse_cond_expr(&in, &e->right, err, errptr, maxdepth - 1); if (ret > 0) *text = in; done: @@ -440,7 +466,7 @@ int cfg_eval_condition(char **args, char **err, const char **errptr) if (!*text) /* note: empty = false */ return 0; - ret = cfg_parse_cond_expr(&text, &expr, err, errptr); + ret = cfg_parse_cond_expr(&text, &expr, err, errptr, MAX_CFG_RECURSION); if (ret != 0) { if (ret == -1) // parse error, error already reported goto done;