httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Kew <n...@webthing.com>
Subject Re: Broken URI-unescaping in mod_proxy
Date Sun, 09 Sep 2007 14:30:57 GMT
On Sun, 09 Sep 2007 11:25:26 +0200
Ruediger Pluem <rpluem@apache.org> wrote:

> 
> 
> On 09/09/2007 02:21 AM, Nick Kew wrote:
> > PR 41798 and many related ones (eg 39746, 38980 - both of which I've
> > closed today) show a history of incorrect URL-unescaping in
> > mod_proxy.
> > 
> > For PR41798, the attached patch looks like a fix: it just uses
> > r->unparsed_uri (escaped) instead of r->uri (unescaped) in
> > proxy_trans.  I'm wondering if using unparsed_uri here risks
> > breaking something or has security implications we need to
> > consider, bearing in mind we already unescaped it and thus
> > verified it is well-formed.
> 
> I think it has security implications, because
> 
> 1. We do the proxy_walk *after* proxy_trans and the normal
> configuration expects that all the shrinking transformations happened
> correctly.

proxy_trans determines whether the request is to be ProxyPassed: if not,
the patch has no effect on the request.  The "filename" we just parsed
is not used locally, it's passed to the backend.  To pass it escaped is
indeed an RFC bug.

As an additional safeguard, we already checked the incoming URL was
well-formed when we parsed r->uri.

> 2. It can be used to circumvent ProxyPass / ProxyPassmatch settings by
>    tricky encodings in order to sent requests to unintended locations.

How so?

> Furthermore it makes it really hard for the administrator to configure
> things as he has to consider all kind of encoding stuff when setting
> up his rules for reverse proxying.

Not sure what existing rules could fail with the change: it's only
backends that require escaped chars (especially slashes) that'll
be affected.  But we can work around that by making it a configuration
option.

>  And: This patch doesn't work with
> mod_rewrite.

The mod_rewrite patch fixes that.

> BTW: IMHO it is not needed to set r->uri to local_uri in your patch
> as proxy handler only deals with r->filename.

Yes, but if we revise the patch (eg to make it a configuration option)
then we might want to do something more interesting with it.

> So as a summary, yes we have some problems with the current approach
> and some things are broken, but using the unparsed uri opens a can of
> worms IMHO.

Not sure.  I'd like some example of the kind of problem it might open.

Bottom line: the old approach is buggy and needs fixing.  If this fix
is too problematic, we need another, and I need the insight to fix it.

-- 
Nick Kew

Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/

Mime
View raw message