Usage of $proxy_add_x_forwarded_for on edge proxies

[Maxim Dounin]

Hello!

On Wed, Jan 13, 2021 at 11:39:13PM +0900, nanaya wrote:

> On Wed, Jan 13, 2021, at 22:53, Maxim Dounin wrote:
> > It's not "dangerous config", it's incorrect usage of 
> > X-Forwarded-For which might be dengerous.  In the most simply 
> > configuration with a single server the X-Forwarded-For header 
> > comes directly from the client, without anything added by nginx - 
> > and this has exactly the same implications.
> > 
> 
> Unfortunately, at least in rails, it's actually dangerous passing the value as is:
> 
> https://github.com/rails/rails/blob/3f4fde4d9f804140be8304b524792c052e3d1024/actionpack/lib/action_dispatch/middleware/remote_ip.rb#L21
> 
> At least they have added a bunch of check to make it less 
> dangerous even when using $proxy_add_x_forwarded_for 
> (essentially works just like $remote_addr in default config).

This code seems to imply that the code is running behind a trusted 
proxy, and both X-Forwarded-For and Client-IP headers are actually 
updated by the proxy.  And it uses a list of trusted proxies it is 
going to trust to switch to a previous address, so I would say the 
code is safe when used properly.  And it's fine with 
$proxy_add_x_forwarded_for, too.

Another question is how often it is used properly.  Given it 
requires update of two headers, at least one of them being very 
rare, I would assume the answer is "almost never".  But again, it 
has nothing to do with $proxy_add_x_forwarded_for.  Rather, 
properly using "proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;" 
will make attacks impossible.

> > While X-Forwarded-For is often misused by applications and 
> > incorrect configurations by blindly trusting addresses in it, 
> > removing the header is going to make destroy the information 
> > available for well-written applications.  While you it might be a 
> > good idea to remove the header in your particular use case - if 
> > you are sure enough your applications doesn't use it - this is 
> > certainly not how things should be configured by default.
> > 
> 
> Yeah, I'm not going to trust X-Forwarded-For sent by client. 
> Maybe it's just me. $remote_addr to me is their geolocation. 
> Anything more "sophisticated" just looked like a potential of 
> failure.

Again: there is a difference between trusting X-Forwarded-For sent 
by client and using it in the cases where trust isn't that 
important.  Certainly no one should blindly trust X-Forwarded-For.  
But anyone can use it properly, and stripping it as you suggest is 
just wrong in most cases.

> And I don't want to have to worry if my $random_app parses the 
> X-Forwarded-For sanely. At most I'd just log it at the edge 
> server.

That's your choice: instead of fixing vulnerabilities in the apps 
you prefer to drop headers which might be used to exploit these 
vulnerabilities, dropping also many legitimate uses of the headers.

> Look at this wonderful function by wordpress (thankfully they do 
> aware it's "unsafe"):
> 
> https://github.com/WordPress/WordPress/blob/c5d1214607be128c99dd27589a58cc5a1d20d522/wp-admin/includes/class-wp-community-events.php#L262

That's exactly what I'm talking about: as long as you don't need 
trusted address, X-Forwarded-For can be used to extract some 
valuable information.

Further, note that even with X-Forwarded-For set to $remote_addr, 
client still can provide arbitrary address to the code via the 
Client-IP header.  So, if the code in question is somehow used to 
extract an addresses where trusted address is actually needed, 
your using $remote_addr instead of $proxy_add_x_forwarded_for 
won't save you from being vulnerable.

> Semi unrelated but I can't find this list of IPs used by Opera 
> Mini proxies. Do you know where I can find it?

A good list of trusted proxies is maintained by Wikimedia, see 
here:

https://meta.wikimedia.org/wiki/XFF_project#Trusted_XFF_list

-- 
Maxim Dounin
http://mdounin.ru/