You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Roy T. Fielding" <fi...@gbiv.com> on 2009/05/18 18:47:47 UTC
Re: 204/304 follow-up
On May 17, 2009, at 12:54 PM, Edward Z. Yang wrote:
> Hello Roy,
>
> Two months ago a patch was committed to Apache which prevented Apache
> from sending content body when forbidden by the HTTP protocol, namely
> 204 and 304 response. You vetoed this patch, and said that the patch
> applied to the incorrect code and should be put somewhere else.
>
> I was curious to know as to where you thought the patch would be more
> appropriately applied.
Where the 204/304 response is generated. I have not seen any examples
where that isn't already being done. The core server code is in
http_protocol.c:ap_send_error_response()
/* We need to special-case the handling of 204 and 304 responses,
* since they have specific HTTP requirements and do not include a
* message body. Note that being assbackwards here is not an
option.
*/
if (status == HTTP_NOT_MODIFIED) {
ap_finalize_request_protocol(r);
return;
}
if (status == HTTP_NO_CONTENT) {
ap_finalize_request_protocol(r);
return;
}
Something similar is in the proxy.
If you know of a specific place where we are sending a body with
one of those codes, then please point it out.
....Roy
Re: 204/304 follow-up
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On May 18, 2009, at 2:19 PM, Nick Kew wrote:
> On Mon, 18 May 2009 12:23:38 -0700
> "Roy T. Fielding" <fi...@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
Re: 204/304 follow-up
Posted by Nick Kew <ni...@webthing.com>.
On Mon, 18 May 2009 12:23:38 -0700
"Roy T. Fielding" <fi...@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?
--
Nick Kew
Re: 204/304 follow-up
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
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
Re: 204/304 follow-up
Posted by Nick Kew <ni...@webthing.com>.
On Mon, 18 May 2009 09:47:47 -0700
"Roy T. Fielding" <fi...@gbiv.com> wrote:
> > I was curious to know as to where you thought the patch would be
> > more appropriately applied.
>
> Where the 204/304 response is generated. I have not seen any examples
> where that isn't already being done. The core server code is in
>
> http_protocol.c:ap_send_error_response()
which is for errors generated within the server.
> Something similar is in the proxy.
>
> If you know of a specific place where we are sending a body with
> one of those codes, then please point it out.
The case under discussion was errors generated by a script and
propagated by the server without reference to ap_send_error_response.
Hence the fix was in util_script.c where it reacts to HTTP
status codes.
That is, assuming the reference is to
http://marc.info/?t=123844627000001&r=1&w=2
--
Nick Kew