Fix heap-use-after-free in HTTP keep-alive connection handling #235
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.
Summary
This patch fixes a heap-use-after-free in
Connection::handleWriteResponse()that occurs when handling HTTP keep-alive connections, particularly when the client disconnects or times out after sending a request.In
src/http/Connection.C, after sending an HTTP response on a keep-alive connection, the code removes all receive buffers except the last one:The bug: The pointer
rcv_remaining_may still point to data in a deleted buffer. The original condition only checks ifrcv_remaining_is less than the buffer end, but does not verify that it points within the bounds of the remaining buffer (i.e.,>= buffer_start).Trigger Scenario
This issue can be reliably reproduced when:
handleWriteResponse()is called to clean up for next keep-alive requestrcv_remaining_points to a deleted buffer → heap-use-after-free inRequestParser::consume()ASAN Report
Full ASAN trace shows the memory was freed by
pop_front()in the same thread just before the access.Solution
Add a proper bounds check to verify that
rcv_remaining_points inside the remaining buffer.Testing
Tested with AFL++ fuzzing harness (raw TCP packets fuzzing) using AddressSanitizer:
Test case that triggered the bug:
Note, that Content-Length exceeds the real body length (33), so server waits fot more data from. Instead, client closes socket after 500ms, what leads to triggering the bug.