httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <>
Subject Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c
Date Tue, 19 Jul 2005 18:15:40 GMT
At 10:07 AM 7/19/2005, Joe Orton wrote:
>On Thu, Jul 14, 2005 at 07:43:35AM -0400, Jeff Trawick wrote:
>> I'm so confused while trying to draw the line between
>> alternate RFC-compliant philosophy

Roy spelled it out, it's not in the RFC but if there is -any- way
we can use C-L, let's do it.  The current 2.0.x code doesn't actually 
do that because it doesn't check to see if we can read all of the
body within our hardcoded limit, before electing a C-L or T-E method.

Everything else represents violations.

>> fixes for actual RFC violations

The currently applied backport patch 171205 by Jeff...

  still believes cl_zero, even with T-E.
  still drops the body altogether for empty bodies in the
    stream_chunked and spool_cl cases 
    (an empty body is still a body!)
  still looks for a C-L value before looking for T-E 
  ignored the fact that *any* T-E overrides C-L
  introduced proxy-sendunchangedcl, something that an administrator
    (as opposed to a developer) is unlikely to be certain of, and 
    then never tested that the C-L values don't mismatch.
  provided no protection against proto_input_filters changing the 
    body length

In other words, send_request_body is altogether bogus.  The problem
is exacerbated by the fact that send_request_body is out-of-line with
the rest of the header handling, making this harder code to review
than a single inline function to evaluate request headers+bodies.

>> fixes for security issues

Each RFC violation between proxy and origin server (or next hop)
becomes a cache poisioning/splitting/spoofing opportunity, I think
anyone who's read Watchfire groks that by now.

>> I think CHANGES should be crystal clear on what change has a security
>> implication.
>I am also confused and still trying to catch up and understand these 
>Bill, can you please describe *exactly* what security issues you see in 
>the 2.0 proxy after the two already-committed patches (r219061).

E.g. the code remains bogus, as it once was, only with more
flavors of mishandling the body.

>CAN-2005-2088 MUST NOT be used to refer to anything other than specific 
>issue described in the WatchFire report.  When you start bandying this 
>CVE reference around with each new patch it defeats the purpose of 
>having a CVE reference in the first place.  If there are wider issues in 
>the proxy then they will need new CVE names assigned.

This patch addresses the fact that 2.0.x today HANGS when passed 
a C-L and T-E.  The 'protocol.c' patch previously committed does
mitigate the problem.  But Watchfire explicitly called out the
mishandling of C-L + T-E headers and the fact that the resulting
request body in some cases would have no request headers whatsoever.

The code in 2.0.x branch is ---still--- invalid, because the C-L
and T-E selection is invalid.  The symptoms are trivial; the proxy
never processes the body correctly because the wrong request
headers are ---still--- passed to the origin server.  And that is
exactly what Watchfire discussed.

I suggested Jeff's approach is *GOOD*, backport the correct body
handling code in its entirety.  Unfortunately, at that point in 
time, request body handling was still broken.

Since the intent was to backport, yet the elections and edge cases 
were not thought through, I'm vetoing and that specific backport.
It leaves me with a question;

is it better to revert [and then, re-credit Jeff in that CHANGES 
entry if/when we have an approved backport to reapply?] or better
to patch over to the current proxy_http body handling code?  I don't
know which will be more legible, later.

We enforce rules up front now with the protocol.c patch, masking
how problematic that mod_proxy_http.c still is;  always revert
before you test the newest mod_proxy_http patches.  Every other 
module which diddles the headers_in can re-trigger these edge cases, 
and anyone using the proxy_http.c as an example of anything will be 
led astray.  Yet, even with this wondrous sipmnle patch, the existing 
proxy_http code in 2.0.x trunk backported from 2.1.x still has issues.

In order to test, I'd applied; 
which provides trivial echo of the body and its headers, and then
used to set up
a pretty wide range of tests from 1.3, 2.0, 2.1 against 1.3, 2.0
and 2.1 back-end servers.  You don't have to do this, of course,
but TraceEnable extended is simpler than sniffing sometimes.

And you can see from using netcat to pipe;
through httpd-2.0 how badly this is all, still, mishandled, both
according to Roy's points r.e. C-L election, and sans the protocol
patch (which doesn't apply to module-mauled C-L and T-E headers).

Some examples;

** Using 2.0.54 proxy

HEAD /20/ HTTP/1.1
Host: localhost
Content-Length: 75
Transfer-Encoding: chunked

Test This!


** HANGS, because this is forwarded...

Host: localhost:8020
Content-Length: 75
Max-Forwards: 10
Via: 1.1 localhost:8054
X-Forwarded-Host: localhost
X-Forwarded-Server: localhost

Test This!

** what 75 characters above?  I don't count 75 [violation]

** so repeating the test with svn 2.0.x trunk code;

Host: localhost:8020
Max-Forwards: 10
Via: 1.1 localhost:8055
X-Forwarded-Host: localhost
X-Forwarded-Server: localhost
Transfer-Encoding: chunked

Test This!


** is a much better forward; however you note that this
** tiny body remains chunked, contrary to Roy's comments.

** Now lets look at empty T-E bodies...

HEAD /20/ HTTP/1.1
Host: localhost
Transfer-Encoding: chunked


** httpd-2.0.54 forwards no body headers, no body [violation]
** httpd-2.0.x branch forwards no body headers, no body [violation]

In other words; the first backport was a backport of a broken
rewrite of mod_proxy_http.c - and the current patch simply brings
it back into alignment.  Heaven forbid you have a module running
around setting or unsetting C-L and/or T-E.  The http_input core
filter already has made it's elections, and we are trusting the
headers_in array to be honest about what ap_get_brigade will do?
That, sir, is madness!


View raw message