Security fix for proxying individual X-Forwarded-* headers.

[Bradez]

There's a security issue wherein proxying a single (or set of) header/s (eg. X-Forwarded-For) will result in proxying all of the X-Forwarded-* headers. This is incredibly bad practice and opens room for various malicious attacks.

For example, if someone were to trust all proxies (which, whilst commonly bad practice, does in fact commonly occur in an environment where you may be using a load balancer. For example, restricting origin access to a load balancer and the load balancer to a reverse proxy provider) then Laravel will begin generating URLs with the user-supplied X-Forwarded-Host header.

When you combine this with an email, you're able to trick the system into sending an email using a URL that does not belong to the service. This attack is amplified as Laravel's default password reset system generates URLs that adhere to the host supplied in X-Fowarded-Host which ultimately can result in a password reset token leak.

Further, in general, if people are customising which headers are proxied, they expect it to only proxy those headers and not everything.

I've added tests for all of the individual scenarios as well as a scenario where the headers are combined individually.

[fideloper]

It's been a while since I source-dived into Symfony classes, I'm confused on how this change actually restricts only the X-Forwared-For header from being used and ignores the *-Host and other headers?

(Additionally, don't we WANT It to check the X-Forwared-Proto headers so Laravel generates URLs correctly with https when behind an LB that terminates SSL?)

Let me know if it sounds like I'm missing something here!

One other bit of info:

Symfony classes used to allow you define other headers (which is why the code in this package reflects that). However I believe it now only has options for X-Forwarded-* (_ALL) or X-Forwarded` headers. Is that your understanding as well?

I appreciate you bringing this to my attention!

[Bradez]

Hello @fideloper, the problem stems from the fact that this package isn't behaving how one would expect.

In Laravel's TrustProxies middleware, you have the ability to set which headers should be passed to setTrustedProxies().

You can see that the second argument ($trustedHeaderSet) is:

A bit field of Request::HEADER_*, to set which headers to trust from your proxies.

This means that the application can decide to only trust a given set of headers that have been defined, rather than them all.

If you know, for example, that you only want to trust the X-Forwarded-For header (and nothing else) from your load balancer / reverse proxy provider then you could do the following in the TrustProxies middleware:

protected $headers = Request::HEADER_X_FORWARDED_FOR;

However, as can be seen in the current TrustProxies file, it's currently handled in such a way that you only allow somebody to specify one of three options: HEADER_X_FORWARDED_AWS_ELB, HEADER_FORWARDED or HEADER_X_FORWARDED_ALL. You fallback on HEADER_X_FORWARDED_ALL if somebody provides anything that doesn't adhere to those three options. Therefore, regardless of what the developer supplies in the middleware's $headers property, you're dictating that every X-Forwarded-* header from the proxy should be trusted which is a big assumption to make.

Some proxies and/or load balancers only provide certain X-Forwarded-* headers based on what's changed from the initial request to the request being queried against the origin based on the proxy configuration. It is this problem that opens the doors for an attacker to abuse the fact that the X-Forwarded-* headers aren't configurable (assuming that the proxy is trusted) and so they can set any missing headers the proxy didn't provide (or override any, in the proxied request, that can be customised by the user).

For example, let's take Cloudflare (the largest reverse proxy provider) allows a connecting user to supply X-Forwarded-Host which will in-turn result in HTTP_X_FORWARDED_HOST being set to that value. In this scenario, let's say the only input that we can trust is X-Forwarded-For. Currently, the application will also trust X-Forwarded-Host (amongst all of the other X-Forwarded-* headers) despite being restricted to only trust X-Forwarded-For. Therefore, an application that trusts Cloudflare (even if it's just trusting the X-Forwarded-For header) will be indirectly updating the return value of the getHost() method with the value of X-Forwarded-Host and Laravel constructs URLs based on that.

Hence, to conclude, this pull request fixes this improper handling of the $headers property by only trusting the headers that have been supplied. By default, Laravel trusts all headers anyway so it's more of an "advanced configuration." Therefore, when a change is made for security reasons, they expect it to work without having to check the underlying middleware is in fact only trusting the headers specified - which it's not currently doing.

You can also refer to Symfony's current documentation about setTrustedProxies():

The Request object has several Request::HEADER_* constants that control exactly which headers from your reverse proxy are trusted. The argument is a bit field, so you can also pass your own value (e.g. 0b00110).

It's almost certainly one of the reasons behind why, beginning with Laravel 7.12, there's also the TrustHosts middleware which allows more granular control on which inputs (of the X-Forwarded-Host header) are to be trusted given the sheer damage that changing the Host header could inflict upon an application.

This pull request ultimately lets the developer decide what they want to trust, rather than making that assumption for them and without informing them.

[fideloper]

Great, thanks! I appreciate your explanation, and the time taken to make it (and of course the addition of tests).

[fideloper]

Phew, i’m not crazy! Thanks again, I’ll merge this in asap (likely a bit later tonight)

…

On Mon, Jun 22, 2020 at 16:11 Brad ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/TrustedProxyTest.php <#135 (comment)> : > + /** + * Test a combination of individual X-Forwarded-* headers are trusted. + */ + public function test_x_forwarded_multiple_individual_headers_trusted() + { + $trustedProxy = $this->createTrustedProxy( + Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | + Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO, + '*' + ); + + $request = $this->createProxiedRequest(); + + $trustedProxy->handle($request, function ($request) { + $this->assertEquals('173.174.200.38', $request->getClientIp(), + 'Assert trusted proxy did not use forwarded header for IP'); You're absolutely right, my apologies! Fixed. 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#135 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADSDU7CO2MDTAY4DWLCYTTRX7CJFANCNFSM4OEMVS3Q> .