Skip to content

fix(string): reject oversized len in JS_NewStringUTF16#1486

Open
andreasrosdal wants to merge 1 commit into
quickjs-ng:masterfrom
nordstjernen-web:claude/sec-newstring-utf16-length
Open

fix(string): reject oversized len in JS_NewStringUTF16#1486
andreasrosdal wants to merge 1 commit into
quickjs-ng:masterfrom
nordstjernen-web:claude/sec-newstring-utf16-length

Conversation

@andreasrosdal
Copy link
Copy Markdown
Contributor

The C API took a size_t len but passed it to js_alloc_string, whose length parameter is int. With len > INT_MAX (e.g. INT_MAX + 1), the cast truncated the value, producing either a tiny or negative-sized allocation while the subsequent memcpy(str16(str), buf, len * 2) wrote the full size_t length — heap overflow on misuse from C.

Reject len > JS_STRING_LEN_MAX before allocating, matching the existing guard in JS_NewStringLen.

Test: api-test now calls JS_NewStringUTF16(ctx, NULL, INT_MAX + 1) and asserts JS_IsException + the "invalid string length" error. Before the fix, the same call segfaults (or is caught by ASan as a heap-buffer-overflow).

The C API took a size_t len but passed it to js_alloc_string, whose
length parameter is int. With len > INT_MAX (e.g. INT_MAX + 1), the
cast truncated the value, producing either a tiny or negative-sized
allocation while the subsequent memcpy(str16(str), buf, len * 2)
wrote the full size_t length — heap overflow on misuse from C.

Reject len > JS_STRING_LEN_MAX before allocating, matching the
existing guard in JS_NewStringLen.

Test: api-test now calls JS_NewStringUTF16(ctx, NULL, INT_MAX + 1)
and asserts JS_IsException + the "invalid string length" error.
Before the fix, the same call segfaults (or is caught by ASan as a
heap-buffer-overflow).
Copy link
Copy Markdown
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo 2 comments (comments about comments, in fact.)

Comment thread quickjs.c
Comment on lines +4413 to +4417
/* Without this clamp, a size_t length just above INT_MAX would truncate
when passed to js_alloc_string (whose length parameter is int). The
resulting allocation is tiny or negative-shifted, while the subsequent
memcpy(str16(str), buf, len * 2) writes the full len*2 bytes — heap
overflow. The sibling JS_NewStringLen has the same guard. */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a superfluous comment. It also has that AI vibe to it.

Suggested change
/* Without this clamp, a size_t length just above INT_MAX would truncate
when passed to js_alloc_string (whose length parameter is int). The
resulting allocation is tiny or negative-shifted, while the subsequent
memcpy(str16(str), buf, len * 2) writes the full len*2 bytesheap
overflow. The sibling JS_NewStringLen has the same guard. */

Comment thread api-test.c
Comment on lines +490 to +497
/* Oversized length: must throw RangeError, not corrupt the heap.
Before the fix, len > INT_MAX was truncated when passed to
js_alloc_string(int), producing a tiny allocation that the
subsequent memcpy(..., len * 2) overflowed. Pre-fix: ASan
reports heap-buffer-overflow or process aborts. Post-fix:
returns an exception. We don't materialise a multi-GB buffer
here — JS_NewStringUTF16 must reject the length *before*
reading from buf. */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto:

Suggested change
/* Oversized length: must throw RangeError, not corrupt the heap.
Before the fix, len > INT_MAX was truncated when passed to
js_alloc_string(int), producing a tiny allocation that the
subsequent memcpy(..., len * 2) overflowed. Pre-fix: ASan
reports heap-buffer-overflow or process aborts. Post-fix:
returns an exception. We don't materialise a multi-GB buffer
hereJS_NewStringUTF16 must reject the length *before*
reading from buf. */

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.

3 participants