BUG/MINOR: mjson: fix out-of-bounds reads on malformed JSON paths

The path passed to mjson_find() is parsed without always staying within
its bounds, so a malformed path can read past its end. This is a
heap-buffer-overflow whenever the path is stored in an exactly sized
allocation, which is the case of the json_query converter's argument
since it is duplicated by my_strndup() at configuration parsing time.
Two code paths are involved.

plen1() and plen2() measure a path segment and skip two characters when
they see a backslash, so that an escaped '.' or '[' is not taken as a
separator. When the path ends with a backslash, the second skipped
character is the trailing zero and the loop then reads past the end of
the string. With:

  http-request set-var(txn.out) req.fhdr(json),json_query($.a\\)

(the config parser turns "\\" into a single backslash, so the path is
"$.a\" followed by the terminating zero). Every request carrying a JSON
object in the "json" header then makes plen1() read past the end of the
5-byte allocation. ASan reports a heap-buffer-overflow READ of size 1 in
plen1(), reached from sample_conv_json_query(). Paths without a
trailing backslash are not affected.

Then mjson_get_cb() locates the array index either by skipping a fixed 3
bytes from the path position when the index is 0, or by scanning
forward for a ']' with no bound otherwise. A truncated index breaks
both: for "$.a[" and "$.a[0", the fixed skip lands past the trailing
zero, and for "$.a[1", the scan walks past the end of the string until
it happens to meet a ']' further in the heap. Nothing validates the
path at configuration parsing time, so such a configuration passes
"haproxy -c" and only fails when the converter is evaluated on a
matching request.

These paths come from the configuration only: the JWE and ACME code
uses hardcoded paths or snprintf() with a bounded index, and the other
callers pass constant strings. So the bug is only reachable when the
configuration itself is malformed, which limits its severity.

Let's only skip the character following a backslash when it is not the
end of the string, in both plen1() and plen2(), and move the array index
parsing to a new skip_idx() helper. It only accepts a decimal number
immediately followed by a ']', and never scans past the end of the
string so that a truncated index is reported as not found. This is
slightly stricter than the previous code, which accepted "+1", "1 " and
"1x" as index 1 because mystrtod() stops at the first character that is
not part of the number, while "$.a[00]" is now recognized as index 0
instead of being skipped as a 3-byte suffix.

The plen1() and plen2() bug is still present in the upstream mjson
project.

This should be backported to all stable versions. The code came with
mjson in 2.4.

Co-authored-by: Martin Mirchev <mirchevmartin2203@gmail.com>

diff --git a/src/mjson.c b/src/mjson.c
index 603947d721e85445721c2e73b81aea82c4002d7d..9a94ed553215c7e2fce47a00b445afb001969667 100644 (file)
--- a/src/mjson.c
+++ b/src/mjson.c
@@ -176,7 +176,7 @@ struct msjon_get_data {
 static int plen1(const char *s) {
   int i = 0, n = 0;
   while (s[i] != '\0' && s[i] != '.' && s[i] != '[')
-    n++, i += s[i] == '\\' ? 2 : 1;
+    n++, i += (s[i] == '\\' && s[i + 1] != '\0') ? 2 : 1;
   // printf("PLEN: s: [%s], [%.*s] => %d\n", s, i, s, n);
   return n;
 }
@@ -184,11 +184,25 @@ static int plen1(const char *s) {
 static int plen2(const char *s) {
   int i = 0, __attribute__((unused)) n = 0;
   while (s[i] != '\0' && s[i] != '.' && s[i] != '[')
-    n++, i += s[i] == '\\' ? 2 : 1;
+    n++, i += (s[i] == '\\' && s[i + 1] != '\0') ? 2 : 1;
   // printf("PLEN: s: [%s], [%.*s] => %d\n", s, i, s, n);
   return i;
 }
 
+// Moves <*pos> past the array index it points to, i.e. from the opening '['
+// to the character following the closing ']'. Returns 0 if the index is not
+// a decimal number properly terminated by a ']', and leaves <*pos> untouched.
+// The scan never goes past the end of the string so that a truncated index
+// such as "$.a[" or "$.a[0" cannot cause an overread.
+static int skip_idx(const char *path, int *pos) {
+  int i = *pos + 1;
+  if (path[i] < '0' || path[i] > '9') return 0;
+  while (path[i] >= '0' && path[i] <= '9') i++;
+  if (path[i] != ']') return 0;
+  *pos = i + 1;
+  return 1;
+}
+
 static int kcmp(const char *a, const char *b, int n) {
   int i = 0, j = 0, r = 0;
   for (i = 0, j = 0; j < n; i++, j++) {
@@ -213,21 +227,16 @@ static int mjson_get_cb(int tok, const char *s, int off, int len, void *ud) {
     if (data->d1 == data->d2 && data->path[data->pos] == '[') {
       data->i1 = 0;
       data->i2 = (int) mystrtod(&data->path[data->pos + 1], strlen(&data->path[data->pos + 1]),  NULL);
-      if (data->i1 == data->i2) {
+      if (data->i1 == data->i2 && skip_idx(data->path, &data->pos))
         data->d2++;
-        data->pos += 3;
-      }
     }
     if (!data->path[data->pos] && data->d1 == data->d2) data->obj = off;
     data->d1++;
   } else if (tok == ',') {
     if (data->d1 == data->d2 + 1) {
       data->i1++;
-      if (data->i1 == data->i2) {
-        while (data->path[data->pos] != ']') data->pos++;
-        data->pos++;
+      if (data->i1 == data->i2 && skip_idx(data->path, &data->pos))
         data->d2++;
-      }
     }
   } else if (tok == MJSON_TOK_KEY && data->d1 == data->d2 + 1 &&
              data->path[data->pos] == '.' && s[off] == '"' &&