Fix heap-buffer-overflow in parse_string on truncated escapes - #74
Open
afonsojanu wants to merge 1 commit into
Open
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
parse_string() estimates the output buffer size in a first pass over the input, then decodes into a buffer of that size in a second pass. The \u escape handling in the second pass advanced its cursor by a fixed 4 (or 6, for a surrogate pair) characters without checking that the string actually had that many characters left. When the input contained an embedded NUL byte or simply ran out mid-escape, the cursor could jump past where the first pass had stopped counting, so the second pass kept copying bytes the allocation was never sized for and wrote past the end of it. A bare trailing backslash right at the end of an unterminated string had the same class of problem in the length-counting pass itself, skipping one character past the terminator and reading out of bounds on the next iteration. Both cases are now treated as malformed input: parse_string bails out and frees the partial buffer instead of reading or writing past it. Valid \u escapes, including surrogate pairs, are unaffected. Reported in Bwar#73 with an ASan repro from libFuzzer; this adds a regression test that reproduces the exact crash bytes plus a few related truncation cases, and checks that normal unicode escapes still round-trip correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the heap-buffer-overflow from #73.
I reproduced the exact crash bytes from that report against a standalone build of
cJSON.cunder ASan/UBSan and got the same overflow atparse_stringline 261 (write past a 6-byte allocation).The cause:
parse_stringfirst walks the string once to estimate how many bytes it will need, then walks it again to actually decode into a buffer of that size. The\uhandling in the second pass doesptr += 4(or+= 6for a surrogate pair) unconditionally, without checking whether the string actually has that many characters left. In the crash input there's an embedded NUL right after\u, so the first (counting) pass stops there, but the second pass's fixed skip jumps past that point and keeps decoding trailing bytes as if they were still part of the string, writing past the buffer the first pass sized.While tracking this down I found a second variant of the same problem: a string ending in a bare trailing backslash with no closing quote lets the length-counting loop step one character past the terminator and read out of bounds on the next iteration.
Both are now treated as malformed input, bailing out and freeing the partial buffer rather than reading/writing past it. Valid escapes, including surrogate pairs like
😀, still decode the same as before.Added
test/parse_string_overflow_test.c, which reproduces the exact crash bytes from #73 plus a few related truncation cases, and confirms normal string/unicode parsing is unaffected. I checked that it crashes under ASan against the currentcJSON.cand passes cleanly with this fix. Build/run it directly, e.g.: