Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[8.x] Uncomment TrustHosts middleware to enable it by default #5477

Merged
merged 1 commit into from Nov 26, 2020
Merged

[8.x] Uncomment TrustHosts middleware to enable it by default #5477

merged 1 commit into from Nov 26, 2020

Conversation

DanielCoulbourne
Copy link
Contributor

We recently learned of a vulnerability in our app through our Bug Bounty program which allowed an attacker to change the host of a signed URL using the X-Forwarded-Host header. This allowed an attacker to generate a malicious password-reset email which, if the link was clicked, would send the user's password reset token to a website controlled by the hacker.

Enabling this middleware by default could close this vulnerability for many websites who may currently be exposed to this same vulnerability.

@driesvints driesvints changed the title Uncomment TrustHosts middleware to enable it by default. [8.x] Uncomment TrustHosts middleware to enable it by default. Nov 23, 2020
@GrahamCampbell
Copy link
Member

GrahamCampbell commented Nov 24, 2020 •
edited

This is not included by default because it breaks multi-tenancy applications. It is also possible to mitigate this vulnerability by using secure nginx/apache/load balancer configuration that checks the Host header. I would argue this is the correct place to address this, not within the app code). One should configure servers to look at the host header, just like one should configure databases to have a password other than the default. That said, Laravel provides this out of convenience.

For example:

$ curl https://styleci.io/ -H'Host: example.com'
<html>
<head><title>403 Forbidden</title></head>
<body>
<center><h1>403 Forbidden</h1></center>
<hr><center>cloudflare</center>
</body>
</html>

@CaddyDz
Copy link
Contributor

CaddyDz commented Nov 24, 2020

@GrahamCampbell do you know if Laravel Forge adds this nginx config by default?

@GrahamCampbell
Copy link
Member

I'm not sure. // cc @jbrooksuk

@taylorotwell
Copy link
Member

@CaddyDz Forge is not vulnerable to this even when not using this middleware because Forge sites listen on a specific host.

@DanielCoulbourne
Copy link
Contributor Author

DanielCoulbourne commented Nov 24, 2020 •
edited

To @GrahamCampbell 's point:

This will break multi-tenancy apps which use custom domains (subdomains of the APP_URL are explicitly supported by the default TrustHosts middleware). This sort of multi-tenancy scenario is pretty niche, and I don't think it's a big ask to make people remove one Middleware for that very specialized use-case.

Conversely, this vulnerability would affect all Laravel sites behind reverse proxies unless they explicitly set the X-Forwarded-Host header. This is not documented anywhere in the Laravel docs (because Laravel has no docs on setting up reverse proxies) and is an incredibly easy trap to fall into. I think we should prefer security-by-default for users who don't know everything to convenience-by-default for people building very specialized use-cases.

Ultimately this comes down to priorities. I'm making a suggestion that we prioritize security for a large segment of users over convenience for a small segment of users.

@GrahamCampbell GrahamCampbell changed the title [8.x] Uncomment TrustHosts middleware to enable it by default. [8.x] Uncomment TrustHosts middleware to enable it by default Nov 24, 2020
@DanielCoulbourne
Copy link
Contributor Author

I put together a quick demo of the vulnerability for anyone who is having a hard time envisioning it: https://twitter.com/DCoulbourne/status/1331317933675581442

@NationwidePharma
Copy link

NationwidePharma commented Nov 24, 2020 via email

@DanielCoulbourne
Copy link
Contributor Author

Nowhere near the majority, but many.

@NationwidePharma
Copy link

NationwidePharma commented Nov 24, 2020 via email

@inxilpro
Copy link

Also, just to be clear, the primary concern isn't the Host header—it's when an X-Forwarded-Host header is sent through a valid trusted proxy.

Imagine a scenario where someone followed one of the top Google search results for "nginx reverse proxy laravel" which, like most other tutorials that I've looked at today, suggest the following directive:

proxy_set_header X-Forwarded-For $remote_addr;

Next, imagine they followed the Laravel docs on TrustedProxies, adding their proxy server's IP address to the $proxies property of the middleware.

Given those two steps, they are now vulnerable to this exploit.

There are 3 possible fixes.

