Reject len == SIZE_MAX in unsafe_yyjson_mut_str_alc#268
Merged
Conversation
unsafe_yyjson_mut_str_alc() in src/yyjson.h computes `len + 1` (to
reserve a trailing null byte) without checking whether the input
`len` is `(size_t)-1`. When that happens:
* `len + 1` wraps to 0.
* The grow path passes 0 to unsafe_yyjson_str_pool_grow(), which
allocates a tiny chunk and returns success.
* The non-grow path is also bypassed because the
`(pool->end - pool->cur) <= len` test compares against the wrapped
SIZE_MAX value.
* `pool->cur = mem + len + 1` wraps to `mem`.
* unsafe_yyjson_mut_strncpy() then runs
`memcpy(mem, str, (size_t)-1)`, which reads SIZE_MAX bytes from
the caller's buffer into a chunk that does not have anywhere near
that much space.
This affects every public mutable-string API that forwards an
untrusted length unchanged: yyjson_mut_strncpy(), yyjson_mut_rawncpy(),
yyjson_mut_obj_add_strncpy(), yyjson_mut_arr_with_strncpy(),
yyjson_mut_arr_with_strn(), and any caller of unsafe_yyjson_mut_strncpy.
Reproducer (ASan, on this tree before the fix):
yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL);
const char src[8] = "abcdefg";
yyjson_mut_strncpy(doc, src, (size_t)-1);
==ERROR: AddressSanitizer: negative-size-param: (size=-1)
#0 __asan_memcpy
ibireme#1 main repro.c:... -- inside unsafe_yyjson_mut_strncpy
Fix: reject `len == (size_t)-1` at the top of the allocator helper, so
`len + 1` is always well-defined. After the fix the same call returns
NULL (the documented error behavior) instead of corrupting memory. The
existing bound in unsafe_yyjson_str_pool_grow() already handles every
other large value, so no other changes are needed.
Tests: configured with -DYYJSON_BUILD_TESTS=ON -DYYJSON_SANITIZER=undefined
on Apple clang and ran ctest -- 12/12 pass.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #268 +/- ##
=======================================
Coverage 98.49% 98.49%
=======================================
Files 2 2
Lines 7699 7700 +1
=======================================
+ Hits 7583 7584 +1
Misses 116 116
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owner
|
Thanks for the patch, merged. |
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.
Bug
unsafe_yyjson_mut_str_alc()insrc/yyjson.hcomputeslen + 1(to reserve anull terminator) without first checking whether the caller-supplied
lenis(size_t)-1. When that happens:len + 1wraps to0.0tounsafe_yyjson_str_pool_grow(), which checkslen > USIZE_MAX - sizeof(yyjson_str_chunk)--0 > max_lenis false, so thegrow proceeds and allocates a tiny chunk.
(pool->end - pool->cur) <= lenis truefor the wrapped SIZE_MAX value.
pool->cur = mem + len + 1wraps back tomem.unsafe_yyjson_mut_strncpy()(and every caller listed below) then runsmemcpy(mem, str, (size_t)-1), which reads SIZE_MAX bytes from the caller'sbuffer into a chunk that does not have anywhere close to that much space.
The vulnerable helper sits behind every public mutable-string API that forwards
an untrusted length unchanged, including
yyjson_mut_strncpy(),yyjson_mut_rawncpy(),yyjson_mut_obj_add_strncpy(),yyjson_mut_arr_with_strncpy(), andyyjson_mut_arr_with_strn()(the last viaunsafe_yyjson_set_strnafter copy). Any service that derives the length fromattacker-controlled data and passes it straight in is exposed.
Reproducer
Compiled against the current tree with AddressSanitizer:
Fix
Reject
len == (size_t)-1at the top ofunsafe_yyjson_mut_str_alc()so thatlen + 1is always well-defined for the rest of the function. After the fixthe reproducer returns
NULL(the documented error path for the public APIs)instead of corrupting memory.
The pre-existing bound check in
unsafe_yyjson_str_pool_grow()(
len > USIZE_MAX - sizeof(yyjson_str_chunk)) already rejects every otheroversize value cleanly; only the exact SIZE_MAX wrap-around needed an
additional guard.
Test plan
-DYYJSON_BUILD_TESTS=ON -DYYJSON_SANITIZER=undefinedonApple clang and rebuilt.
ctest-- 12/12 tests pass.yyjson_mut_strncpynow returnsNULLinsteadof triggering an ASan report.
Change is one file, four added lines (one branch + a short comment).