HTTP_HOST header cannot be trusted

Note that Drupal 8 is subject to the same attack vector via HttpFoundation. The solution is to set only selected hosts to be trusted. See this Symfony SA:

http://symfony.com/blog/security-releases-symfony-2-0-24-2-1-12-2-2-5-an...

I recommend adding another commented-out block to settings.php for

Request::setTrustedHosts(array('.*\.?trusted.com$'));

with appropriate documentation.

I think we should use $_SERVER['SERVER_NAME'] in core to generate links.

You can then either set ServerName in your vhost config, or set $base_url if you get bad links. Both of those are less onerous than cache/e-mail poisoning with the current behaviour.

Bumping this to critical for 8.x and tagging D8 upgrade path since it could require particular installs to update their configuration.

That might not be an acceptable fix for 7.x but it'd be good to have some kind of hardening in there so also tagging for backport.

This is indeed critical.

In D8 we have _lots_ of opportunities for cache poisoning with the active-by-default #render_cache.

Want to take over a sites links to do some fishing right on the site? (only for absolute links though)

Then it is as easy as sending a poisoned host header and being the first to hit that node / listing ...

Want to display your own images? No problem at all ...

Happy hacking!

Yes, it can be easily fixed by doing proper vhosts, but seriously: Is convenience for Devs so much better than secure-by-default?

I would say: No and we should really fix it.

I don't think that adding a commented setting is going to help much. It needs to be enforced by default.

How about having a configuration item for a list of trusted (regexp) hosts, and have the installer ask for the site hostname(s) and prepopulate with the hostname used to install the site?

That host list is going to be different between my local system and prod, so it's one more thing to have to update. You already need to update db credentials, so maybe that's OK, but that would make it another must-remember-to-change property.

I defer to the sec team if that's an appropriate requirement.

Seems like we need a way for admins to configure a whitelist in the UI in addition to using SERVER_NAME

Suggest UX:

if you are logged out or don't have access to configure this setting and HTTP_HOST is not in the white list, you get a 403. If you are the admin, redirect to the configuration page if the setting is wrong (basically serve that one form to any domain for a logged-in user).

Talked with Nat and Peter Wolanin about it:

Consensus we reached was:

- Change to SERVER NAME by default -- secure
- Have a setting to use - HTTP_HOST
- Have a white list of things that contain *.local, localhost by default

That should cater to all use cases.

Would anyone be willing to Assign this issue to themselves. I'm a Base system component maintainer and doing triage on Critical items.

Bumping this to critical for 8.x and tagging D8 upgrade path since it could require particular installs to update their configuration.

Would #8 require a change of existing configuration from a D8 beta prior to this issue getting fixed? If not, should we remove the "D8 upgrade path" tag?

@effulgentsia it would for any install where SERVER_NAME won't work for multi-site selection - in that case they'd either need to add to the whitelist or set SERVER_NAME in their vhost config or similar.

@catch: but wouldn't that be the same if they installed for the first time after this issue was fixed? I'm confused what the "upgrade path" portion is.

@effulgentsia yes, but I think there's a material difference:

Install - you have to do an extra step to get your new install working properly.

Update - your site suddenly stops working until you fix this.

Once we start offering beta to beta upgrades, I think that includes not knowingly breaking sites that previously were working unless we absolutely can't avoid it.

Additionally we already said we'd get all public security critical issues fixed prior to providing the beta beta upgrade path - this doesn't allow for privilege escalation at all though so it's not like XSS/CSRF/SQL injection but broadly that felt like a good cut off point for resolving known issues.

I'll take this on. It's related to #2304949: Port HTTP Host header DoS fix from SA-CORE-2014-003, which I have been working on and was thinking about some similar issues. Ideally that would get in first, as I think this would be touch some of the same parts.

I won't be able to start real work until second week of December. Unassign me if this isn't OK.

Rock, thanks so much mpdonadio!

