On May 18, 2009, at 2:19 PM, Nick Kew wrote:
On Mon, 18 May 2009 12:23:38 -0700
"Roy T. Fielding" <[email protected]> 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
