Better handling of X-Forwarded-For header to get client ip address

[ashtuchkin]

Currently, if the proxy is trusted, express uses first (leftmost) ip address in X-Forwarded-For header as the client ip address. This value cannot be trusted (easily spoofed by clients) and it's usage is not recommended. Please see discussion about this issue applied to Ruby:

http://blog.gingerlime.com/2012/rails-ip-spoofing-vulnerabilities-and-protection/

In a scenario where we have more than one proxy we can trust, the rightmost IP address will be of one of those proxies. The leftmost IP however cannot be blindly trusted. The recommended procedure is therefore to follow the chain, from the rightmost-element to the left, ignoring IP addresses of the proxies we trust. Once we hit the first unknown IP addresses when traversing from right to left, we use this as our client’s IP.

This seems like the best-practice, and implemented both with Apache mod_remoteip and nginx realip module.

Quote from Apache:

When multiple, comma delimited useragent IP addresses are listed in the header value, they are processed in Right-to-Left order. Processing halts when a given useragent IP address is not trusted to present the preceding IP address.

I'd suggest the following algorithm:

1. Filter out all non-ip entries in X-Forwarded-For array.
2. Add a setting with a name like "trust proxy levels" and if it's set to N, return N-th rightmost ip in the array (skip N proxy levels).
3. Else, remove all private ip addresses from the end of array (they are probably trusted) and return the rightmost (last) element as the client ip.

Please also see Ruby pull request that solves this rails/rails#7980

I could write a pull request to fix it, please let me know if you're interested.

[dougwilson]

If I pulled something like https://github.com/dougwilson/node-connect-x-forwarded-for into core, would that work for you?

[dougwilson]

Filter out all non-ip entries in X-Forwarded-For array.

why?

Add a setting with a name like "trust proxy levels" and if it's set to N, return N-th rightmost ip in the array (skip N proxy levels).

Terrible idea, because if a service is accessible directly and through a proxy, this just leads to spoofing anyway; you need to specify the IP addresses or netmasks of your border proxies, not the level.

Else, remove all private ip addresses from the end of array (they are probably trusted) and return the rightmost (last) element as the client ip.

This could be the default, but with the added requirement that the TCP connection is also from a private IP.

[ashtuchkin]

Yes, something like that should work fine. It would need to work with netmasks, not only trusted ip-s.

why?

Defensive programming? I, as a developer, would like to assume that client ip would actually be ip, not be some arbitrary string given in headers, even in case of misconfiguration.

Terrible idea, because if a service is accessible directly and through a proxy, this just leads to spoofing anyway; you need to specify the IP addresses or netmasks of your border proxies, not the level.

While I agree that ip/netmasks are another solution, the proxy level has an advantage that it's easier to manage for the major use case of a single proxy (it sure can determine automatically if there's no proxy at all and handle it correctly). It could be the default if 'trust proxy' setting is on. Just an idea, I don't insist.

This could be the default, but with the added requirement that the TCP connection is also from a private IP.

👍

[dougwilson]

advantage that it's easier to manage for the major use case of a single proxy

But a single proxy would also be very simple just giving the single IP address or netmask of it, no?

[ashtuchkin]

You would need to keep this ip address ​somewhere in case it will change,
and remember to update it in different environments. No such problems with
a proxy level = 1, just hardcode it. It could even be made the default (if
'trust proxy' is set), then people would not need to know about it at all.

Architectural changes like the number of proxy levels are arguably rarer
than environment changes.

That being said, I see ip/netmask solution more obvious and clear, which is
good in the long run.

Alexander Shtuchkin

On Sun, May 4, 2014 at 3:44 PM, Douglas Christopher Wilson <
[email protected]> wrote:

advantage that it's easier to manage for the major use case of a single
proxy

But a single proxy would also be very simple just giving the single IP
address or netmask of it, no?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2099#issuecomment-42148645
.

[dougwilson]

You would need to keep this ip address ​somewhere in case it will change, and remember to update it in different environments

People probably have to do this already; I know I deploy the same app with different settings per environment already; my main reason against this is that if we are going to make these changes so it is more secure, why introduce some new setting that is not at all secure with it? Seems to go against the whole reasoning for the changes proposed here.

It could even be made the default

Unfortunately to introduce these changes in 4.x, the default will have to be the current behavior (i.e. use the left-most address).

In most reverse-proxy environments (all?), your proxies on the front end are actually completely re-writing the X-Forwarded-For header, so it is typically completely trusted, unless your app is running in a dual-environment where it is accessible directly as well as through the proxy.

[ashtuchkin]

And one more thing, both 'proxy level' and ip/netmask settings could work more like an extension of current setting 'trust proxy':

Value	Meaning
undefined / false / 0	no proxy
true / 1	single proxy
2	2 proxies
"1.2.3.4/24"	ip & netmask of trusted proxies.

[ashtuchkin]

People probably have to do this already.

Another config setting, no big deal, I agree.

why introduce some new setting that is not at all secure with it?

If the default level is 1, then I don't see the case where it's not secure. It's secure if you have single proxy or no proxy. It will return the IP of the second proxy if there is more than one proxy level, in which case people would understand that something is wrong. It also more secure than current state and doesn't require any additional input from developer, unlike the ip/netmask approach.

Unfortunately to introduce these changes in 4.x, the default will have to be the current behavior (i.e. use the left-most address).

