Fix use-after-free in cJSONUtils_SortObject: restore child->prev tail invariant - #1091
Open
nvnzno-a11y wants to merge 1 commit into
Open
nvnzno-a11y wants to merge 1 commit into
nvnzno-a11y wants to merge 1 commit into
Conversation
The mergesort in sort_list() rebuilds the object's child list but never restores cJSON's invariant that the head's prev pointer references the list tail. After cJSONUtils_SortObject(), child->prev is either NULL or a live interior node. Deleting that node leaves child->prev dangling, and the next add_item_to_array() then performs suffix_object() -> prev->next = item, an 8-byte write into freed heap memory (also drops the appended item). Fixes DaveGamble#1090.
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.
Issue
Fixes #1090 — use-after-free (8-byte pointer write) reported by ReisterJ with a minimal three-call PoC (
cJSONUtils_SortObject→cJSON_DeleteItemFromObject→cJSON_AddItemToObject).Root cause
sort_list()rebuilds an object's doubly linked child list with mergesort but never restores cJSON's list invariant that the head'sprevpointer references the list tail. The parser establishes it (head->prev = current_itemincJSON.c) andadd_item_to_array()consumes it viasuffix_object(child->prev, item)for O(1) appends. After a sort,child->previs stale —NULLor a live interior node. Deleting that interior node leaveschild->prevdangling, and the next append writesprev->next = iteminto freed heap memory (and silently drops the new item whenprevisNULL).Fix
After the merge completes, walk to the end of the merged list once and set
result->prevto the tail. One extra O(n) pass persort_listcall, negligible next to the O(n log n) sort itself. All otherprevlinks are already maintained by the merge.Verification
6d9f244):child->prevno longer equals the tail aftercJSONUtils_SortObject, and a custom allocator harness (poison-on-free) observes the stale write into the freed node. On the patched build the invariant holds, the appended item lands at the tail, and no write touches a freed block.sort_object_should_restore_prev_tail_invariantintests/misc_utils_tests.ccheckschild->prev == tailafter sorting and after a delete+add sequence. It fails on the unpatched build and passes with the fix.prev-chain integrity (forward and backward walks) across 10 cases: sorted/reversed/duplicate keys, case-sensitive and case-insensitive, nested objects, and append-after-sort.