You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@gmail.com> on 2005/07/14 13:43:35 UTC
Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c
On 7/13/05, wrowe@apache.org <wr...@apache.org> wrote:
> How can I fix thee? let me count the ways...
>
> * pass a chunked body always (no-body requests don't go chunked).
We tried to send C-L whenever practical because it is common for a
origin server not to be able to handle chunked. It looks like the
default configuration will be broken for some folks with input
filters, such as origin server = Apache mod_cgi, or origin server =
IIS (somebody else's comments). So previously we optimized for
fool-proof operation instead of efficiency, and the administrator
could tune for efficiency if they could determine that there was no
issue with the back-end server handling chunked.
> * validate that the C-L counted body length doesn't change.
It looks like we are counting bytes after they go through input
filters here, and then comparing that byte count with the specified
C-L header field. That doesn't have to match since filters can change
the size. (Admittedly I'm probably misunderstanding something.)
> * follow RFC 2616 for C-L / T-E in the request body C-L / T-E
> election logic.
Can you be more specific about what exactly had to be done for RFC
2616 conformance?
> * do not forward HTTP/1.0 requests as HTTP/1.1, unless the admin
> configures force-proxy-request-1.1
What is the harm? The potential value is that the administrator can
tell us to use chunked iin the case where there is an input filter and
the origin server can handle chunked.
http://svn.apache.org/viewcvs/httpd/httpd/trunk/CHANGES?rev=218978&r1=218977&r2=218978&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES (original)
> +++ httpd/httpd/trunk/CHANGES Wed Jul 13 20:42:22 2005
> @@ -1,12 +1,19 @@
> Changes with Apache 2.1.7
> [Remove entries to the current 2.0 section below, when backported]
>
> + *) SECURITY: CAN-2005-2088
> + proxy: Correctly handle the Transfer-Encoding and Content-Length
> + headers, discard the request Content-Length whenever T-E: chunked
> + is used, always passing one of either C-L or T-E: chunked whenever
> + the request includes a request body, and no longer upgrade HTTP/1.0
> + requests to the origin server as HTTP/1.1. Resolves an entire class
> + of proxy HTTP Request Splitting/Spoofing attacks. [William Rowe]
I'm so confused while trying to draw the line between
alternate RFC-compliant philosophy
fixes for actual RFC violations
fixes for security issues
I think CHANGES should be crystal clear on what change has a security
implication.
Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
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
Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c
Posted by Joe Orton <jo...@redhat.com>.
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
> fixes for actual RFC violations
> fixes for security issues
>
> 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).
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.
Regards,
joe
Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 06:34 AM 7/15/2005, Roy T. Fielding wrote:
>>All of it, except for the preference to RB_STREAM_CHUNKED when,
>>perhaps, we could be more sub-optimal, falling back on RB_SPOOL_CL.
>>
>>Many RB_STREAM_CL choices, before, were equally dangerous, and that
>>C-L == length_read test in the stream_reqbody_chunked() was meant
>>to exclude future abuse.
>
>We should be sending C-L if the brigade includes the entire message.
>CL is always preferred for requests. The only time we should send
>T-E on a proxied request is if we cannot buffer enough of the
>received request to know how large it will be.
++1 and committed! The solution was blindingly obvious, try to
fetch MAX_MEM_SPOOL bytes, and determine if we hit EOS before
proceeding to elect -any- C-L or T-E method :)
The logic works out like this once it exceeds MAX_MEM_SPOOL;
was it T-E?
Unless the user overrides by setting proxy-sendcl or by
setting force-proxy-downgrade-1.0, keep using T-E.
proxy-sendchunks causes proxy-sendcl to be ignored for this case.
was it C-L?
Only stream (for unfiltered requests) or spool as C-L, unless
the user overrides with proxy-sendchunks (probably to specific
applications or servers that they are certain will handle
chunked request bodies effectively.)
proxy-sendcl causes proxy-sendchunks to be ignored for this case.
Question for folks familiar with the input stack; should we loop
until MAX_MEM_SPOOL is hit, or is one call to the filter (as my
patch has done) enough to get about MAX_MEM_SPOOL bytes?
>> * No longer upgrade HTTP/1.0 requests to the origin server as
>> HTTP/1.1 unless the force-proxy-request-1.1 envvar is set.
>> [William Rowe]
>
>-1 (veto), since it is a clear violation of
> http://www.ietf.org/rfc/rfc2145.txt
>and the intent of HTTP versions.
Thanks! Reverted.
Bill
2.0.55 Release
Posted by Sascha Kersken <sk...@lingoworld.de>.
Hi,
I am currently finishing the companion CD-ROM for my Beginners' Guide To
MySQL (for O'Reilly Germany). So I am wondering whether the announced
release 2.0.55 will be available within the next few days or whether I
should just use 2.0.54. Does anybody know an approximate release date yet?
Regards
Sascha
P.S.: Of course, I would test-drive release candidates when available and
report building problems.
Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
> * RFC 2616 says
>
> "All HTTP/1.1 applications MUST be able to receive and decode the
> "chunked" transfer-coding, and MUST ignore chunk-extension
> extensions
> they do not understand."
>
> I read this as "an HTTP/1.1 server must accept "chunked" or quit
> reporting it complies with the HTTP/1.1 specification".
No, you should read it as an HTTP/1.1 server MUST be able to receive
and decode the "chunked" transfer-coding, because that's what I wrote.
In other words, it must be able to parse the message.
> All of it, except for the preference to RB_STREAM_CHUNKED when,
> perhaps, we could be more sub-optimal, falling back on RB_SPOOL_CL.
>
> Many RB_STREAM_CL choices, before, were equally dangerous, and that
> C-L == length_read test in the stream_reqbody_chunked() was meant
> to exclude future abuse.
We should be sending C-L if the brigade includes the entire message.
CL is always preferred for requests. The only time we should send
T-E on a proxied request is if we cannot buffer enough of the
received request to know how large it will be.
> * No longer upgrade HTTP/1.0 requests to the origin server as
> HTTP/1.1 unless the force-proxy-request-1.1 envvar is set.
> [William Rowe]
-1 (veto), since it is a clear violation of
http://www.ietf.org/rfc/rfc2145.txt
and the intent of HTTP versions.
....Roy
Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 06:43 AM 7/14/2005, Jeff Trawick wrote:
>On 7/13/05, wrowe@apache.org <wr...@apache.org> wrote:
>
>> How can I fix thee? let me count the ways...
>>
>> * pass a chunked body always (no-body requests don't go chunked).
>
>We tried to send C-L whenever practical because it is common for a
>origin server not to be able to handle chunked. It looks like the
>default configuration will be broken for some folks with input
>filters, such as origin server = Apache mod_cgi, or origin server =
>IIS (somebody else's comments).
Good points... however...
>So previously we optimized for
>fool-proof operation instead of efficiency, and the administrator
>could tune for efficiency if they could determine that there was no
>issue with the back-end server handling chunked.
Well, not fool proof, here were the faults with the old logic;
* "c-l: 0" can still have a request body, the RB_SPOOL_CL is
now used in all but the 'r->input_filt == r->proto_input_filt'
case, for paranoia that the given cl can mismatch the input
length read from the filter chain. Adding a test against the
value reinforces our faith that the request isn't split.
* I'd agree to drop the RB_STREAM_CHUNKED from the old_cl_val test
path, and always fall back on RB_SPOOL_CL instead. Costly but,
as you point out, not designed for efficiency. And manditory
to change for httpd-2.0, I agree.
* In the old_te_val case, I'd also agree to probe the server first
with an OPTIONS to determine if it is an HTTP/1.1 server before
we choose RB_SPOOL_CL. Connection persistence makes this more
cost-effective if we cache the result for the specific backend,
at least for the lifetime of this request. But for the normal
scenario, the origin server will return a 411 Length Required,
and the client will retry falling into the old_cl_val case.
* RFC 2616 says
"All HTTP/1.1 applications MUST be able to receive and decode the
"chunked" transfer-coding, and MUST ignore chunk-extension extensions
they do not understand."
I read this as "an HTTP/1.1 server must accept "chunked" or quit
reporting it complies with the HTTP/1.1 specification".
However, I agree this might be insufficient. We simply will have
to be prepared for the request to be rejected. I'm thinking that
an initial probe of the origin server would work, especially if we
can then keep a cache of backend server capabilities.
We might even send the headers and an Expect: 100-continue, wait
for our continue, and send the chunked body. If that fails, or
sends nothing after 2 sec or whatever, send as C-L request instead.
We still have the headers and haven't consumed (burned) the client
request body.
That thought is something to contemplate for 2.1 not 2.0.
>> * validate that the C-L counted body length doesn't change.
>
>It looks like we are counting bytes after they go through input
>filters here, and then comparing that byte count with the specified
>C-L header field. That doesn't have to match since filters can change
>the size. (Admittedly I'm probably misunderstanding something.)
Yes! We only hit that STREAM_CL choice if there are no body filters,
only protocol filters. We just reported to the origin server that
the C-L was X - and now we are sending the server Y. This is the
very definition of response splitting, this time at the hands of
an httpd filter.
We will simply kill the request if this happens. That's why I'd
killed the 'please prefer STREAM_CL' flag and moved the C-L "0"
case over to RB_SPOOL_CL. The flag was beyond the administrators
knowledge (and the developer who recommends it can't be certain
the admin hasn't added another filter), while the C-L "0" case
is just as efficient passing through the spool_reqbody_cl path.
>> * follow RFC 2616 for C-L / T-E in the request body C-L / T-E
>> election logic.
>
>Can you be more specific about what exactly had to be done for RFC
>2616 conformance?
If T-E is given, ignore C-L. The old code didn't do so for the
repeated request. This is exactly what Watchfire observed.
Watchfire also observed that all body headers would disappear
entirely under certain scenarios, leaving the body to appear to
the origin server as a new request.
>> * do not forward HTTP/1.0 requests as HTTP/1.1, unless the admin
>> configures force-proxy-request-1.1
>
>What is the harm? The potential value is that the administrator can
>tell us to use chunked iin the case where there is an input filter and
>the origin server can handle chunked.
Because header results may change, rendering headers instead that
the client does not understand.
>> + *) SECURITY: CAN-2005-2088
>> + proxy: Correctly handle the Transfer-Encoding and Content-Length
>> + headers, discard the request Content-Length whenever T-E: chunked
>> + is used, always passing one of either C-L or T-E: chunked whenever
>> + the request includes a request body, and no longer upgrade HTTP/1.0
>> + requests to the origin server as HTTP/1.1. Resolves an entire class
>> + of proxy HTTP Request Splitting/Spoofing attacks. [William Rowe]
>
>I'm so confused while trying to draw the line between
>alternate RFC-compliant philosophy
>fixes for actual RFC violations
>fixes for security issues
There's no distinction. The security issue *is* RFC non-compliance
in handling C-L and T-E headers. The RB_SPOOL/STREAM_CL/CHUNKED
choose-one logic violated that principal. The only reason you no
longer see it is the change to protocol.c - back out that patch
(which doesn't affect internal actions by modules or filters) and
you can observe exactly what I mean. Additional errors in not
sending empty bodies, etc, compounded the issue.
>I think CHANGES should be crystal clear on what change has a security
>implication.
All of it, except for the preference to RB_STREAM_CHUNKED when,
perhaps, we could be more sub-optimal, falling back on RB_SPOOL_CL.
Many RB_STREAM_CL choices, before, were equally dangerous, and that
C-L == length_read test in the stream_reqbody_chunked() was meant
to exclude future abuse.
So how is this;
* proxy: Correctly handle the Transfer-Encoding and Content-Length
request body headers. Discards the request Content-Length whenever
Transfer-Encoding: chunked is used; ensures that any request which
includes a body (even zero length) passes on that request body to
the origin server with either the T-E: chunked -or- C-L: header as
appropriate. Resolves an entire class of proxy HTTP Request
Splitting / Spoofing attacks. [William Rowe]
* No longer upgrade HTTP/1.0 requests to the origin server as
HTTP/1.1 unless the force-proxy-request-1.1 envvar is set.
[William Rowe]
Bill