You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@gmail.com> on 2011/12/12 19:24:51 UTC
Re: [RFC] further proxy/rewrite URL validation security issue (CVE-2011-4317)
On Mon, Nov 28, 2011 at 9:38 AM, Joe Orton <jo...@redhat.com> wrote:
> On Thu, Nov 24, 2011 at 11:37:34PM +0100, Rainer Jung wrote:
>> Don't know whether that could happen here, but could "OPTIONS *" be
>> a problem?
>
> Hmmm, another good question.
>
> What should mod_rewrite or mod_proxy's translate_name hook do for a
> request-URI of "*"? 2616 says:
>
> The asterisk "*" means that the request does not apply to a
> particular resource, but to the server itself
>
> ... so I would say they should return DECLINED for "*"? It makes no
> sense to apply rewrite rules against "*", nor can it be proxied.
>
> Index: modules/mappers/mod_rewrite.c
> ===================================================================
> --- modules/mappers/mod_rewrite.c (revision 1203669)
> +++ modules/mappers/mod_rewrite.c (working copy)
> @@ -4266,6 +4266,18 @@
> return DECLINED;
> }
>
> + if (strcmp(r->unparsed_uri, "*") == 0) {
> + /* Don't apply rewrite rules to "*". */
> + return DECLINED;
> + }
> +
> + /* Check that the URI is valid. */
> + if (!r->uri || r->uri[0] != '/') {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> + "Invalid URI in request %s", r->the_request);
> + return HTTP_BAD_REQUEST;
> + }
> +
> /*
> * 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 1203669)
> +++ modules/proxy/mod_proxy.c (working copy)
> @@ -566,6 +566,18 @@
> return OK;
> }
>
> + if (strcmp(r->unparsed_uri, "*") == 0) {
> + /* "*" cannot be proxied. */
> + return DECLINED;
> + }
> +
> + /* Check that the URI is valid. */
> + if (!r->uri || r->uri[0] != '/') {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> + "Invalid URI in request %s", r->the_request);
> + return HTTP_BAD_REQUEST;
> + }
> +
> /* 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.
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?
Re: [RFC] further proxy/rewrite URL validation security issue (CVE-2011-4317)
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Dec 16, 2011 at 11:17 AM, Joe Orton <jo...@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
...
Re: [RFC] further proxy/rewrite URL validation security issue
(CVE-2011-4317)
Posted by Joe Orton <jo...@redhat.com>.
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.