Do you plan for 5.x? :)

In most reverse-proxy environments (all?), your proxies on the front end are actually completely re-writing the X-Forwarded-For header, so it is typically completely trusted, unless your app is running in a dual-environment where it is accessible directly as well as through the proxy.

My experience is different. For example, in Nginx, almost all manuals advise to add directive proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;, which appends address, not rewrites it.

[ashtuchkin]

Well, you're right about the dual environment, there it's not safe. Hmm.

[dougwilson]

Do you plan for 5.x? :)

Yea, I would look to do it whenever it becomes possible :)

For example, in Nginx

Ah, looks like nginx pretty much forces you to use append-only.

[ashtuchkin]

Ok, so I suggest making it an extension to 'trust proxy' setting:

Value	Current behavior	Proposed behavior
undefined / false	direct ip	direct ip
true	leftmost ip	rightmost non-private ip
"1.2.3.0/24,1.2.4.0/24"	--	rightmost ip not within the netmasks given

Where the list of ips is concatenation of X-Forwarded-For header and direct ip.

[dougwilson]

Using the table is kind of cool. This is what I am planning to add to 4.x:

Value	Behavior
undefined/false	socket ip
true	leftmost ip (backwards-compatible)
"localhost"	use ["::1/128","127.0.0.0/8"]
"local"	use ["fe80::/10","::1/128","127.0.0.0/8","169.254.0.0/16"]
"private"	use ["fe80::/10","::1/128","10.0.0.0/8","127.0.0.0/8","169.254.0.0/16","172.16.0.0/12","192.168.0.0/16"]
["1.2.3.0/24","1.2.4.0/24"]	first ip (from right to left) outside ranges; the socket ip is implicitly added at the right-most ip

Edit: added localhost, private, and changed local to mean link-local.

[ashtuchkin]

Thats very nice!

My subjective comments:

1. I'm not sure we need all the options here (localhost, local, private) - it's more burden to think & choose IMHO. Maybe just "local"? It'll be easier to understand for users and, in 5.x, easier make it just 'true'.
2. Strings are usually easier to store in configs that arrays :) Everybody will have to do .split(',').

Btw, I researched current state of the business and it seems that Apache has the same non-safe handling of X-Forwarded-For header

Be careful when using these headers on the origin server, since they will contain more than one (comma-separated) value if the original request already contained one of these headers.

And HAProxy setting forwardfor also appends ip, not replaces it.

To solve this problem, the well-known HTTP header
"X-Forwarded-For" may be added by HAProxy to all requests sent to the server.
This header contains a value representing the client's IP address. Since this
header is always appended at the end of the existing header list, the server
must be configured to always use the last occurrence of this header only.

(All conforming servers will join several headers of the same name to a comma-separated list, so this is equal to appending as I understand)

So, it most proxy servers are actually appending the ip to our header, making current setup unsafe by default.

[dougwilson]

(All conforming servers will join several headers of the same name to a comma-separated list, so this is equal to appending as I understand)

That is correct; by default HTTP defines that multiple instances of the same header can be concatenated together with a comma in the order from first to last.

Strings are usually easier to store in configs that arrays :) Everybody will have to do .split(',').

I know, but then we really can't accept any "nice names" at all--users will just always need to specify a list of IPs; doing something special by sniffing the string would just be a bad API. It also depends on what people are using for config storage; some things have an array type.

I'm not sure we need all the options here (localhost, local, private)

I can see people using both "private" and "localhost" easily in those options; "localhost" is useful if you are just terminating SSL on the same machine (and thus need the SSL terminator to forwards along the remote IP). "private" would be the more general use-case.

In some clouds (specifically Stackato) the proxies only communicate over link-local IPs to the app. Since the apps live in a "dual" environment where you can talk to them through the proxy and directly, "local" is a perfect easy choice for that environment.

[ashtuchkin]

I know, but then we really can't accept any "nice names" at all--users will just always need to specify a list of IPs; doing something special by sniffing the string would just be a bad API.

Thats a matter of taste I believe. These nice names are just shortcuts, much like predefined formats in logger middleware. You don't need to 'sniff', you just need to compare it with predefined string(s) and if nothing matched - use it as a comma-separated list of ip/netmasks. On a pro side, this would allow users to move this setting to config and change painlessly from "local" to "1.2.3.4/10" and back in different environments without type mangling. Plus, it will just look nicer in the major use case of a single ip/netmask.

I can see people using both "private" and "localhost" easily in those options

I just wanted to suggest choosing a single option that'll fit 90% of users that know nothing about the differences and don't want to choose. The other 10% will be happy to use generic netmasks to do exactly what they need.
Keeping several similar predefined options would require more documentation, explanation of differences and giving advice for where to use which option, plus more time & brain power from users.

"localhost" is the easiest to explain and will fit most users IMHO. If they need something else, they'll know it for sure and it's obvious how to implement.

Anyway, I think both issues are not that important and I'll be happy with any decision from your side. I really appreciate your efforts!

[dougwilson]

See https://github.com/expressjs/proxy-addr (initial release was made to just mimic express).

[dougwilson]

@ashtuchkin this is about to land. This is mostly the same as the tables I put above, but in addition will accept a number for a hop count and comma-separated strings. The pre-defined choices will be there, but simplified from earlier, with the most common choice being "loopback".

[ashtuchkin]

This is awesome, thank you!