IP address calculated from X-Forwarded-For are insecure

[danielcompton]

The way that IP addresses are inferred from X-Forwarded-For is unsafe/incorrect here. You cannot trust any IP addresses in the headers except for the ones added by your proxies.

The code for this section links to:
http://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html#x-forwarded-for

At the bottom of that line it says:

If a request from a client already contains an X-Forwarded-For header, Elastic Load Balancing appends the IP address of the client at the end of the header value. In this case, the last IP address in the list is the IP address of the client. For example, the following header contains two IP addresses added by the client, which might not be trustworthy, plus the client IP address added by Elastic Load Balancing:

X-Forwarded-For: ip-address-1, ip-address-2, client-ip-address

The correct way to detect this is to start at the end of the header and work backwards based on how many proxies you know are between your app and the internet. This should be configured by the user, or default to the last IP. For example, if you are running with a single ELB proxy, then you need to take the last value (which is added by ELB), not the first in the list.

[austince]

Would you accept a PR for this? What are the situations where the reverse format applies?

[pbojinov]

@austince I will gladly accept a PR for this

[rannmann]

This is also true of CF-Connecting-IP. Cloudflare sends the true client's IP in this header, but also appends it to X-Forwarded-For.

In general, the first IP is supposed to be the correct one, but in cases where third parties get involved (meaning LBs, CDNs and the like) the real IP is actually at the end.

That said, it might be wise to read CF-Connecting-IP before X-Forwarded-For, but I'm not sure if that will break anything. :(

[rudza]

@pbojinov You are gladly accepting PR for 18 months now!

[stuckj]

This is also incorrect for Google Cloud Run (or any Google Cloud service behind their load balancers). Docs are here: https://cloud.google.com/load-balancing/docs/https

Works basically the same as AWS. I think all the major cloud providers do this.

[YSaxon]

TLDR: a pull request was merged to fix this but then reverted, the library remains insecure

To anyone who comes across this, what appears to have happened is that a PR was eventually merged that changed the behavior to taking the rightmost (last) comma delimited IP instead of the leftmost (first).

#51 (PR)

This was good for security but incorrect as per spec, since if a request passes through multiple chained load balancers / proxies and each one appends the previous one's IP, then the rightmost entry contains the IP of the last proxy, and it's actually an entry to the left of this that contains the clientIP (the leftmost if no spoofing). The pull request seemed to misunderstand this and think that the clientIP always ends up at the end: it doesn't, it's just that only the end is always[*] safe from spoofing.

X-Forwarded-For: [spoofed, spoofed, ..., spoofed,] clientIP, [proxy, proxy, ..., proxy]

To put it another way, spoofing inserts some arbitrary number of fake entries to the left of the actual IP, and chained proxies insert a number of useless entries to the right of the actual IP. The true clientIP could be anywhere in the list, and there is no algorithm that can be locate it robustly without any further information. The only hope is to know the number of proxies you expect to see on the right side (or the IP ranges of all of them) and keep moving to the left until you find the one true clientIP. Note that in the most common case[*] of only a single proxy, this reduces to taking the rightmost entry, but the point is that this is not a universal truth.

Accordingly just taking the rightmost entry broke some people's applications, who complained that it was against spec, and the pull request was reverted.

#60 (complaints)
#61 (reverting PR)

And so that is how things stand today, back to where we started from, with a parsing routine that is "correct" (against arbitrary numbers of proxies) but insecure (against spoofing).

[*]: Actually, in the arguably even more common case of zero proxies (ie the client spoke to the server directly, or at least, through no proxies of the type that set this header), nothing in that header will be correct, and anything in that header (including the rightmost entry) can only have been spoofed. But that's more the scope of this issue: #26

[YSaxon]

Given how difficult an issue this is, I suggest just being upfront in the documentation that this library is not and cannot be secure against spoofing, and should not be relied upon for security purposes.

I would also mention that if you must parse the IP for security purposes, you need to either:

• only rely on the TCP Connection IP (if there aren't any load balancers in the way)
• if there are load balancers, first determine that the request passed through them, determine exactly how many, and parse the ip out manually from the correct entry (counting from the right) of the correct header. For most simple AWS ELB setups this will reduce to just taking eg the rightmost entry from the X-Forwarded-For header.