[I had a lengthy post ready, but lost it when the site went into maint. mode ... I'll try to remember what I had written]

Here are two options. They aren't full patches, just ideas to talk about.

Option 1 one sets up trusted hosts in DrupalKernel::createFromRequest() using the HttpFoundation mechanism that is already built into Request. It sets up some trusted hosts for localhost and variants, as well as trusting the current SERVER_NAME. Anything later on that does a Request::getHost() or friend will be checked against the trusted hosts. If the check fails, the server will send back a 400 b/c the change in #2304949: Port HTTP Host header DoS fix from SA-CORE-2014-003. While this doesn't follow the exact proposed solution, I think follows the spirit. I think this method will be testable, and I also think we can leverage sites.php for setting up the initial trusted hosts for multisite uses. rebuild.php wouldn't be covered (it doesn't go through createFromRequest). I think only users who use Domain Access and/or run true multiple domains w/o canonical redirection would have to adjust settings.php to add in some trusted hosts.

Option 2 handles it the URL generator. This gets ugly. Essentially we would have to subclass Request or proxy/bypass a lot of its public API. The patch just shows what would happen to generateFromRoute, and it is only partial logic. generateFromPath uses different logic. This also wouldn't prevent contrib from using the public API on Request, and bypassing the protections we put in place. There are also a few other places in core, eg file_url_transform_relative, that use ->getHost(), so there would have to be a careful audit.

I love option 1!

This is not only in the spirit of the original proposal, but IMHO even better, to just include SERVER_NAME as one trusted host and avoid having to special case that case.

I also like option #1 a lot.

Additionally that sounds like we might be able to do it in 8.0.x after the upgrade path is supported and/or backport it to Drupal 7 (at minimum for new installs with a killswitch/bypass defaulting to ON for existing installs)?

Option 1 is generally better, no question. But is SERVER_NAME a trustworthy value?

Also, I think we've been trying to move away from Settings to using container parameters setup via services.yml. Could that be used here instead?

This has to be in sites.php because the information is used to determine which site is selected. There's no container at this point and cannot be because which container is loaded depends on which site you're in.

Ah, fair point, got it.

SERVER_NAME is always admin entered, so a safe value.

This is also again just a heads up on the critical-ness on the issue.

Just saw a cache poison "attack" on a rather large site. Someone used the IP address to access the site and then images failed to load as they were wrongly cached in the views_cache (and SSL would correctly block the images coming from the wrong domain) ...

This was not an attack per se, but it totally means, this is not just theoretical.

I also think the handbook page should first of all say to have a default vhost with empty or disallow from all and then having vhosts for all the allowed server names.

Neither option stated there of fiddling with base url would have worked as the site is multi-lingual.

Is it worth testing these patches yet?

No, I will be proceeding with Option 1, but those patches are just rough concepts. Option 1 is close in spirit, but it will find a better place to live in the kernel. I am hoping early next week for a preliminary, but real, patch.

Manual testing the option 1 patch to see where it doesn't work would be useful so we can revise our proposed solution.

Here is a first pass. I don't know why Drupal\views_ui\Tests\FilterNumericWebTest and Drupal\views_ui\Tests\PreviewTest are failing here w/ an untrusted host.

Where should the tests for this live? It needs to be a simple test, as we need to add some exception handling in the kernel to catch this, and then generate a proper 400. The test also needs to do some cURL shenanigans to spoof the Host header.

I'll add in a section to default.settings.php once we have tests and know what our final solution is.

This is more a note to myself, but we can add an explicit check right after we set up the trusted hosts that does a ->getHost(), catches that exception and then throws BadRequestHttpException. The mechanism to handle that is already in place (very similar to how #2304949: Port HTTP Host header DoS fix from SA-CORE-2014-003 got done), and that will make this unit testable, and the test can live in \Drupal\Tests\Core\DrupalKernel.

Will followup later on this patch, but the interdiff may reveal what I am worried about...

OK, still waiting on a clogged TestBot, but here is what's happening.

The two fails from #30 seemed to be related to previewing Views. As far as I can tell, those are the only two test that actually do a views preview. The problem is that

1. The Host header seems to be an empty string when the preview runs
2. This triggers an untrusted host exception (the Host header is mandatory as of HTTP 1.1)

The interdiff shows what I did to "fix" it. The preview makes a new Request for the preview and pushes it onto the stack. The patch uses `Request::createFromGlobals()` instead of `new Request()` to make sure the server properties get set properly, and then copies over the trusted hosts. When you set the trusted hosts, you pass in an array of regxes w/o delimiters, but when you get them, the array or regexes has the delimiters in them.

This leads me to a few observations.

1. Request:: setTrustedHosts() sets a class static for using the same trusted hosts / patterns across Request objects, but it doesn't seem work here.
2. All of the other uses of the RequestStack seem to work (well, the tests pass). So, we are either lacking test coverage, or something weird is going on.

The rest of the patch is somewhat in progress to show the direction I was going in. The patch includes a stubbed out, failing test, that I will be working on next. We will also need some bash scripts that curl w/ naughty headers to test out the 400 generation.

Tests, and bugfixes that writing the tests revealed.

Here is a script to test with. Edit the script for the config you want to use, then invoke as `test1.sh www.example.com` or whatever you want to spoof as.

Note, that Apache sets SERVER_NAME and HTTP_HOST to the same hostname (verified on Apache 2.2.26. You need to enable UseCanonicalName for this patch to be effective. See also this answer at StackOverflow. The final documentation will have to reflect this.

Added test messages, and added tests w/ empty Host headers to simulate older HTTP/1.0 clients.

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -622,6 +622,12 @@ public function renderPreview($display_id, $args = array()) {
+      $hostPatterns = $current_request->getTrustedHosts();
+      $hostPatterns = array_map(function ($hostPattern) {
+        return substr($hostPattern, 1, -2);
+      }, $hostPatterns);
+      $request->setTrustedHosts($hostPatterns);

This feels like it needs some documentation. I could suss out what's going on, but the Why escapes me.

You mention above "final documentation". Is there still more doc work to do here, do you think? At least conceptually this seems good to me; I dislike throwing yet more statics into DrupalKernel but I don't think there's much we can do about that at this point.

1. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -622,6 +622,12 @@ public function renderPreview($display_id, $args = array()) {
    
   +      $hostPatterns = $current_request->getTrustedHosts();
   +      $hostPatterns = array_map(function ($hostPattern) {
   +        return substr($hostPattern, 1, -2);
   +      }, $hostPatterns);
   +      $request->setTrustedHosts($hostPatterns);
   +
   

   I have to fully agree with crell here, it is so not obvious what is going on.

2. +++ b/core/tests/Drupal/Tests/Core/DrupalKernel/TrustedHostsTest.php
   @@ -0,0 +1,129 @@
   +class TrustedHostsTest extends UnitTestCase {
   

   Can we prefix the test name with DrupalKernel?

So ...

I'll document what is happening and what this code does, but I don't know *why* it is happening.

The Request object uses a class static for saving the trusted hosts (cf, Reqeust::setTrustedHosts), which means that the same list is shared between Request objects. If I put in a hack somewhere and do a

$request2 = new Request();
$trustedHosts = $request2->getTrustedHosts();

I can see the trusted list is the same as what the kernel set up.

However, it does not seem to do this in Views previews; hence explicitly copying over the trusted host list. Lots of other places in code make Requests on the fly and also use the RequestStack, but this appears to be the only place that the trusted mechanism breaks down. I find that troubling / weird.

Final documentation. That just means adding a section to default.settings.php showing how to use this. Since we are happy with the approach/implementation in general, I'll get that into the next patch.

This has tests now, removing tag.

Did some more testing with this. The explicit trust host copying doesn't seem to be needed if Request::createFromGlobals() is used instead of new Request(). Added comment.

Renamed test.

First pass at a new section in default.settings.php to explain the new settings.

Posted wrong interdiff...

+  public static function setupTrustedHosts(Request $request) {

This is internal code, should be protected.

+    $hostPatterns = Settings::get('trusted_host_patterns', array());
+
+    // Allow an empty Host header
+    $hostPatterns = array_merge($hostPatterns, array(
+      '^localhost$',
+      '^localhost\.*',
+      '\.local$',
+    ));

I don't think this makes much sense. We should trust the host name mapping to the current site (if not "default"), and we should not allow anything else by default.

+    $server_name = $request->server->get('SERVER_NAME');

SERVER_NAME is untrusted user input under Apache if UseCanonicalName if off (its default value), which basically defeat the purpose of this patch. so we should think about this twice before trusting it blindly.

+      $hostPatterns[] = '^' . str_replace('.', '\.', $server_name . '$');

This needs to use preg_quote(), there are many special characters that can get in there. For example, on Nginx if you use a regexp in the server_namedirective, you will get the full regep in SERVER_NAME.

#46-1, the method is public because it is explicitly called in the test.

#46-2, the comment there is misleading. It should probably read "Trust local hostnames to account for misconfigured local development environments." It can go, though, if there is consensus.

#46-3, other than strongly suggesting `UseCanonicalName On` in the installation instructions, not sure what else we can do here, but I am open to ideas.

#46-4, yup. Fixed in my local branch.

#46-1, the method is public because it is explicitly called in the test.

Then move it to a method object. This should not be a public method of the kernel.

#46-2, the comment there is misleading. It should probably read "Trust local hostnames to account for misconfigured local development environments." It can go, though, if there is consensus.

I understand the intent, but you cannot know what is a development environment or not. Having those hosts there basically defeats the purpose of this patch too.

#46-3, other than strongly suggesting `UseCanonicalName On` in the installation instructions, not sure what else we can do here, but I am open to ideas.

As far as I can see, this directive can be used in a .htaccess, anyone tested that?

I have tested it; it doesn't work:

[Wed Dec 31 12:57:52 2014] [alert] [client ::1] /Applications/MAMP/htdocs/drupal-8.0.x/.htaccess: UseCanonicalName not allowed here

Per http://httpd.apache.org/docs/current/mod/core.html#usecanonicalname, the context for that directive is "server config, virtual host, directory" The context ".htaccess" would be needed for that to work (contexts are described at http://httpd.apache.org/docs/current/mod/directive-dict.html#Context)

This addresses #46-1 by using reflection, and #46-4. Since we are messing with the kernel with hostname related validation, should I also make `validateHostname()` protected and test via reflection?

Can we discuss allowing the local names by default, and the UseCanonicalName / SERVER_NAME issue during the WSCCI mtg on Thursday?

+++ b/sites/default/default.settings.php
@@ -631,3 +631,36 @@
+ * will allow the site to run off of all variantes of example.com and

%s/varaintes/varaints

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1313,4 +1326,67 @@ public static function validateHostname(Request $request) {
+    $hostPatterns = array_merge($hostPatterns, array(
+      '^localhost$',

Not sure I agree with this - can't this be part of the default.settings.php comments/examples instead?
Not sure we should babysit broken configuration, also wouldn't localhost.evil.com match against '^localhost\.*' so that won't fix the original problem

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -222,6 +225,16 @@ public static function createFromRequest(Request $request, $class_loader, $envir
     $kernel->setSitePath($site_path);

Note the call to findSitePath above already calls $request->getHost() as well as static::validateHostname - which also calls $request->getHost(). Perhaps some of these could be consolidated?

I'll remove the local defaults, and figure out a better way to have this enabled by default, but also work with misconfigured local devs.

Re #51 and the findSitePath. The first call to $request->getHost() is done before we have a path to settings.php, and the validateHostname() is the DOS protection from #2304949: Port HTTP Host header DoS fix from SA-CORE-2014-003. If someone needs to have overrides, they need to go into settings.php, so we need to set that up into the Symfony trusted hosts, and then go another $request->getHost(), which does the trusted host check via protected method. Not sure we can clean this up until we abandon multisite support.

If we remove the localhost defaults, and consider SERVER_NAME as untrustworthy in Apache situations where `UseCanonicalName off`, then I don't think we can have this enabled by default. This patch makes the protection opt-in via settings.

Open to suggestions on where to go from here.

@mpdonadio: Drupal just doesn't have enough information to validate this by default, so making it opt-in sounds quite reasonable to me.

@mpdonadio: Drupal just doesn't have enough information to validate this by default, so making it opt-in sounds quite reasonable to me.

That said, we should add a requirements warning in that case, as suggested by Peter in the original private issue.

We could even make that configurable directly from the installer.

After I submitted this, I started to think about the UX of adding this to the installer.

Would we want to keep it in settings, or move it to config? I am thinking keeping it in settings so that (1) it will vary host-to-host so we don't want it to move between dev<->staging<->live and (2) we probably want this to happen before we boot the kernel so the protection is already in place there.

I think the best in the installer would be the CONFIGURE SITE page on the installer. I see three potential options: checkbox (defaulted to checked) to use current SERVER_NAME, a textfield prepopulated with SERVER_NAME that user can change to set up a single host (probably my preference), or a textarea prepopulated with SERVER_NAME and that the user can add multiple patterns, too.

How about this plan. I add a hook_requirements('runtime') that does a REQUIREMENT_WARNING if the setting is empty; we would want that anyway. We review/commit the current approach, and publish a CR with the new setting. Then we file a followup issue as a major for adding the config to the installer. That way we can get more people testing this in real situations earlier, and I have a nagging suspicion that we are going to run into complications with `drush site-install` which will affect TestBot, and also spend a while working out the minutiae of the UX.

Plan?

We in general try very, very hard not to add options to the installer. And this:

"checkbox (defaulted to checked) to use current SERVER_NAME, a textfield prepopulated with SERVER_NAME that user can change to set up a single host (probably my preference), or a textarea prepopulated with SERVER_NAME and that the user can add multiple patterns, too."

Sounds like something I would have no idea how to fill out, let alone a typical Drupal site builder. :\

We discussed the installer option at the WSCCI mtg today, and decided to proceed with just using settings for this issue, and warn the user via hook_requirements(). I will post a patch with this soon.

Having the installer create the setting will be done as a followup, #2404259: Allow trusted hosts to be configured with the installer. Details can be discussed there, and we can always Close (Won't Fix) if it becomes a problem.

Without the installer bit, the patch from #53 looks good to me, unless you want to add a hook_requirements.

Well, I would assume this is often not the case, so for those people they would have to adapt their settings.php? I guess they would have to do that most of the time
anytime in order to fix their sql connection settings and maybe even more.

Can we just save the current SERVER_NAME to settings.php on installation? The assumption would be that users invoke the installer on the final production domain.

I don't think that is a good assumption. A lot of projects where I was involved used a pre-production environment for "installation", followed by a migration/switch to the production domain which didn't involve any installation step.

Given that people don't remember that, does that mean the 400 should point to the settings.php to change it?

Added a hook_requirements and test coverage for the additional status.

Will experiment with adding the equivalent of

$settings['trusted_host_patterns'] = array('^' . preg_quote(\Drupal::request()->getHost()) . '$');

to the installer code. I will likely not do this if `PHP_SAPI === 'cli'`. Not sure how to handle drush installs.

Hmmm. That passed for me locally... Trying some things.

Because the Request::$trustedHostPatterns is static, this breaks in a number of places as soon as you change your trusted host pattern away from anything that matches 'localhost' as we saw in the Views portion of this patch. I traced this failure to the breadcrumb system in PathBasedBreadcrumbBuilder::getRequestForPath(), which does:

    // @todo Use the RequestHelper once https://drupal.org/node/2090293 is
    //   fixed.
    $request = Request::create($this->context->getBaseUrl() . '/' . $path);

#2090293: Fix RequestHelper says that RequestHelper has been dropped...if this is the case, we're going to have to find another way to handle this use of Requests for setting up routes.

EDIT: The resulting request object has it's host as 'localhost' since the base url from the request context is empty here, this means that trusted host checking fails when calling getHost() on the requests created as part of the breadcrumb.

Setting $base_url in settings.php did not help.

I'll remove the local defaults, and figure out a better way to have this enabled by default, but also work with misconfigured local devs.

This could live in example.settings.local.php. See https://www.drupal.org/node/2259531.

This should fix the errors from white screen related to the breadcrumb system. I'm not in love with the solution, but creating a new request from bits and pieces of the current request context, and other local vars, seems a bit kludgy to start with.

EDIT: The resulting request object has it's host as 'localhost' since the base url from the request context is empty here

This is a bug. Symfonys RouterListener is supposed to update the request context with values from the current request.

this means that trusted host checking fails when calling getHost() on the requests created as part of the breadcrumb.

Could you try swapping out the request context with request stack in the breadcrumb builder?

Note: #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes introduces a method for this exact usecase, don't it? It's in, so we can use getCompleteBaseUrl.

Thanks, everything seems to work as expected with the new getCompleteBaseUrl() from RequestContext.

I think the remaining task on this would be to use to create a section in example.settings.local.php, using the old defaults and section in default.settings.php as a starting point. I can do this tomorrow. Otherwise, I think people can review #75 for the actual code changes now that it is green.

Thanks for the feedback; I will work have something ready for further review soon. I am not sure if I explicitly tried to test the BadRequestException(); I'll look into it.

I will also create a followup to investigate prepopulating the trusted hosts during setup.

Addresses #77. Added section to example.settings.local.php for localhost. Also added a short section to SiteSettingsForm to save off the hostname when the installer is run from the web. I don't know if this is testable (if so, please suggest how).

I don't see how we can actually test the BadRequestException in our testing framework.

-    $request = Request::create($this->context->getBaseUrl() . '/' . $path);
+    $request = Request::create( $this->context->getCompleteBaseUrl() . '/' . $path);

Extra space introduced

Fixed #81 and cleaned up imports.

Some typos and missing dots at the end

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -217,6 +220,15 @@ public static function createFromRequest(Request $request, $class_loader, $envir
   +    // Initialize our list of trusted HTTP Host headers to protect against
   +    // header attacks
   

   Missing dot at the end

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1254,4 +1266,46 @@ public static function validateHostname(Request $request) {
   +   *   The request object
   ...
   +   *   The array of trusted host patterns
   ...
   +   *   TRUE if the Host header is trusted, FALSE otherwise
   

   Missing dot at the end

3. +++ b/core/modules/system/src/Tests/System/TrustedHostsTest.php
   @@ -0,0 +1,62 @@
   +   * Tests that the status page shows the trusted patterns from settings.php
   

   Missing dot at the end

4. +++ b/core/modules/system/system.install
   @@ -611,6 +611,28 @@ function system_requirements($phase) {
   +  // See if trusted hostnames have been configured, and warn the user if not
   +  // set.
   

   and warn the user if they are not set

5. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -611,8 +611,9 @@ public function renderPreview($display_id, $args = array()) {
   +      // Request object gets all of proper values from $_SERVER.
   

   gets all the proper values

6. +++ b/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTrustedHostsTest.php
   @@ -0,0 +1,78 @@
   +    // Tests canonical URL
   ...
   +    // header is optional
   

   Missing dot at the end

7. +++ b/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTrustedHostsTest.php
   @@ -0,0 +1,78 @@
   +    // Tests mismatches
   

   Tests mismatch.

8. +++ b/sites/default/default.settings.php
   @@ -607,3 +607,40 @@
   + * expression paterns, without delimiters, representing the hosts you would like
   

   paterns -> patterns

Addressed 83.1-8
Addressed 84.2

DID NOT address 84.1

P.S., not sure I did the interdiff quite right, the diff format does not look right...

I was in the middle of posting my patch. Stand by.

This patch will address everything from #83 and #84. The interdiff in #85 is messed up, so I also attached that done via a `git diff` between my branches.

Re, #84-2, we did not want a message in #2304949: Port HTTP Host header DoS fix from SA-CORE-2014-003, so that hunk was there to explicitly make sure none was printed now that index.php outputs the exception message. However, I did test everything with that hunk removed with my DOS script, and it works properly, and as expected.

I'll work on both today.

OK, I think the IS and CR are good to go.

throw new BadRequestHttpException('Invalid HOST header detected, check $settings[\'trusted_host_patterns\'] in settings.php.');

Here we are potentially displaying site administrator recommendations to end users. I consider this a bad practice. I would recommend instead that we display a simple, but specific error message that site administrators can google. Something like "The provided host name is not valid for this server.", maybe?

While I agree with #95 we already have the developer/admin specific info in index.php for rebuilds.

It would be nice if we could do some detection to see if the user is likely to have access to settings.php, but I suspect there is no way to do this so early in the bootstrap.

I agree with @Damien Tournoud in #95. This message shouldn't have anything about the settings.php - the error status is 400 and unlike the cache re-generate message this exception can be caused by something external Drupal - i.e the evil http post in the issue summary. Also the developer specific info in the index.php only occurs if the drupal error handler throws an exception or the exception is thrown before the error handler is registered.

When this get committed, I'll edit the Handbook and also post a question/answer at Drupal Answers to the error message has high googleability.

I'm not 100% certain how pre-population in the installer is going to turn out. But we can always remove that if it creates more problems than it solves. Also Drupal 8's current exception handler is never ever reached because of the catch all in index.php. I think we need to review what we are doing with errors and exceptions - but that is not up to this issue.

Committed 2c9eddf and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

After applying this patch I have locel phpunit failures for both php 5.4 and 5.6. I'm reverting the patch. See https://gist.github.com/alexpott/4ea9a9c0b9de6918e816

@alexpott
So we should investigate IMHO why the testbot did not caught that, do you know why?

Rather baffled, but I will see what is going on...

So testbot runs each phpunit test individually whilst I run them all at once. Symfony's request object stores the trusted headers in a static property - which allows them to bleed across tests. I've removed the usage of Settings since that is not under test and is unnecessary.

Symfony do exactly the same thing in their tests see Symfony\Component\HttpFoundation\Tests\RequestTest::testTrustedHosts()

Going to optimistically bump this up. The changes are straightforward and I recreated both the previous failures and the lack there of with the new patch.

Looks good to me, I ran the phpunit with an older patch and the latest to confirm the testing bug and the latest patch fixes it.

+++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
@@ -155,6 +155,13 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    // Set the initial trusted host value if this isn't a command line install.
+    if (PHP_SAPI !== 'cli') {
+      $settings['settings']['trusted_host_patterns'] = (object) array(
+        'value' => array('^' . preg_quote(\Drupal::request()->getHost()) . '$'),
+        'required' => TRUE,
+      );
+    }

I'm extremely uneasy about this. I think this will represent a big challenge to users who build a site locally on something like mamp and copy everything up to a host. I think we should remove this. Discussed with @catch in IRC and he agreed.

I'm also questioning my choice to make the error message very generic. This is a hard call. In some ways having trusted_host_patterns configured wrongly is like mis-configuring your database settings, on the other hand, it is possible to cause this exception by making a malicious http request.

Per more IRC w/ @klausi and @alexpott

1. Remove installer setting.
2. Leave error message as-is.

Since my changes are only to the unit test I think it is valid for me to commit this. Committed 98377d5 and pushed to 8.0.x. Thanks!

My only issue, coming from the noob perspective, was when I hit the new nag message in the admin, that the link takes you to https://www.drupal.org/node/1992030, which doesn't really give you a good "here's what you are supposed to do, dummy" kind of thing. It starts talking about D7, and I'm on D8, and I end up just being like, "huh?"
I was able to figure it out, find the change record, find this issue, but less well equipped noobs might find this kind of thing really frustrating.
I would suggest changing the link in the nag message in the admin to instead point to the change record, and make sure that the change record has a good, simple, "this is what you do to get rid of this message in the right way" kind of how-to right up front.
Also the comment right above the trusted_host_patterns section in settings.local.php could be a little more helpful as to how you are supposed to alter it to get the nag to go away. It defaults to localhost, but an idiot like me who calls their computer by "http://desktop" has to make a change there, so maybe a link to something on regex patterns?

@sidharrell, editing Protecting against HTTP HOST Header attacks to give information about this patch is on my todo list. Adding a questions w/ answer to Drupal Answers is also on my todo list. So, plans are underway to fully document this, but we just haven't gotten to yet.

The changes to example.settings.local.php really threw me off today, we don't want to do that.
#2416563: Follow-up to "HTTP_HOST header cannot be trusted"

Initial D7 port version.
There is a major issue with this patch it doesn't work.
Configuration is not initialized yet when host is been checked with drupal_valid_http_host() in drupal_environment_initialize().
Not sure how to handle this further. Any advise or recommendation are welcomed.

+++ b/includes/bootstrap.inc
@@ -711,12 +711,16 @@ function drupal_environment_initialize() {
-  return strlen($host) <= 1000
+  $length = strlen($host) <= 1000;
     // Limit the number of subdomains and port separators to prevent DoS attacks
     // in conf_path().
-    && substr_count($host, '.') <= 100
-    && substr_count($host, ':') <= 100
-    && preg_match('/^\[?(?:[a-zA-Z0-9-:\]_]+\.?)+$/', $host);
+  $subdomains = substr_count($host, '.') <= 100;
+  $ports = substr_count($host, ':') <= 100;
+  $characters = preg_match('/^\[?(?:[a-zA-Z0-9-:\]_]+\.?)+$/', $host);

Didn't get past this. The DoS protection is the chaining here. The preg_match() can explode so it relies on the behavior of && to not run if the sanity checks don't pass.

Follow up on #120 on D8: #2419225: settings.local.php trusted_host_patterns are ignored.

Today I was confused that the domain I'm installing the site on is not added by default to the trusted_host_patterns variable.

Are there plans to configure this setting by default with the installation url?

@hass, we tried to do that and it got rather complicated (note the several followups to get this version right). There is an issue to discuss automagically setting this with the installer: #2404259: Allow trusted hosts to be configured with the installer

Based on #125, changing to NW. We want to return FALSE before we hit preg_match with a bazillion subdomain request.

Got this today when setting up a demo site, and realized that now site builders are required to hand-craft regular expression patterns in PHP (including manually escaping problematic characters like "." which are in literally every URL), after futzing with file permissions to make settings.php writable (hope they remember/know how to switch it back... :\) in order to not have errors showing on their status screen on a fresh install. :(

It seems like it would be vastly preferable to move this to a YAML file somewhere or similar which would be much easier for non-technical users to edit. Or better yet a config page, but I'm assuming we can't do that for security reasons, heh.

I can open a follow-up, but wasn't sure if it would be vetoed out of the gate. I know about #2404259: Allow trusted hosts to be configured with the installer but not sure that's a solution since a lot of people install on localhost or similar before moving it to their "real" environment which will have a different URL.

#130: How did you hit that bug / feature? In my testing the protection was by default off as that variable was not set at all. Or was this just a "best practice" training?

@Fabianx you don't hit the actual exception, but because we don't set anything for this in the installer, you get a hook_requirements() warning - attaching screenshot.

@webchick the reason this is in settings.php is so we can bail out really, really early if there's no match.

I think we could look at whether it would be OK to bail out later (and then this could go into config and have a UI). Or also if we can't do that, whether we should allow people to set up some domains in the installer.

I think the minimum we should do is create a code generator based on a UI that you can then copy to settings.php.

Removing some D8 tags so I don't get confused. :D

I'm finding a need for this functionality in Drupal 7. Is anyone else in the same situation?

Hi

I need this for Drupal 7 version.
I added predefined domain name list and a default redirect domain name in hook_boot().

What will be issues/problems with this approach?

  // hook_boot()

   function trusted_boot() {
     $host = $_SERVER['HTTP_HOST'];
     $trusted_host = array('domain1.com', 'domain2.com');
     $default_redirect_domain = 'http://domain3.com';
      
     if (!in_array($host, $trusted_host)) {
          Header("Location: $default_redirect_domain");
	  exit;
     }
  }

Addressing #125.
Still not sure how to be with configuration not being initialized when the check is happening.

Maybe we can combine this somehow with sites mapping from "sites.php" and conf_path()?

+++ b/includes/bootstrap.inc
@@ -3849,3 +3850,28 @@ function drupal_clear_opcode_cache($filepath) {
+    if (in_array($host, $trusted_hosts)) {

How often is this called? If we bother to statically cache here, we might also think about using the faster isset() here?

About the variable_get() not working in that stage - for me the answer seems to be there right after the corner and asking if we could switch the function order in _drupal_bootstrap_configuration()?

I mean the new function drupal_check_trusted_hosts is called inside drupal_valid_http_host(). drupal_valid_http_host() is called in quite simple function drupal_environment_initialize(). And here's the key part - both drupal_environment_initialize() and drupal_settings_initialize() is called in _drupal_bootstrap_configuration() - https://git.drupalcode.org/project/drupal/-/blob/7.x/includes/bootstrap.... Can't we just switch these functions?

As security is first citizen and is taken quite seriously in Drupal then who could we ask if this is ok or what would be better solution?

edit:
Check a bit further and I see that $_SERVER['HTTP_HOST'] is modified in drupal_environment_initialize() and then that variable is used in drupal_settings_initialize() for $base_root.

Still, answer seems so close. Let's move drupal_valid_http_host() to _drupal_bootstrap_configuration()?

@geek-merlin

I have 1 more question on your last comment - why is it returning $host and not TRUE:

    if (in_array($host, $trusted_hosts)) {
      return $host;
    }

I'm pretty sure it should be boolean.

Not sure why drupal_static is used here - don't see when this would be called multiple times.

• I know in ideal world it would be nice to have the check in drupal_valid_http_host() but to me it would not hurt to run it just a tiny bit later to make it actually work and be able to red from settings.php
• Added the same error message as in D8 and moved it to exit() so it would be visible to users (also exactly like in D8)
• Didn't see the point in using static plus it was returning $host. It is requested only once or did I miss something?
• Added function comment to drupal_check_trusted_hosts()

Error code was still 200 on my server so I've replaced header() with http_response_code().

Wanted to add that when Lando for some unknown reasons changed port to 8000 on my local domain, then the regex started failing. Reason is that in drupal_check_trusted_hosts($host) the $host parameter also contains port eg mydomain:8000. It should probably not happen or maybe it's D7 specific thing. We should remove port when doing regex.

Rerolled the #147 patch porting symfony/http-foundation's logic.

Main changes from the last commit:

• Using the $t variable for translations in the system_requirements hook.
• Address use cases where the port is included in the hostname (using logic from symfony/http-foundation).
• Use header($_SERVER['SERVER_PROTOCOL'] . ' 400 Bad Request'); rather than http_response_code(); to maintain parity with how the rest of Drupal sends its header response.
• Removed the str_replace('}', '\\}', $pattern) logic to maintain parity with the D8 version.

Reverting to proper status.