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.

diff --git a/include/haproxy/cfgcond.h b/include/haproxy/cfgcond.h
index a1f86e6e43c780c3c93865da857b5b08f61c87b1..3171f8192001babcaf7b0b8fe0d08a498c02c111 100644 (file)
--- a/include/haproxy/cfgcond.h
+++ b/include/haproxy/cfgcond.h
@@ -26,15 +26,15 @@
 #include <haproxy/cfgcond-t.h>
 
 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 3f6cbfd99502b01acfb9d605cdb3a6027d0453c3..e07ff324f9c73956b4f11a059319323c427efab7 100644 (file)
--- 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 c1259c0f47f1079c8c26083801a7fbae3c61673f..50ba3dfab8568e3d4147a70d5ec473f69a66a47c 100644 (file)
--- 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 <term> using
  * cfg_free_cond_term(). An error will be set in <err> on error, and only
  * in this case. In this case the first bad character will be reported in
- * <errptr>.
+ * <errptr>. <maxdepth> 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
  * <err> on error, and only in this case. In this case the first bad
- * character will be reported in <errptr>.
+ * character will be reported in <errptr>. <maxdepth> 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
  * <err> on error, and only in this case. In this case the first bad
- * character will be reported in <errptr>.
+ * character will be reported in <errptr>. <maxdepth> 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;