Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: domain of trawick@gmail.com designates
 209.85.215.173 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20111216161757.GA12113@redhat.com>
References: <20111123142321.GB22547@redhat.com>
	<4ECEC72E.1000508@kippdata.de>
	<20111128143822.GB27242@redhat.com>
	<CAKUrXK4uwT=P1KtEziNqFdxXs+tyWvggzpL8x2u-Bbq8tZ-Zsw@mail.gmail.com>
	<20111216161757.GA12113@redhat.com>
Date: Fri, 16 Dec 2011 15:17:33 -0500
Message-ID: 
 <CAKUrXK4oZSOBwFZB-ot_CRSDpdCX=68hNS6nx7oNtXhLi9RoJw@mail.gmail.com>
Subject: Re: [RFC] further proxy/rewrite URL validation security issue
 (CVE-2011-4317)
From: Jeff Trawick <trawick@gmail.com>
To: dev@httpd.apache.org
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 16, 2011 at 11:17 AM, Joe Orton <jorton@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 critica=
l:
>>
>> 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 thoug=
ht

> 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). =A0It allows cases where some weird
> module can provide a translate_name hook for non-HTTP URIs ("urn:fish")
> which genuinely lack a path segment. =A0This 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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- server/protocol.c =A0 (revision 1215187)
> +++ server/protocol.c =A0 (working copy)
> @@ -640,25 +640,6 @@
>
> =A0 =A0 ap_parse_uri(r, uri);
>
> - =A0 =A0/* RFC 2616:
> - =A0 =A0 * =A0 Request-URI =A0 =A0=3D "*" | absoluteURI | abs_path | aut=
hority
> - =A0 =A0 *
> - =A0 =A0 * authority is a special case for CONNECT. =A0If the request is=
 not
> - =A0 =A0 * using CONNECT, and the parsed URI does not have scheme, and
> - =A0 =A0 * it does not begin with '/', and it is not '*', then, fail
> - =A0 =A0 * and give a 400 response. */
> - =A0 =A0if (r->method_number !=3D M_CONNECT
> - =A0 =A0 =A0 =A0&& !r->parsed_uri.scheme
> - =A0 =A0 =A0 =A0&& uri[0] !=3D '/'
> - =A0 =A0 =A0 =A0&& !(uri[0] =3D=3D '*' && uri[1] =3D=3D '\0')) {
> - =A0 =A0 =A0 =A0ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"invalid request-URI %s", ur=
i);
> - =A0 =A0 =A0 =A0r->args =3D NULL;
> - =A0 =A0 =A0 =A0r->hostname =3D NULL;
> - =A0 =A0 =A0 =A0r->status =3D HTTP_BAD_REQUEST;
> - =A0 =A0 =A0 =A0r->uri =3D apr_pstrdup(r->pool, uri);
> - =A0 =A0}
> -
> =A0 =A0 if (ll[0]) {
> =A0 =A0 =A0 =A0 r->assbackwards =3D 0;
> =A0 =A0 =A0 =A0 pro =3D ll;
> Index: modules/mappers/mod_rewrite.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- modules/mappers/mod_rewrite.c =A0 =A0 =A0 (revision 1215187)
> +++ modules/mappers/mod_rewrite.c =A0 =A0 =A0 (working copy)
> @@ -4266,6 +4266,10 @@
> =A0 =A0 =A0 =A0 return DECLINED;
> =A0 =A0 }
>
> + =A0 =A0if (strcmp(r->unparsed_uri, "*") =3D=3D 0 || !r->uri || r->uri[0=
] !=3D '/') {
> + =A0 =A0 =A0 =A0return DECLINED;
> + =A0 =A0}
> +
> =A0 =A0 /*
> =A0 =A0 =A0* =A0add the SCRIPT_URL variable to the env. this is a bit com=
plicated
> =A0 =A0 =A0* =A0due to the fact that apache uses subrequests and internal=
 redirects
> Index: modules/proxy/mod_proxy.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- modules/proxy/mod_proxy.c =A0 (revision 1215187)
> +++ modules/proxy/mod_proxy.c =A0 (working copy)
> @@ -566,6 +566,10 @@
> =A0 =A0 =A0 =A0 return OK;
> =A0 =A0 }
>
> + =A0 =A0if (strcmp(r->unparsed_uri, "*") =3D=3D 0 || !r->uri || r->uri[0=
] !=3D '/') {
> + =A0 =A0 =A0 =A0return DECLINED;
> + =A0 =A0}
> +
> =A0 =A0 /* XXX: since r->uri has been manipulated already we're not reall=
y
> =A0 =A0 =A0* compliant with RFC1945 at this point. =A0But this probably i=
sn't
> =A0 =A0 =A0* 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
...