Skip to content

autolink: fix unsigned pointer overflow in validate_protocol - #394

Open
iceray00 wants to merge 1 commit into
github:masterfrom
iceray00:fix-autolink-pointer-overflow
Open

iceray00 wants to merge 1 commit into
github:masterfrom
iceray00:fix-autolink-pointer-overflow

Conversation

@iceray00

Copy link
Copy Markdown

validate_protocol() reads the character immediately before a matched protocol:

char prev_char = data[-((ptrdiff_t)rewind) - len - 1];

len is a size_t, so the usual arithmetic conversions promote the whole index to
size_t. Instead of the intended small negative offset the expression evaluates to a
huge unsigned value, and adding it to data wraps the pointer. UBSan aborts on it:

extensions/autolink.c:315:20: runtime error: addition of unsigned offset to 0x6080000009d5 overflowed to 0x6080000009c6

The address that is finally computed is the intended one and lies inside the text
buffer: the guard right above guarantees rewind + len + 1 <= max_rewind. The problem
is the wrapping arithmetic itself, which is undefined behaviour and makes every UBSan
build abort on crafted input even though the parse is correct.

The fix keeps the index signed:

// Compute the offset as a signed value: with size_t operands the index
// becomes a huge unsigned number and adding it to `data` wraps the pointer,
// which is undefined behaviour even though the address computed is in bounds.
char prev_char = data[-((ptrdiff_t)(rewind + len + 1))];

The byte that is read is unchanged, so there is no change in observable behaviour.

Reproducing

Found with UBSan using the upstream test/cmark-fuzz.c driver
(-fsanitize=undefined,address -fno-sanitize-recover=all). The two inputs attached to
the issue reach the same line through different entry points (validate_protocol
directly and postprocess_text); with the 8-byte fuzz config header stripped they also
reproduce through the CLI:

$ cmark-gfm --extension autolink poc-a.md
extensions/autolink.c:315:20: runtime error: addition of unsigned offset to 0x607000000283 overflowed to 0x607000000274
$ cmark-gfm --extension autolink poc-b.md
extensions/autolink.c:315:20: runtime error: addition of unsigned offset to 0x606000000210 overflowed to 0x606000000200

Both runs are clean with the fix applied, ctest still passes 13/13, and the rendered
HTML for the two inputs is byte-identical before and after.

Fixes #393

validate_protocol computes the character just before the protocol with:

    char prev_char = data[-((ptrdiff_t)rewind) - len - 1];

len is a size_t, so the usual arithmetic conversions make the whole index a
size_t: instead of a small negative offset the expression evaluates to a huge
unsigned value, and adding it to data wraps the pointer. That is undefined
behaviour, and UBSan builds abort on it:

    extensions/autolink.c:315:20: runtime error: addition of unsigned offset
    to 0x6080000009d5 overflowed to 0x6080000009c6

The address that is finally computed is the intended one and lies inside the
text buffer (the guard above guarantees rewind + len + 1 <= max_rewind), so the
only problem is the wrapping arithmetic itself. It is still undefined
behaviour, and it makes every UBSan build abort on crafted input even when the
resulting parse is correct.

Cast the complete offset to ptrdiff_t so the index stays signed and the pointer
arithmetic cannot wrap. The byte that is read is unchanged, so there is no
change in observable behaviour.

Found with UBSan using the upstream test/cmark-fuzz.c driver.

Fixes github#393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UBSan pointer-overflow in autolink postprocessing (extensions/autolink.c:315)

1 participant