httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <field...@gbiv.com>
Subject Re: 204/304 follow-up
Date Mon, 18 May 2009 23:44:34 GMT
On May 18, 2009, at 2:19 PM, Nick Kew wrote:

> On Mon, 18 May 2009 12:23:38 -0700
> "Roy T. Fielding" <fielding@gbiv.com> wrote:
>
>> On May 18, 2009, at 11:53 AM, Nick Kew wrote:
>>> The case under discussion was errors generated by a script and
>>> propagated by the server without reference to
>>> ap_send_error_response.
>>
>> Fix the script.
>>
>> ....Roy
>
> So, to take just one example, we should've ignored CVE-2008-2939
> and fixed the backend instead?

Does this issue allow remote injection of nefarious data?  No.

It might cause weird response-replacement errors on pipelined
responses from the same server, but only if the installed
script is intentionally brain-dead.  If such a script is installed,
then sending a body on 204/304 is the least of your problems.

In any case, I vetoed the patch because it changed the request
in order to trigger a side-effect that mimics a fix, not because
the actual error wasn't worth fixing if you have a test case.

The code in ap_scan_script_header_err_core() returns both HTTP
status codes (errors only) and server module status OK.  Then
both mod_cgi.c and mod_cgid.c do crazy stuff:

     /* Handle script return... */
     if (!nph) {
         const char *location;
         char sbuf[MAX_STRING_LEN];
         int ret;

         bb = apr_brigade_create(r->pool, c->bucket_alloc);
         b = apr_bucket_pipe_create(tempsock, c->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(bb, b);
         b = apr_bucket_eos_create(c->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(bb, b);

         if ((ret = ap_scan_script_header_err_brigade(r, bb, sbuf))) {
             ret = log_script(r, conf, ret, dbuf, sbuf, bb, NULL);

             /*
              * ret could be HTTP_NOT_MODIFIED in the case that the  
CGI script
              * does not set an explicit status and  
ap_meets_conditions, which
              * is called by ap_scan_script_header_err_brigade,  
detects that
              * the conditions of the requests are met and the  
response is
              * not modified.
              * In this case set r->status and return OK in order to  
prevent
              * running through the error processing stack as this would
              * break with mod_cache, if the conditions had been set by
              * mod_cache itself to validate a stale entity.
              * BTW: We circumvent the error processing stack anyway  
if the
              * CGI script set an explicit status code (whatever it  
is) and
              * the only possible values for ret here are:
              *
              * HTTP_NOT_MODIFIED          (set by ap_meets_conditions)
              * HTTP_PRECONDITION_FAILED   (set by ap_meets_conditions)
              * HTTP_INTERNAL_SERVER_ERROR (if something went wrong  
during the
              * processing of the response of the CGI script, e.g  
broken headers
              * or a crashed CGI process).
              */
             if (ret == HTTP_NOT_MODIFIED) {
                 r->status = ret;
                 return OK;
             }

             return ret;
         }

Which for some unknown reason makes up for the details of how
ap_scan_script_header_err_core() fails to set r->status for 304
and then blows it away, while returning other error conditions
to ap_process_async_request via invoke_handler, which eventually
leads to a canned error via ap_die().  Quite frankly, that code
sucks, but it isn't the cause of this issue.

The mod_cgi code isn't even checking for a 204/304 set via the
Status: line. Those codes are simply stuck in r->status and
returned OK.  The body is sent on down the line when mod_cgi
calls ap_pass_brigade().

So, if you still want to fix this bug, we have a choice: either
check r->status after the above code block in mod_cgi(d).c, with
something like (untested)

     if (r->status == HTTP_NO_CONTENT ||
         r->status == HTTP_NOT_MODIFIED) {
             discard_script_output(bb);
             apr_brigade_destroy(bb);
             return r->status;
     }

or figure out why the HTTP output filter is not smart enough to
discard the body in the brigade before sending it on the wire.

....Roy

Mime
View raw message