On Fri, Dec 16, 2011 at 11:17 AM, Joe Orton <jor...@redhat.com> wrote:
> Sorry, I missed this earlier.
>
> On Mon, Dec 12, 2011 at 01:24:51PM -0500, Jeff Trawick wrote:
>> The new code and the core translate name hook agree on something critical:
>>
>> if it isn't "*" and it isn't a fully qualified path, return 400.
>>
>> For proxy and rewrite to return 400 without knowing if these were
>> proxied or rewritten requests implies that it is never valid (as
>> returning 400 from those hooks will bypass other hooks that might be
>> able to handle that).
>>
>> One of the following should be true:
>>
>> a) (if always invalid) core should check the condition before running
>> translate name
>> b) (if not always invalid) proxy and rewrite should decline (like
>> alias) instead of returning 400, in case there is still another hook
>> that runs before core and needs to handle it
>>
>> Make sense?
>
> I can't fault your logic, which is more astute than I've managed to be
> with my bungled hacks to date...
just one of those cases where having code to look at triggers further thought
> The protocol.c change, r1179239, effectively presumed case (a), but
> failed because it wasn't sufficient to actually enforce the "what is a
> valid URI" condition.
>
> The translate_name hook changes, r1209436, effectively presumed case
> (b), but do not follow your reasonable conclusion on DECLINE vs 400.
>
> So probably we should go with (b). It allows cases where some weird
> module can provide a translate_name hook for non-HTTP URIs ("urn:fish")
> which genuinely lack a path segment. This means we can back out the
> hack from protocol.c and leave the translate_name hooks to DTRT.
>
> What do you think?
>
> Index: server/protocol.c
> ===================================================================
> --- server/protocol.c (revision 1215187)
> +++ server/protocol.c (working copy)
> @@ -640,25 +640,6 @@
>
> ap_parse_uri(r, uri);
>
> - /* RFC 2616:
> - * Request-URI = "*" | absoluteURI | abs_path | authority
> - *
> - * authority is a special case for CONNECT. If the request is not
> - * using CONNECT, and the parsed URI does not have scheme, and
> - * it does not begin with '/', and it is not '*', then, fail
> - * and give a 400 response. */
> - if (r->method_number != M_CONNECT
> - && !r->parsed_uri.scheme
> - && uri[0] != '/'
> - && !(uri[0] == '*' && uri[1] == '\0')) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> - "invalid request-URI %s", uri);
> - r->args = NULL;
> - r->hostname = NULL;
> - r->status = HTTP_BAD_REQUEST;
> - r->uri = apr_pstrdup(r->pool, uri);
> - }
> -
> if (ll[0]) {
> r->assbackwards = 0;
> pro = ll;
> Index: modules/mappers/mod_rewrite.c
> ===================================================================
> --- modules/mappers/mod_rewrite.c (revision 1215187)
> +++ modules/mappers/mod_rewrite.c (working copy)
> @@ -4266,6 +4266,10 @@
> return DECLINED;
> }
>
> + if (strcmp(r->unparsed_uri, "*") == 0 || !r->uri || r->uri[0] != '/') {
> + return DECLINED;
> + }
> +
> /*
> * add the SCRIPT_URL variable to the env. this is a bit complicated
> * due to the fact that apache uses subrequests and internal redirects
> Index: modules/proxy/mod_proxy.c
> ===================================================================
> --- modules/proxy/mod_proxy.c (revision 1215187)
> +++ modules/proxy/mod_proxy.c (working copy)
> @@ -566,6 +566,10 @@
> return OK;
> }
>
> + if (strcmp(r->unparsed_uri, "*") == 0 || !r->uri || r->uri[0] != '/') {
> + return DECLINED;
> + }
> +
> /* XXX: since r->uri has been manipulated already we're not really
> * compliant with RFC1945 at this point. But this probably isn't
> * an issue because this is a hybrid proxy/origin server.
That looks right to me.
--/--
Are these the testcases for this?
for protocol in (0.9, 1.x) { # rpluem found a 0.9-specific
issue fixed in 1188745
for config_mechanism in (RewriteRule, ProxyPassMatch) {
for exploit in (CVE-2011-3368, CVE-2011-4317-B, CVE-2011-4317-B) {
expect 400
...
