On 09/09/2007 04:30 PM, Nick Kew wrote: > On Sun, 09 Sep 2007 11:25:26 +0200 > Ruediger Pluem <[EMAIL PROTECTED]> 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
