Protect the cached chunk list against corruption - #23367
Conversation
|
Part of #14083 |
|
I think that we should at least swap the byte order in zend_mm_encode_cached_chunk(), otherwise a small buffer overwrite is likely to result in a valid chunk address despite the xor. Ideally we should also use a shadow to verify the result of the decode function. Maybe store the shadow at chunk + 64 - sizeof(void*) so it's in the same cache line. |
Yeah, good point about the |
|
I fear that by using the shadow key in this context, but with less checks than in the freelist, we are weakening it. I would prefer if this used a shadow key + xor with the holder here as in #23376. We can add a NB: Test failures look related |
Empty chunks are not always unmapped. zend_mm_delete_chunk() may retain
them in heap->cached_chunks so they can be reused without another mmap().
The list is linked through chunk headers that stay mapped and writable, so
an overwrite of a link controls the value that zend_mm_alloc_pages()
removes from the cache and hands to zend_mm_chunk_init(), which writes
through it and links it into the active chunk list.
Protect the list with the same key material as the small allocation
freelists. chunk->next keeps the plain pointer and the new
chunk->next_shadow holds an encoded copy:
next_shadow = BSWAPPTR(next) ^ heap->shadow_key ^ &chunk->next_shadow
The byte swap makes a small overwrite corrupt the most significant bytes
of the address, which is unlikely to yield another valid chunk. Mixing in
the address of next_shadow prevents a valid (next, next_shadow) pair from
being replayed into another chunk. The shadow is an integrity check, not a
secret; the secret remains heap->shadow_key.
Reading a cached link decodes the shadow, checks that the result is
chunk-aligned and that it matches chunk->next, and only then dereferences
it. The head of the list is stored in the heap rather than in a chunk
header, so it gets an alignment check of its own when it is popped.
Cached chunks outlive request resets and forks, so their shadows are
recomputed by zend_mm_rekey_cached_chunks() whenever zend_mm_refresh_key()
or zend_mm_refresh_key_child() changes the key. That walk validates every
link against its old shadow, so corruption is detected rather than
silently re-encoded.
The next_shadow field is carved out of the chunk header's reserve field,
so the header is still 64 bytes, and chunk->next remains the ordinary
doubly-linked-list pointer while the chunk is active.
8510490 to
56ede4d
Compare
|
Something like this? |
|
Looks great! |
|
Thank you! |
2 participants
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".