The background is that we use mod_headers to set request headers like this : RequestHeader set X-MS-Unique-Id %{UNIQUE_ID}e env=!HAVE_X-MS-Unique-Id [ the env=! part does not matter -- it does it even for statically configured request headers ] This works fine when Apache directly serves a request first time. It breaks badly when Apache uses a subrequest during its processing. This is because mod_headers sets the header (in r->headers_in) during the subrequest, using the request pool from that subrequest. Unfortunately, Apache passes subrequests a reference to r->headers_in, not a copy. So at the end of the subrequest when the sub-request's pool is destroyed, you have invalid data on the parent request's headers_in table. The core apache code responsible for this is ap_set_sub_req_protocol() in server/protocol.c : /* did the original request have a body? (e.g. POST w/SSI tags) * if so, make sure the subrequest doesn't inherit body headers */ if (apr_table_get(r->headers_in, "Content-Length") || apr_table_get(r->headers_in, "Transfer-Encoding")) { clone_headers_no_body(rnew, r); } else { /* no body (common case). clone headers the cheap way */ rnew->headers_in = r->headers_in; } Unfortunately, the 'cheap way' allows subrequests to modify the parent request's data structures. We fixed the problem by changing this to : else { /* no body (common case). clone headers the cheap way */ /* rnew->headers_in = r->headers_in; */ rnew->headers_in = apr_table_copy(rnew->pool, r->headers_in); } There is a performance penalty with this. One other solution would be to modify mod_headers to set the table value using the table's own pool rather than its request pool, but that doesn't prevent subrequests can having potentially strange side-effects. This bug has potentially serious security consequences as the main request ends up referencing memory it shouldn't be doing. We have seen it give away security sensitive data to the client, and would like to see this patched and a security fix made available as soon as possible please.
With reference to trunk, but I'm sure this applies also to 2.2 ... A look at the code in question indicates that the use of r->pool causing the issue is in function process_tags(), and could be fixed by a simple patch: --- modules/metadata/mod_headers.c (revision 888304) +++ modules/metadata/mod_headers.c (working copy) @@ -551,6 +551,10 @@ format_tag *tag = (format_tag*) hdr->ta->elts; + while (r->main != NULL) { + r = r->main; + } + for (i = 0; i < hdr->ta->nelts; i++) { s = tag[i].func(r, tag[i].arg); if (str == NULL) Does this make sense, or am I missing something? Haven't checked the implications of passing the parent to tags[i].func, FWIW.
Committed a fix to trunk as r889408.
The fix does make sense, but I think that this means that the behavior of the Request headers is different to that of the Response headers w.r.t. sub-requests. Core Apache makes a copy of the outgoing headers when calling a sub-request -- so any changes that mod_headers make are discarded at the end of the subrequest and don't effect the rest of the processing. Request headers are passed to a sub-request by reference, so a change by mod_headers in a sub-request persists into the rest of the processing. If this is by design then fine, but I think that needs to be documented. I tend to think that making a copy of the request headers as is done for the response headers is more consistent. Thanks Jake Scott
I agree with Jake; letting the subrequest manipulate the input headers is probably not the way to go; I'd suggest reverting the trunk patch and applying his proposed solution. Copying the input header array seems like a better patch than the solution in trunk, and more consistant with user and module author expectations.
(In reply to comment #4) > I agree with Jake; letting the subrequest manipulate the input headers is > probably not the way to go; I'd suggest reverting the trunk patch and applying > his proposed solution. > > Copying the input header array seems like a better patch than the solution > in trunk, and more consistant with user and module author expectations. Is this a veto? If yes I would simply revert. If not as I am short of time currently feel free to revert r889408 and implement Jakes proposal.
It's a RFC - if your comment represents a +1, I'm happy to revert and commit a patch based on his proposal - I was looking for a sanity check from the other committers who had reviewed the original fix.
(In reply to comment #6) > It's a RFC - if your comment represents a +1, I'm happy to revert and commit > a patch based on his proposal - I was looking for a sanity check from the > other committers who had reviewed the original fix. Yes, this a +1.
No, Nick. No modules relied upon this, because any module which relied upon this behavior would have crashed.
Will, don't technical discussions belong on-list? Why would a module crash, except in the special case (of this bug) where it sets something allocated on r->pool in the headers table?
Unfortunately, the list doesn't have this thread, in any case bugzilla is fine for accepting or rejecting patches with a rational. You've rejected this because, as you state, modules might rely on percolating their changes up a level. If these are pool allocated, as you point out, the upstream modules crash. If these are static or an 'unset' operation, they would not crash. But the subrequest is distinct and should *not* pollute the top level request. Thanks for making me spend more time considering this, so I'm quite certain that Jake's observations were correct.
(In reply to comment #10) > Unfortunately, the list doesn't have this thread, in any case bugzilla is fine > for accepting or rejecting patches with a rational. > > You've rejected this because, as you state, modules might rely on percolating > their changes up a level. If these are pool allocated, as you point out, the > upstream modules crash. If these are static or an 'unset' operation, they > would not crash. > > But the subrequest is distinct and should *not* pollute the top level request. Not good enough. We have an API that supports this, and a contract with the world not to break that API in the lifetime of 2.2. "Should not" is no part of that. We haven't told module developers not to manipulate headers in a subrequest, nor even to check whether they're in a subrequest (e.g. something running from mod_includes). Nor have we forbidden modules to reverse-engineer other modules observed to have side-effects such as manipulating headers. > Thanks for making me spend more time considering this, so I'm quite certain > that Jake's observations were correct. Yes, they're correct. And indeed they're *observations* of how things are, with a balanced suggestion that we either change it (fine for 2.4) or document it. No quarrel with that.
Nick, this makes no sense. The subrequest should not violate the parent request, and the patch in trunk is evidence that the module's behavior was bugged. Bugs are fixed on stable branches. By all means, we preserve behavior, but not to the extreme of an API contract on all incidental misbehavior. IIF the subrequest-handling module wishes to violate the parent request, there is an API for that; walk the request parent chain backwards and modify that request rec closely observing that specific request pool. I'll patch trunk and propose for backport.
Nick; your objection is further flawed by the fact that all requests-with-bodies already received a shallow copy of headers_in. A patch is committed as r901578, please review and reconsider your vote. Many thanks.
Note the change is r901578+r901589 per rpluem's observation.
Created attachment 24896 [details] Backport of 2nd revision, to 2.2 Patch as proposed for 2.2 backport in STATUS
Note finally, the API you claim exists, Nick, has been broken since; ------- Revision 158798 - (view) (download) (annotate) - [select for diffs] Modified Wed Mar 23 16:36:45 2005 UTC (4 years, 10 months ago) by gregames File length: 52751 byte(s) Diff to previous 151408 (colored) don't propagate input headers describing a body to a subrequest. this can cause a back end server to hang in a read for a body which no longer exists. ------- for all requests-with-bodies, which shipped as 2.1.5, although this change was apparently never propagated to 2.0 (committed just before 2.0.54). The proposed patch breaks the 'contract' of permitting the user to skip the presumably mandatory step of walking r->main backwards to modify r->main only for the remaining bodiless-requests. For such requests, the existing code has always allowed subrequest header_in changes to become invalid, which in the case of non-prefork MPM's may later consist of another request's data (which the administrator is free to then reflect with mod_headers).
The old code seems to have arbitrarily chosen to sometimes copy the input headers, and sometimes not, entirely arbitrarily from the point of view of a module. From what I can see, any module that depended on being able to manipulate main request headers by fiddling with subrequest headers is not only broken but likely to suffer inconsistent behaviour depending on the request. +1 to wrowe's patch.
This was committed to 2.2.x in r917867. Do you have a simple configuration which can be used to reliably reproduce the bug? I'm trying to make a test case for the test suite. It looks like this will affect 2.0.x too.
Note that this incident has been assigned CVE-2010-0434 (cve.mitre.org)