Memory pointer error in JSON sanitization

[marcstern]

In json_add_argument(), we calculate the offset of the data to sanitize the following way: arg->value_origin_offset = value - base_offset; 'value' is a parameter, 'base_offset' is a pointer containing the original string to sanitize. Usually, 'value' is a pointer in the original string pointed by 'base_offset'. BUT, when unescaping the string in yajl_do_parse() on line 253, we unescape it in a new string (hand->decodeBuf->data) and pass this new string - that is in a totally different memory space - to the yajl_string() function, passing it to json_add_argument().

Impact: this could lead to a major memory corruption (putting '*' characters in an invalid part of memory)

Solution: we need to pass the original string to yajl_string(). Hopefully, unescaping the string is always shorter, so we can copy the result inline:

- _CC_CHK(hand->callbacks->yajl_string(
                                    hand->ctx, yajl_buf_data(hand->decodeBuf),
                                    yajl_buf_len(hand->decodeBuf)));
+ bufLen = yajl_buf_len(hand->decodeBuf);
+ strcpy(buf, yajl_buf_data(hand->decodeBuf));
+ _CC_CHK(hand->callbacks->yajl_string(hand->ctx, buf, bufLen));

As this modifies the original input string, we need to copy it before parsing it. In msc_json.c in json_process_chunk():

-    base_offset = buf;
+    base_offset = apr_pstrmemdup(msr->mp, buf, size);
+    if (!base_offset) return -1;

    /* Feed our parser and catch any errors */
-    msr->json->status = yajl_parse(msr->json->handle, buf, size);
+    msr->json->status = yajl_parse(msr->json->handle, base_offset, size);

[martinhsv]

Hi @marcstern ,

I'm not sure I understand the problem that you are describing here. Would it be possible for you to provide more detail?

[marcstern]

I cannot provide more details but I can try to reexplain it in another way.

In json_add_argument(), we calculate the offset of the data to sanitize the following way: arg->value_origin_offset = value - base_offset; 'base_offset' is the beginning of the body string. 'value' is the beginning of the value string in the body. Example: body equals {"mykey": "myvalue"} 'base_offset' points to the { character (let's say address 0x555555), 'value' points to 'base_offset' + 11. We store that result in arg->value_origin_offset which is used to sanitize the value (with the length of the value in arg->value_len: we write 7 '*' at memory 0x555560 That's when the value is not escaped.

When the value is escaped in yajl_do_parse(), we allocate a new string (let's say at address 0x666666) that contains the unescaped version of the value. This is used to test patterns against ARGS. However, that value (0x666666) is used in json_add_argument(), leading to an error in the calculation of the offset: arg->value_origin_offset = 0x666666 - 0x555555;

I hope it's clear now.

[martinhsv]

Hi @marcstern ,

Thank you for the explanation. I have done some investigation and can confirm that you are correct in your main finding: that the sanitization code is trying to use the wrong memory location in the use case that you describe. So, thanks as well for pointing to that.

However, it looks to me like there actually isn't a danger of '*' sanitization characters being written to inappropriate memory locations. The relevant memset seems to be protected by a proper if-condition.

Have you actually observed (via gdb, or other means) such incorrect memory writes? Or were you merely hypothesizing that that might be the case?

As of now, the only tangible negative impact that I have identified is that the json argument will not get sanitized in the audit log.

(Note, if you have additional information to disclose that is too sensitive for a public forum, feel free to use the mechanism described in https://github.com/SpiderLabs/ModSecurity under the 'Security issue' heading.)

[marcstern]

I was able to reproduce it in debug and visualize it. It overwrites some random memory, leading (sometimes?) to a crash dump (exception on memory access).

[marcstern]

The relevant memset seems to be protected by a proper if-condition

Which one? I don't see any if condition protecting against that. Actually no condition could protect against that, as the (wrong) address could be higher than the base one.

[martinhsv]

Just to wrap up one aspect discussed in this ticket: This was discussed further offline and there is no evidence that this particular issue can result in a memory overwrite or a crash.

The non-sanitization that was confirmed on July 10, however, is a legitimate issue.