Bug 48359

Summary: Buffer overflow related to setting RequestHeader
Product: Apache httpd-2 Reporter: Philip Pickett <phil.pickett>
Component: CoreAssignee: Apache HTTPD Bugs Mailing List <bugs>
Status: RESOLVED FIXED    
Severity: major CC: jacob.scott
Priority: P2 Keywords: FixedInTrunk, PatchAvailable
Version: 2.2-HEAD   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: Backport of 2nd revision, to 2.2

Description Philip Pickett 2009-12-09 07:21:41 UTC
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.
Comment 1 Nick Kew 2009-12-09 09:46:49 UTC
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.
Comment 2 Ruediger Pluem 2009-12-10 12:19:14 UTC
Committed a fix to trunk as r889408.
Comment 3 Jake Scott 2009-12-15 03:02:45 UTC
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
Comment 4 William A. Rowe Jr. 2010-01-19 12:17:53 UTC
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.
Comment 5 Ruediger Pluem 2010-01-19 23:30:35 UTC
(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.
Comment 6 William A. Rowe Jr. 2010-01-19 23:36:08 UTC
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.
Comment 7 Ruediger Pluem 2010-01-20 02:47:50 UTC
(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.
Comment 8 William A. Rowe Jr. 2010-01-20 14:06:54 UTC
No, Nick.  No modules relied upon this, because any module which relied upon this
behavior would have crashed.
Comment 9 Nick Kew 2010-01-20 15:33:24 UTC
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?
Comment 10 William A. Rowe Jr. 2010-01-20 16:32:57 UTC
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.
Comment 11 Nick Kew 2010-01-20 18:03:15 UTC
(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.
Comment 12 William A. Rowe Jr. 2010-01-20 20:44:13 UTC
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.
Comment 13 William A. Rowe Jr. 2010-01-20 23:21:10 UTC
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.
Comment 14 William A. Rowe Jr. 2010-01-21 10:17:27 UTC
Note the change is r901578+r901589 per rpluem's observation.
Comment 15 William A. Rowe Jr. 2010-01-27 10:54:01 UTC
Created attachment 24896 [details]
Backport of 2nd revision, to 2.2

Patch as proposed for 2.2 backport in STATUS
Comment 16 William A. Rowe Jr. 2010-02-04 15:41:47 UTC
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).
Comment 17 Graham Leggett 2010-03-01 01:44:37 UTC
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.
Comment 18 Joe Orton 2010-03-04 14:38:39 UTC
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.
Comment 19 William A. Rowe Jr. 2010-03-06 00:13:42 UTC
Note that this incident has been assigned CVE-2010-0434 (cve.mitre.org)