The first is to explicitly set all X-Forwarded-* headers at your reverse proxy. You can do this by adding these additional directives:

proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header X-Forwarded-Host $host;
proxy_set_header X-Forwarded-Port $server_port;

The second fix is to set TrustProxies::$headers to Request::HEADER_X_FORWARDED_FOR which means that it will only trust the X-Forwarded-For header and discard the rest.

The third is to enable the default TrustHosts middleware, which ensures that only subdomains of the the APP_URL are trusted.

I agree with @DanielCoulbourne that enabling TrustHosts is probably the best default for 99% of users. If that's not going to happen, I think that setting TrustProxies::$headers to Request::HEADER_X_FORWARDED_FOR by default is probably another good option.

@taylorotwell taylorotwell merged commit f6f772b into laravel:8.x Nov 26, 2020
@taylorotwell
Copy link
Member

I think I will go ahead with this. "Multi-tenant" applications are not as common as traditional applications and if someone is building one they can just comment out the middleware.

@DanielCoulbourne DanielCoulbourne deleted the enable-trust-hosts-middleware-by-default branch November 29, 2020 10:25
@patrickcarlohickman
Copy link

Just some additional context if anyone was curious.

This vulnerability was first fixed in 5.4.22, with this commit: laravel/framework@cef1055.

Here is a Laravel News article related to it: https://laravel-news.com/laravel-5-4-22-is-now-released-and-includes-a-security-fix. This fix also broke domain-based multi-tenant applications, but it seems the security aspect won out.

It looks like the vulnerability was reintroduced in 6.18.7, with this commit: laravel/framework@1eaaf6c (PR32345)

This seems like this is a nicer fix than the original one, though. Of course, this middleware didn't exist back then.

@jbrooksuk
Copy link
Member

Just a note for Forge users deploying their applications to the default site; by default, your app will return 404 Not Found. You have a few options to fix this:

  1. Comment out the TrustHosts middleware.
  2. Update your Environment so that APP_URL is set to the server's IP address.
  3. Deploy your site to a (sub)domain.

@themsaid
Copy link
Member

themsaid commented Dec 10, 2020 •
edited

This also broke Vapor. Vanity domains stop working once you add a custom domain, and if you have multiple custom domains only one will work and the rest will not work.

If we're keeping this middleware uncommented we should make a major announcement about it so people on Forge & Vapor make sure they comment it. Or add a build step in vapor to comment that middleware before deploying.

@inxilpro
Copy link

I do think that setting a new default for TrustProxies::$headers is another viable option. Maybe that's going to be a better solution that strikes the right balance.

@taylorotwell
Copy link
Member

Probably we have to revert this and let people opt-in to it like Symfony does.

@Thiritin
Copy link

@DanielCoulbourne Just for clarification, normally a Man in the middle attack should be adverted by using a secure connection aka "https". Isn't a Site protected from this vulnerability or more perhaps the user if the site is fully secured with https?

@michaeldyrynda
Copy link
Contributor

It's fairly common, I think, for load balancers to handle TLS offloading and communicate with application servers behind them over HTTP.

@paras-malhotra
Copy link
Contributor

I think @inxilpro's suggestion on the TrustProxies header might be the best way to go without breaking Forge. @Thiritin check out Daniel's demo video.

@inxilpro
Copy link

Just for clarification, normally a Man in the middle attack should be adverted by using a secure connection aka "https". Isn't a Site protected from this vulnerability or more perhaps the user if the site is fully secured with https?

It's not an HTTPS issue. The issue is that TrustProxies also trusts the X-Forwarded-Host header, which many proxies do not set by default.

@inxilpro
Copy link

@themsaid and @jbrooksuk — Are you able to check what happens when you:

  1. Disable the TrustHosts middleware
  2. Update TrustProxies::$headers from Request::HEADER_X_FORWARDED_ALL to Request::HEADER_X_FORWARDED_FOR

Do those two changes address the issues with Vapor and Forge?

suyar referenced this pull request Dec 21, 2020
@taylorotwell
comment trust hosts
b7cde8b
@paras-malhotra paras-malhotra mentioned this pull request Mar 5, 2021
Closed
@hackerESQ hackerESQ mentioned this pull request Mar 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone