httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: Broken URI-unescaping in mod_proxy
Date Sun, 09 Sep 2007 20:00:25 GMT


On 09/09/2007 04:30 PM, Nick Kew wrote:
> 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?

ProxyPass /a http://backend/
ProxyPass /b http://backend/


<Proxy http://backend/a>
       allow from someip
       deny from all
</Proxy>

<Proxy http://backend/b>
       allow from someotherip
       deny from all
</Proxy>

Request:

GET /a/%2E%2E/b/somewhere
GET /a/../b/somewhere

This allows someip to access http://backend/b/somewhere with the patch.
It does not without because r->uri would be /b/somewhere.


> 
>> 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.

ProxyPass /a http://backend/

would match

GET /a/somewhere
GET /b/../a/somewhere
GET /%91/somewhere

without the patch. With the patch it only matches the first case.


> 
>>  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.

See above.

> 
> 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.
> 

Agreed.

Regards

RĂ¼diger


Mime
View raw message