fix: heap-use-after-free in php_brotli_decompress_read#86
Conversation
The read handler emalloc'd the input buffer on every call and efree'd it at the end, but self->ctx.next_in kept pointing into it across calls. On the next entry, when the decoder did not request more input, BrotliDecoderDecompressStream re-read the freed buffer. ASAN detects this via tests/streams_003.phpt through the copy() path. Move the input buffer onto php_brotli_stream_data, allocate it lazily on first read, and free it in php_brotli_decompress_close. Also drop the now-pointless emalloc() NULL checks since Zend MM bails out on allocation failure. Reported and patched by @ndossche.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors the Brotli decompression stream to lazily allocate and persist an input buffer in the stream state, reuse it across read calls when the decoder requests more input, and free it on stream close. Adds a PHPT that exercises oversized-chunk decompression. ChangesDecompression stream buffer optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@ndossche Thanks again for the careful report and the patch. For the record: I reproduced the UAF locally on Your patch is applied here as-is; after the fix, ASAN runs cleanly through all 48 tests with no regressions. Credit is in the commit message body. On your meta question: a regular public PR is fine on this repo for memory-safety reports going forward — keeping the fix open helps downstream packagers pick it up. Of course, feel free to reach out privately first if you're ever unsure about a specific case. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@brotli.c`:
- Around line 904-906: The read into self->input_buf uses `count` which can
exceed the buffer allocation; change the read to limit its size to the buffer
(use `PHP_BROTLI_BUFFER_SIZE` or compute `min(count, PHP_BROTLI_BUFFER_SIZE)`)
so `self->ctx.available_in = php_stream_read(self->stream, self->input_buf,
<limited_size>);` and keep `self->ctx.next_in = self->input_buf;` unchanged;
ensure any loop that consumes `self->ctx.available_in` accounts for partial
fills when `count` > `PHP_BROTLI_BUFFER_SIZE`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The inner php_stream_read on the NEEDS_MORE_INPUT path wrote up to the caller-requested count bytes into self->input_buf, which is allocated with PHP_BROTLI_BUFFER_SIZE (1<<19). When the stream chunk size exceeds PHP_BROTLI_BUFFER_SIZE this overflows the heap buffer. Cap the read at PHP_BROTLI_BUFFER_SIZE, matching the outer read at brotli.c:867, so the maximum write into input_buf is uniform over its lifetime. Regression test tests/streams_007.phpt: with stream_set_chunk_size set to 1 MiB, read an uncompressible random_bytes payload back through the brotli stream so the inner-loop read is exercised. Reported-by: @ndossche
1 participant
The read handler emalloc'd the input buffer on every call and efree'd it at the end, but self->ctx.next_in kept pointing into it across calls. On the next entry, when the decoder did not request more input, BrotliDecoderDecompressStream re-read the freed buffer. ASAN detects this via tests/streams_003.phpt through the copy() path.
Move the input buffer onto php_brotli_stream_data, allocate it lazily on first read, and free it in php_brotli_decompress_close. Also drop the now-pointless emalloc() NULL checks since Zend MM bails out on allocation failure.
Reported and patched by @ndossche.
Summary by CodeRabbit