Repository navigation
mod_http2: fix use-after-free of response and trailer bucket data - #846
Closed
arshsmith1 wants to merge 1 commit into
Closed
arshsmith1 wants to merge 1 commit into
arshsmith1 wants to merge 1 commit into
Conversation
stream_do_response() and stream_do_trailers() read the RESPONSE/HEADERS bucket's data after apr_bucket_destroy() has freed it, and keep the response pointer in stream->response for the life of the stream. Copy the data into the stream pool before destroying the bucket, as h2_c2_filter.c already does for the headers it consumes.
Collaborator
|
Duplicate of #708 - let's discuss there. |
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.
In the
AP_HAS_RESPONSE_BUCKETSbuild,stream_do_response()andstream_do_trailers()inmodules/http2/h2_stream.ckeep a pointer into a RESPONSE/HEADERS metadata bucket (resp = b->data) and then callapr_bucket_destroy(b)before using it. That struct is allocated from the bucket allocator and freed by the bucket destructor (apr_bucket_free()once the shared refcount reaches 0, seeserver/headers_bucket.c), so the following reads ofresp->status,resp->notes,resp->headers(andheaders->headersfor trailers) read freed memory.stream_do_response()also stores the freed pointer instream->response, which is dereferenced later fromstream_data_cb()(stream->response->status).The bucket arrives in
out_bufferas a fresh clone produced byh2_beam_receive()(ap_bucket_response_clone) with refcount 1, so the destroy really frees it; the session keeps allocating buckets from the same per-connection allocator, the freed slot gets handed back, and the response is corrupted.The struct contents (headers/notes/reason) already live in
stream->pool, so copying the small struct there withapr_pmemdup()before the destroy keeps it valid for the life of the stream. This mirrors the clone-before-destroy patternh2_c2_filter.calready uses for the headers it consumes. The non-AP_HAS_RESPONSE_BUCKETSbuild is unaffected, since thererespis the pool-allocatedh2_headers.I could not turn this into a crash reproducible under a supported configuration (the freed slot is frequently not reused within the window), so it is offered as a correctness fix rather than a reported vulnerability. I checked the lifetime with a small program driving the real APR bucket allocator: after
apr_bucket_destroy()the freedap_bucket_response-shaped slot is returned by the next same-sizeapr_bucket_alloc()and overwritten, so the retained pointer reads corrupted data; with theapr_pmemdup()copy the value survives.(see
SECURITY.mdandhttps://www.apache.org/security/#reporting-a-vulnerability).
I was unable to reproduce a crash under a supported configuration, so
this is submitted as a memory-safety correctness fix; please redirect it
if you would rather handle it privately.
APLOGNO()tag;numbers are assigned by a committer at merge time (see
docs/log-message-tags/README).(No new log messages in this change.)
changes-entries/*.txtfile is included,following the template in
README.CHANGES(not needed otherwise).test/modules/xxxdirectory matching themodules/xxxmodule source,or
test/modules/corefor core server changes. On Unix, verify thesepass locally with e.g.
make check-pytest PYTEST_DIRS=test/modules/xxx.A deterministic pyhttpd test is impractical for this latent use-after-free
(it only manifests when the freed slot is reused), so I verified it with a
standalone reproducer against the real APR bucket allocator instead.