Protect the cached chunk list against corruption - #23367
Open
jvoisin wants to merge 1 commit into
Open
Conversation
When a chunk becomes empty it is not systematically unmapped: zend_mm_delete_chunk() often keeps it in heap->cached_chunks so a later allocation can reuse it without going back to mmap(). This single-linked list works via chunk->next, stored in free chunks headers, still mapped and writable. This is the same shape as free list poisoning (25360ef), and the free lists are the only thing currently protected. An overflow reaching a cached chunk header lets an attacker pick the value that zend_mm_alloc_pages() will pop: chunk = heap->cached_chunks; heap->cached_chunks = chunk->next; /* fully attacker controlled */ The popped pointer is then handed to zend_mm_chunk_init(), which writes through it and links it into the live chunk list, so a single controlled qword in a cached header turns into an arbitrary write. Given that corrupting one allocator list pointer is basically the technique to exploit CVE-2024-2961 in PHP (https://blog.lexfo.fr/iconv-cve-2024-2961-p1.html and https://blog.lexfo.fr/iconv-cve-2024-2961-p2.html), leaving a second unprotected one next to it is not great. Give the cached list the same treatment as the small bins: xor the links with heap->shadow_key, and check that the decoded value is chunk aligned before dereferencing it. NULL terminates the list and is chunk aligned, so it needs no special case. All of this is on the chunk allocation and deletion paths, which are cold, so the cost does not matter. Something that bit me during the development is rekeying: zend_mm_shutdown() calls zend_mm_refresh_key() at the end of every request, but cached chunks deliberately outlive the request, so their links have to be re-encoded with the new key. Same thing in zend_mm_refresh_key_child() for the post-fork re-key. What made this a pity to find out was that the test suite does not cover the re-keying, because the CLI serves a single request per process. It only shows up over the built-in server, where omitting the re-encode aborts on the second request. Testing was done with GDB: force a chunk into the cache, overwrite its link with 0x4141414141414141, then force a pop. Before, the corrupted pointer was accepted silently and became heap->cached_chunks. After, it aborts with "zend_mm_heap corrupted".
Contributor
Author
|
Part of #14083 |
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.
When a chunk becomes empty it is not systematically unmapped: zend_mm_delete_chunk() often keeps it in heap->cached_chunks so a later allocation can reuse it without going back to mmap(). This single-linked list works via chunk->next, stored in free chunks headers, still mapped and writable.
This is the same shape as free list poisoning
(25360ef), and the free lists are the only thing currently protected. An overflow reaching a cached chunk header lets an attacker pick the value that zend_mm_alloc_pages() will pop:
The popped pointer is then handed to zend_mm_chunk_init(), which writes through it and links it into the live chunk list, so a single controlled qword in a cached header turns into an arbitrary write. Given that corrupting one allocator list pointer is basically the technique to exploit CVE-2024-2961 in PHP (https://blog.lexfo.fr/iconv-cve-2024-2961-p1.html and https://blog.lexfo.fr/iconv-cve-2024-2961-p2.html), leaving a second unprotected one next to it is not great.
Give the cached list the same treatment as the small bins: xor the links with heap->shadow_key, and check that the decoded value is chunk aligned before dereferencing it. NULL terminates the list and is chunk aligned, so it needs no special case. All of this is on the chunk allocation and deletion paths, which are cold, so the cost does not matter.
Something that bit me during the development is rekeying: zend_mm_shutdown() calls zend_mm_refresh_key() at the end of every request, but cached chunks deliberately outlive the request, so their links have to be re-encoded with the new key. Same thing in zend_mm_refresh_key_child() for the post-fork re-key. What made this a pity to find out was that the test suite does not cover the re-keying, because the CLI serves a single request per process. It only shows up over the built-in server, where omitting the re-encode aborts on the second request.
Testing was done with GDB: force a chunk into the cache, overwrite its link with 0x4141414141414141, then force a pop. Before, the corrupted pointer was accepted silently and became heap->cached_chunks. After, it aborts with "zend_mm_heap corrupted".