Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c

["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

2.0.55 Release

[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

["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  

Re: svn commit: r218978 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c

["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