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
>changes...
>
>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
http://people.apache.org/~wrowe/httpd-2.0-cl-te-protocol.patch
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;
http://people.apache.org/~wrowe/httpd-2.0-trace.patch
which provides trivial echo of the body and its headers, and then
used http://people.apache.org/~wrowe/httpd.proxy.conf 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;
http://people.apache.org/~wrowe/chunked20.req
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
c
Test This!
0
** HANGS, because this is forwarded...
HEAD / HTTP/1.1
Host: localhost:8020
Content-Length: 75
Max-Forwards: 10
Via: 1.1 localhost:8054
X-Forwarded-For: 127.0.0.1
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;
HEAD / HTTP/1.1
Host: localhost:8020
Max-Forwards: 10
Via: 1.1 localhost:8055
X-Forwarded-For: 127.0.0.1
X-Forwarded-Host: localhost
X-Forwarded-Server: localhost
Transfer-Encoding: chunked
c
Test This!
0
** 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
0
** 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!
Bill
