Yii2: Improve getUserIP

[SamMousa]

I'm aware of the current trustedHosts functionality, this is about something else.
I remember an issue / PR related to this bug but I could not find it.

The issue arises when your application is behind multiple levels of proxies.

What steps will reproduce the problem?

Configure your application behind (multiple) trusted proxies, with trustedHosts configured to trust LB-BACK. For simplicity assume all 3 hosts are publicly reachable by anyone.

Internet --> LB-FRONT --> LB-BACK --> APP

Example headers received when doing a curl request:

curl -H "X-Forwarded-For: SPOOFED-IP" http://mysite"
GET / HTTP/1.1
Host: mysite
User-Agent: curl/7.55.1
Accept: */*
X-Forwarded-For: SPOOFED-IP
X-Forwarded-For: REAL-CLIENT-IP
X-Forwarded-For: LB-FRONT

What is the expected result?

The expectation for the current implementation would be to get LB-FRONT as the users' IP.

What do you get instead?

Currently, if I trust LB-FRONT, I will get userIP() === SPOOFED-IP, since the first header is taken.

Fix the bug

Instead of taking the first X-Forwarded-For header we should default to taking the last, this will make it secure by default. (https://en.wikipedia.org/wiki/X-Forwarded-For)

Improve the resolution

Optionally we should allow getUserIP to skip trusted hosts. The assumption here is that any trusted host is not the IP we are interested in.
Note that this is only secure if each host only accepts forwarded-for headers from trusted hosts.

Additional info

| Q | A
| ---------------- | ---
| Yii version | 2.0.?
| PHP version |
| Operating system |

[SamMousa]

Another problem I just noticed when considering the order of headers is that we lose order between different header names (specifically relevant if we have multiple $ipHeaders configured).

Consider these headers:

X-Client-Ip: A
X-Client-Ip: B
X-Forwarded-For: C
X-Forwarded-For: D
X-Client-IP: E

In our HeaderCollection we will be able to verify that a header names X-Client-IP appears before any header named X-Forwarded-For. And that:

A comes before B
A comes before C
C comes before D
B comes before E

We cannot determine the rest of the ordering.

This is compliant with the IETF specification but becomes troublesome with multiple $ipHeaders. So it might make sense to rename it to $ipHeader and only support in a single value instead...

[samdark]

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For:

X-Forwarded-For: <client>, <proxy1>, <proxy2>

[SamMousa]

Excerpt from wikipedia, (and confirmed in real world testing of reverse-proxies):

The general format of the field is:

X-Forwarded-For: client, proxy1, proxy2[3]

here the value is a comma+space separated list of IP addresses, the left-most being the original client, and each successive proxy that passed the request adding the IP address where it received the request from. In this example, the request passed through proxy1, proxy2, and then proxy3 (not shown in the header). proxy3 appears as remote address of the request.

Since it is easy to forge an X-Forwarded-For field the given information should be used with care. The last IP address is always the IP address that connects to the last proxy, which means it is the most reliable source of information. X-Forwarded-For data can be used in a forward or reverse proxy scenario.

--

Specifically, if I specify that I trust proxy3, I should get proxy2 as the clients' IP.
Since I only specified that I trust proxy3 and the default behavior of the proxy is to append X-Forwarded-For headers it doesn't make sense to trust more than the last value.

Consider for example this snippet from the HaProxy docs for option forwardfor:

...
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.
...
Alternatively, the keyword "if-none" states that the header will only be added if it is not present. This should only be used in perfectly trusted environment, as this might cause a security issue if headers reaching haproxy are under the control of the end-user.

[SamMousa]

Okay, so we just had a discussion and the conclusion is that we disagree on the expected behavior.

The assumption of the current functionality is that any edge-proxy removes all existing X-Forwarded-For headers, before adding its own.

This is currently not the default behavior for HAProxy, CloudFlare and Amazon AWS.
Also anyone using nginx with $proxy_add_x_forwarded_for will be subject to spoofed IPs.

In my opinion with the trusted hosts functionality we should use the last untrusted IP in the X-Forwarded-For header. This will work exactly the same in all scenarios where there is just 1 level of reverse proxy.

In case of multiple levels of reverse proxying it will return the IP of the 2nd-last proxy since that is the only one it can guarantee is correct (ie trusted)...

[samdark]

The last IP address is always the IP address that connects to the last proxy, which means it is the most reliable source of information

It's most safe but definitely isn't most reliable. If we want client IP we can get it only from leftmost value. Proxy should be configured so it replaces X-Forwarded-For ignoring user-supplied value. Then it could be considered trusted.

[SamMousa]

That would work but it is not the default behavior anywhere; the usual approach for a reverse proxy is to append a header (which is semantically the same as appending an IP to the existing header).
No implementation by default discards the incoming value...

[cebe]

That would work but it is not the default behavior anywhere

that is a bold statement. A properly configured reverse proxy must set headers so that they contain only trustable values. If it does not, you can not trust the headers it sends and there is no way on the Yii side to know which values to trust and which not.

See nginx docs for an example:
https://docs.nginx.com/nginx/admin-guide/web-server/reverse-proxy/#passing-request-headers

[SamMousa]

that is a bold statement.

It definitely is, it is probably slightly hyperbolic; still I have yet to find a proof that it's not so. Of course any reverse proxy allows us to configure whatever we want, but there is no proxy that by default overwrites instead of appends the X-Forwarded-For header.

For example, nginx offers the embedded variable $proxy_add_x_forwarded_for.
Guides I could find suggest to use it, for example the guide from digitalocean.

[wooyen]

I test the two most popular web service Apache2 and Nginx. The default behavior of them are both adding the remote IP to the end of the existing X-Forwarded-For header. So if you deploy an proxy with the default config with Apache2 or Nginx and trust it, Yii2 can not get the real client IP when the client send a X-Forwarded-For to spoof the server. To prevent these, we must try to change the config of the proxy. But it is not a easy work for nginx, and more badly, it is an impossible mission (or a very hard work) for Apache2.
If we do this work in the Yii application, it will be very easy. We can just pickup the last untrusted IP in X-Forwarded-For header if the remote IP is truested. For example, if we have two trusted proxies of 1.2.3.4 and 5.6.7.8, and the application get a X-Forwarded-For header of "13.14.15.16, 9.10.11.12, 5.6.7.8" from the remote IP 1.2.3.4, we can just pick 9.10.11.12 as the real userIP.

[samdark]

How would that behave in case nginx is configured as in https://github.com/yiisoft/yii2/issues/16122#issuecomment-382419713?

[kamarton]

same #17573

[samdark]

Verified that issue description from @SamMousa (not solution) and entire comment of @wooyen are correct. Each proxy is appending remote host to the end of X-Forwarded-For as per specification.

Solution mentioned should work as well.

Test cases

There are test cases we should check against.

Having the following network:

Client -> Proxy1 -> Proxy2 -> Server

And a config where Proxy1 and Proxy2 are set to trusted in config.

Test cases are the following:

1. Client does not send X-Forwarded-For. Server is getting "Client, Proxy1" in X-Forwarded-For. User IP returned should be Client.
2. Client send SPOOF in X-Forwarded-For. Server is getting "SPOOF, Client, Proxy1" in X-Forwarded-For. User IP returned should be Client.

[samdark]

It is better to start with tests for it first.

[kamarton]

It already has a test.

Client does not send X-Forwarded-For. Server is getting "Client, Proxy1" in X-Forwarded-For. User IP returned should be Client.

remote address, x-forwarded-for, trusted hosts, expected user IP

https://github.com/yiisoft/yii2/blob/23a8d041a6867090d95df9051b294dcc73f50b7e/tests/framework/web/RequestTest.php#L739

Client send SPOOF in X-Forwarded-For. Server is getting "SPOOF, Client, Proxy1" in X-Forwarded-For. User IP returned should be Client.

remote address, x-forwarded-for, trusted hosts, expected user IP

https://github.com/yiisoft/yii2/blob/23a8d041a6867090d95df9051b294dcc73f50b7e/tests/framework/web/RequestTest.php#L738

[samdark]

So this issue is fixed?

[kamarton]

So this issue is fixed?

I think this is the same problem as the #17573 issue. Based on the tests, this case is already solved.