httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: [RFC] further proxy/rewrite URL validation security issue (CVE-2011-4317)
Date Fri, 16 Dec 2011 16:17:57 GMT
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...

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.

Mime
View raw message