Port HTTP Host header DoS fix from SA-CORE-2014-003

drupal_valid_http_host() was removed in #2016629: Refactor bootstrap to better utilize the kernel and we no longer perform any validation, but DrupalKernel::findSitePath() looks very similar to D7's conf_path() so the vulnerability likely still exists.

Let's add back the validation from 7.x while we're here, looks like an oversight in the other issue.

Just to kick this off, I am trying to figure out where this should go.

Comment removed for security reasons.

https://github.com/sensiolabs/symfony-httpfoundation-request-host/pull/2...
We will need to update our findSitePath to use getHost

https://security.drupal.org/node/124338

Symfony had the same vulnerability that had to be patched before we could proceed with this issue. Symfony just released a patch. http://symfony.com/blog/cve-2014-5244-denial-of-service-with-a-malicious...

So we must postpone on #2234277: Composer update (includes security fixes) then figure out if anything else needs to be done on this.

Tagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.

#2278353: Update to Symfony 2.5 is in so this is active.

See Symfony\Component\HttpFoundation\Request::getHost.

Just to be thorough, HEAD is on Symfony 2.5.5 and here is the commit where the fix was added: https://github.com/symfony/symfony/commit/0aaabbfe8b49b436fe321823d89ae8...

Do you think we should bring in the tests too? Although note that we got them into Symfony upstream anyway.

@larowlan I am divided on that. If we were building Drupal from scratch right now and using the Symfony Request class, we would not write a test for this.

@klausi Thank you for the review. I think that if a request is this malformed, there should be no fallback.

#23 doesn't take into account hosts running on ports other than 80. Patch attached, but still shows ~6sec requests w/ the @klausi script.

Setting to Needs Review just so tests run.

If I just throw in

--- a/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Request.php
+++ b/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Request.php

@@ -1166,6 +1166,10 @@ public function getHost()
             }
         }
 
+        if (strlen($host) > 1000) {
+          throw new \UnexpectedValueException(sprintf('Invalid Host "%s"', $host));
+        }
+
         // trim and remove port number from host
         // host is lowercase as per RFC 952/2181
         $host = strtolower(preg_replace('/:\d+$/', '', trim($host)));

Then the DOS script runs in about ~170ms. Does this mean that the Symfony update isn't sufficient to protect against the DOS?

@klausi, if you change

--- a/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Request.php
+++ b/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Request.php
@@ -1173,7 +1173,7 @@ public function getHost()
         // as the host can come from the user (HTTP_HOST and depending on the configuration, SERVER_NAME too can come from
         // check that it does not contain forbidden characters (see RFC 952 and RFC 2181)
         // use preg_replace() instead of preg_match() to prevent DoS attacks with long host names
-        if ($host && '' !== preg_replace('/(?:^\[)?[a-zA-Z0-9-:\]_]+\.?/', '', $host)) {
+        if (TRUE) {
             throw new \UnexpectedValueException(sprintf('Invalid Host "%s"', $host));
         }

to force the exception, your test script executes in about 150ms.

@klausi I am trying to do it by-the-book with that exception. It is interesting that it may not actually help.

This:

header($request->server->get('SERVER_PROTOCOL' . '400 Bad Request'));

is preferred over

header($_SERVER['SERVER_PROTOCOL'] . ' 400 Bad Request');

@mpdonadio: Could you try:

header($request->server->get('SERVER_PROTOCOL' . '400 Bad Request'));
exit();

... at both places with the try/catch in DrupalKernel?

The header() thing didn't seem to make any difference. We should profile this to see where the delay is coming from.

Two more data point on this before profiling,

1. I wasn't totally accurate in #32.

--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -319,7 +319,8 @@ public static function findSitePath(Request $request, $require_settings = TRUE)
       $http_host = $request->getHost();
     }
     catch (\UnexpectedValueException $exception) {
-      throw new BadRequestHttpException(t('Bad request'));
+      header($request->server->get('SERVER_PROTOCOL' . '400 Bad Request'));
+      exit();
     }
 
     $sites = array();
@@ -824,7 +825,8 @@ protected function initializeRequestGlobals(Request $request) {
         }
       }
       catch (\UnexpectedValueException $exception) {
-        throw new BadRequestHttpException(t('Bad request'));
+        header($request->server->get('SERVER_PROTOCOL' . '400 Bad Request'));
+        exit();
       }
 
       $base_url = $base_root;

does not result in the 400 Bad Request being generated. The output from the script is a 404 Not Found page where all of the URLs are using the spoofed HOST from the script.

2. The delay does not seem to depend on the length of the spoofed hostname. If I edit the DOS script above to make it a 10 char spoofed hostname, it still takes 6 seconds to run.

Wouldn't the correct code actually be:

header($request->server->get('SERVER_PROTOCOL') . ' 400 Bad Request');

rather than:

header($request->server->get('SERVER_PROTOCOL' . '400 Bad Request'));

?

(note the parentheses)

@David_Rothstein that's what happens when I code in comment boxes

We're actively working to remove exit from core, can this be done without adding new calls?

Ok, so we figured out two things:

a) To be vulnerable to this in 8.x, you need a sites.php (it can be empty), as otherwise, we don't do the multisite check.
b) Symfony does *not* include the same validation spectrum as https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup..., it does *not* validate the length or amounts of ./: in any way.

We can of course fix this just in Drupal, as the meaning of ./: is somewhat custom to Drupal/multisite behavior. But we should probably check with symfony just to be sure. There is no information on https://github.com/symfony/symfony/pull/11828 what exactly they are preventing from.

https://github.com/symfony/symfony/issues/12349

We could handle this in Drupal with a forward-port of drupal_valid_http_host() to a method on DrupalKernel, and call it before getHost(). If anyone knows of a better way, speak up!

@larowlan, et al. We will try not to introduce another exit(). BadRequestHttpException does some more processing and that later results in a fatal because the hostname isn't found. It may be possible to create a custom Response instead. I am hoping that would end execution, in order to avoid the fatal.

Better version of patch in #25 to account for HTTP running on something other than port 80, and HTTPS running on something other than port 443.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -314,7 +315,12 @@ public static function findSitePath(Request $request, $require_settings = TRUE)
   +      throw new BadRequestHttpException(t('Bad request'));
   
   @@ -810,7 +816,21 @@ protected function initializeRequestGlobals(Request $request) {
   +        throw new BadRequestHttpException(t('Bad request'));
   

   Exceptions should not use t()

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -810,7 +816,21 @@ protected function initializeRequestGlobals(Request $request) {
   +        $base_root = $http_protocol . '://' . $request->getHost();
   ...
   +      // Check for non-default port numbers, and append if needed.
   +      $port = $request->getPort();
   +      if ($http_protocol == 'https' && $port != 443) {
   +        $base_root .= ':' . $port;
   +      }
   +      elseif ($http_protocol == 'http' && $port != 80) {
   +        $base_root .= ':' . $port;
   +      }
   

   $request->getHttpHost() handles all of this logic for us.

Thanks for the input. Used $request->getSchemeAndHttpHost() to simplify things more. Also added the @throw links to the docblocks.

Because, Request::getHost() does not check for the number of periods or colons in the hostname, Drupal is still vulnerable without some more validation. @Berdir and I agree that we need to add a method to DrupalKernel, something like the following. This is my idea for the method, which is based on drupal_valid_http_host() from D7, but without the preg_match, which is redundant to getHost(). We would want to throw an exception in this method or in the calling code if this method does not return TRUE:

  /**
   * Validates a hostname length.
   *
   * @param string $host
   *   A hostname.
   *
   * @return
   *  TRUE if only containing valid characters, or FALSE otherwise.
   */
  protected static function validateHostname($host) {
    // Limit the length of the host name to 1000 bytes to prevent DoS attacks
    // with long host names.
    return 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;
  }

I'm going to start work on this based on the patch in #45.

@mpdonadio: this is an effort handle it at a low level. Sorry, I know you are working on this but I thought about it all day.

I don't think we even call drupal_bootstrap() anymore in the normal bootstrap?

I think it is OK if we do that check specifically before the multisite lookup, the DOS attack is specific do that, so it's OK to do the validation specific to that? Then we also don't have to call the validation method if there is no sites.php, as we don't really care about crazy hostnames then.

@Berdir - Thank you. More like this?

I mean ... this

Yes, something like that I think. Now we need to figure out how to test this, web test won't work, maybe a unit test? But that won't work as long as findSitePath() calls functions like drupal_valid_test_ua().

@klausi: Thank you for reviewing. This is just 3 and 4. @mpdonadio: Would you be kind enough to do 2?

OK, here is a first pass, but it is incomplete. There is a slight deviation from @cilefen and I discussed, but I think this is better.

There are two places we need to do the hostname checking, DrupalKernel::findSitePath (because of the multisite negotiation), and either ::preHandle or ::initializeRequestGlobals because the global $base_url gets set there, and getHost() gets called. I only modified the first.

We had originally thought about a new exception for the length problem, but that didn't make sense when I added the catch.

I attached three bash scripts. test1 has a long spoofed host, and will generate a proper 400 response. test2 has a spoofed Host under the character limit, and will generate a proper 200 (which is expected). test2 has an invalid spoofed host, and will generate a proper 400 response. For all three, you should have an empty sites/sites.php so the patch gets triggered.

Looking for input on two things.

1. Is this approach correct?

2. If so, is there a better way to generate the 400 that can avoid the exit.

If we think this approach is OK, then we need to figure out where to do the catch when there isn't a sites.php.

Setting Needs Review b/c I want to see the testbot output.

Why not do the bail out directly in findSitePath() since that's where the DoS is?

For $base_url I think that's not relevant here given #2221699: HTTP_HOST header cannot be trusted was bumped to critical.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -206,7 +207,13 @@ public static function createFromRequest(Request $request, $class_loader, $envir
   +      exit;
   

   So lower in this same function we call $response->prepare()->send() to redirect to install.php but don't call exit.
   Is this different because it's not a redirect?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1249,4 +1272,29 @@ protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
   +  }
   

   nitpick: missing new line between the }'s

@cilefen's latest patch w/ better handling for non-standard port numbers. interdiff-57-62.txt shows this.

interdiff-48-62.txt just shows what the differences to my approach are.

@larowlan, I assume #62 was looking at port_http_host_header-2304949-48.patch...

Yeah, the exit is there b/c without it Drupal went through the bootstrap process and rendered out a page, when all I wanted was a simple error code to go back to clients as this is an exceptional case. I see it an analogous to the fast 404 handling in Drupal 7. Is there a better way / place to catch this?

@catch, I assume #61 was to me, also. I did a try/catch in createFromRequest() instead of a direct abort from findSitePath() as it seemed the more better way to do it from a semantic perspective of initializing from a request object and getting that particular exception.

I am deferring to everyone else as to how to proceed here... And sorry for the confusion here; I must have had an old tab open and forgot to refresh when went to post my patch.

Working on this, starting from the patch in #63.

This adds the check per @klausi above, and attempts to add an event subscriber to generate a proper 400 Bad Request.

The exception doesn't get caught. Testing with AccessDeniedHttpException and NotFoundHttpException don't work either, so I think I am missing a key concept for how the event subscribers work.

Advice / guidance appreciated.

Summary from discussion on IRC: It has been proposed to move the check into an early middleware. However, the container is initialized after the site path has been determined and therefore a check at this point in time is not effective.

It follows that the HTTP_HOST header needs to be validated before the container is initialized, and consequently a response needs to be generated without the help of any container services.

Also note that Symfony Request::getHost() throws an UnexpectedValueException if validation fails. Therefore we probably should just do the same.

Here is a temporary patch that will give us a starting point for making tests. It's mostly evil, but it will generate a 400 for bad Host headers. Just settings Needs Review for testbot.

Additional notes from IRC:

core/rebuild.php doesn't go through DrupalKernel::createFromRequest(), so the hostname validation may need to be moved back to DrupalKernel::findSitePath() (it is protected by default). We should also audit the entry paths to make sure there aren't any other exceptions.

One idea to do this properly is to do the check and return a NullKernel with an empty handle() method instead of using exceptions. The same thing could be done with the install redirect.

There is no way we can implement a reliable web test here? a) this only works if you have a sites.php (Ok, if we do the validation earlier, then this is not needed, but there's still b)) and b) Only if you provide an invalid host name, which doesn't work for common vhost setups?

Patch rerolled.

I messed something up in #70 (I think I generated the patch w/o rebasing first). I am going to reroll starting from #67, possibly with two or more ideas about how to proceed.

I had thoughts about testing, but can't remember what they were. Maybe they will come back to me as I work on this...

Somewhere between port_http_host_header-2304949-62.patch and now, I think I missed a rebase when I generated a patch, and some extra code got in. The attached fixes this.

I think this is our best (least bad?) approach.

It generates a quick 400 with my test scripts, and it will do the detection for both multisite and normal.

The check is done in DrupalKernel.php::findSitePath() as that is the entry point from rebuild.php, should someone accidentally expose this. Otherwise, I think directly after the static::bootEnvironment(); in DrupalKernel.php::createFromRequest() would be more logical place.

The check throws BadRequestHttpException(), which is the Symfony 400, but index.php and rebuild.php catches HttpException. This should future proof things in case Symfony barfs before we have a kernel in place.

My questions are

-- Do we want this protection in rebuild.php? If not, I think moving the code to DrupalKernel.php::createFromRequest() is better.
-- Do we want this protection in authorize.php also? If so, we need the logic there.
-- Any ideas on if this is actually testable?

Apparently, I forgot to change the status...

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -293,12 +294,28 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE)
+    // @todo Adjust per resolution to https://github.com/symfony/symfony/issues/12349

We could consider making a pull request for this.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -293,12 +294,28 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE)
   +    // Validate the hostname.  $request->getHost can also throw a
   +    // UnexpectedValueException if it detects a bad hostname, but it does not
   +    // validate the length.
   +    // @todo Adjust per resolution to https://github.com/symfony/symfony/issues/12349
   +    try {
   +      $http_host = $request->getHost();
   +      if (self::validateHostnameLength($http_host) == FALSE) {
   +        throw new \UnexpectedValueException('Bad hostname');
   +      }
   +    }
   +    catch (\UnexpectedValueException $e) {
   +      throw new BadRequestHttpException('Bad hostname');
   +    }
   +
   

   I wonder whether we should move this code a little bit down, so that this validation check is done directly before using the host ... its a bit odd to have that before the simpletest check.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -809,8 +826,7 @@ protected function initializeRequestGlobals(Request $request) {
   -      $http_protocol = $request->isSecure() ? 'https' : 'http';
   -      $base_root = $http_protocol . '://' . $request->server->get('HTTP_HOST');
   +      $base_root = $request->getSchemeAndHttpHost();
   

   <3 but a little bit out of scope?

3. +++ b/core/rebuild.php
   @@ -25,7 +26,16 @@
   +  $code = $e->getStatusCode();
   +  http_response_code($code);
   +  print Response::$statusTexts[$code];
   +  exit;
   
   +++ b/index.php
   @@ -24,6 +26,11 @@
   +catch (HttpException $e) {
   +  $code = $e->getStatusCode();
   +  http_response_code($code);
   +  print Response::$statusTexts[$code];
   +}
   

   I think you should do rather create a response object and call $response->send() instead

Re #77

-- Do we want this protection in rebuild.php? If not, I think moving the code to DrupalKernel.php::createFromRequest() is better.
-- Do we want this protection in authorize.php also? If so, we need the logic there.
-- Any ideas on if this is actually testable?

Regarding whether or not rebuild.php should be protected or not, it was argued in IRC that this script should not be accessible in production anyway (because that would lead to all sorts of DoS opportunities). The rebuild script can be disabled by adding $settings['rebuild_access'] = FALSE; to settings.php. However, note that before being able to read settings, the rebuild script needs to invoke DrupalKernel::findSitePath() - a classic chicken-egg problem. Therefore it is necessary to protect rebuild.php.

Also note that findSitePath() is always called - even for sites which do not use the multisite functionality. It is the only way to actually find the proper settings.php. Therefore the concerns expressed against findSitePath() are moot IMHO (it seems to me that #56 is not so well-informed in this regard). Keeping it there will automatically protect authorize.php, rebuild.php and will work for virtually any crazy way to set up a working DrupalKernel.

Instead of putting the whole logic into findSitePath() it might be feasible to extract it to a public static validateRequest(Request $request) method in DrupalKernel[Interface] and then just invoke that method from within findSitePath(). This might also make it possible to cover that method with a unit-test, because it resolves the problem pointed out by #54 (i.e. a unit test simply could test validateRequest() instead of the whole findSitePath() method.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -892,9 +908,7 @@ protected function initializeCookieGlobals(Request $request) {
-      // HTTP_HOST can be modified by a visitor, but has been sanitized already
-      // in DrupalKernel::bootEnvironment().
-      if ($cookie_domain = $request->server->get('HTTP_HOST')) {
+      if ($cookie_domain = $request->getHost()) {
         // Strip leading periods, www., and port numbers from cookie domain.
         $cookie_domain = ltrim($cookie_domain, '.');
         if (strpos($cookie_domain, 'www.') === 0) {

getHost() always returns the hostname without the port number, thus remove the code which strips some lines further down.

Re #80

a little bit out of scope?

I don't think so. This comes from #23. In my understanding HTTP_HOST should not be accessed directly anywhere throughout our codebase. This patch removes all occurrences from production code.

OK, another patch...

#80.1: If this code is being called on every request don't we want to make sure it is also called for every simpletest request? That is why I put it at the top.

#80.2: As mentioned above, #23 made the change to not access HTTP_HOST directly. This change was introduced in #25 because the code below it needs the port number.

#80.3: I put this in. It may be overkill, but calling ->prepare() since we are returning a 4xx code may not be a bad idea.

#81.1: I made a public validateRequest() that returns a boolean. Added tests to DrupalKernelTest that i think cover all the bases, and they pass locally for me. Left the throw BadRequestHttpException in findSitePath().

#81.2: Removed port logic for from the cookie setup code.

Comment on validateHostnameLength(). This was based on the Drupal 7's drupal_valid_http_host(). These limits could possibly be tightened up per RFC1035, which says 255 octets for a hostname (that count includes the implicit trailing dot), and we may not need the colon detection since ->getHost doesn't seem to include that?

It would help to attach the files...

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -293,12 +294,18 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE)
   +    if (self::validateHostname($request) !== TRUE) {
   

   Use static:: instead of self::. See docs on late static binding.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -293,12 +294,18 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE)
   +    if (self::validateHostname($request) !== TRUE) {
   
   @@ -1249,4 +1253,50 @@ protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
   +      if (self::validateHostnameLength($http_host) == FALSE) {
   

   Use the same comparison operators in both places.

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -293,12 +294,18 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE)
   +      throw new BadRequestHttpException('Bad hostname');
   

   The message of the exception is never used, therefore it is not very useful to set it here.

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -892,16 +898,14 @@ protected function initializeCookieGlobals(Request $request) {
   +        // Restore one leading period per RFC 2109
   

   Do not mention that RFC here, it is outdated. In fact we add the leading dot in order to maximize browser-compatibility, see #2347877-17: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service. If in doubt do not add the comment in order to prevent bike-shedding about that.

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1249,4 +1253,50 @@ protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
   +  protected static function validateHostnameLength($host) {
   

   I tend to think that this could be inlined into the new validHostname function. On the other hand I also see the value in keeping it analogous to the D7 function. Other opinions?

6. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1249,4 +1253,50 @@ protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
   +      if (self::validateHostnameLength($http_host) == FALSE) {
   

   Use static.

7. +++ b/core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
   @@ -9,6 +9,7 @@
   +use Drupal\Core\Test\TestKernel;
   

   That does not seem to be used.

8. +++ b/core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
   @@ -153,4 +154,44 @@ public function testCompileDIC() {
   +  /**
   +   * Tests hostname validate.
   +   */
   +  public function testValidateHostame() {
   

   Great that this is finally testable. Note that it might be worthwhile to keep the test-scenarios inline with the ones from D7 in order to simplify maintenance. See BootstrapIPAddressTestCase::testIPAddressHost().

9. +++ b/index.php
   @@ -24,6 +26,12 @@
   +  $code = $e->getStatusCode();
   +  $content = Response::$statusTexts[$code];
   +  $response = new Response($content, $code);
   

   In the D7 version, the response has no body (i.e. there is no print in the respective part of drupal_environment_initialize(). Just use $response = new Response('', $e->getStatusCode());.

I think keeping validateHostnameLength() as a separate function makes the code read better, especially with the try/catch logic that is needed in validateHostname.

And before I post another version of the patch, keeping the validation in DrupalKernel::findSitePath() will protect index.php, rebuild.php, and authorize.php (our only entry points other than install.php, which doesn't need protection). We didn't fully decide whether we need to generate a 400 in rebuild.php and authorize.php. Do we need/want to generate a 400 from these when validation fails? The change would be similar to the attached.

OK, all of the feedback from #85 is done, but I left validateHostnameLength() as a separate function as I think this make the code read better.

Added try/catch logic to rebuild.php and authenticate.php so a 400 will be generated. I can back this out if desired.

Reverted core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest back to HEAD

Created \Drupal\Tests\Core\DrupalKernel\ValidateHostnameTest. Didn't see a better place to put unit tests for DrupalKernel. Mirrors the D7 version.

Tomorrow I'll make a followup issue about a generic request validation phase, and try to summarize it.

@klausi: A web test is impossible here, for the simple reason as that will never work in a vhost environment as you never get to the right vhost, AFAIK.

From Name-based Virtual Host Support:

With name-based virtual hosting, the server relies on the client to report the hostname as part of the HTTP headers. Using this technique, many different hosts can share the same IP address.

So, when we spoof the Host header in the test, we may never arrive at the proper virtual host. The only situation where this may work is if qa.drupal.org doesn't do name-based virtual hosting, but that would really complicate people running tests themselves and I have never seen a reliable way to detect from PHP whether name-based virtual hosting is actually being used.

This should address #92:1-6

+++ b/core/authorize.php
@@ -54,9 +54,16 @@ function authorize_access_allowed() {
+catch (HttpException $e) {

While manual testing I noted that the exception is not caught here. This is due to a missing use-statement. Same for rebuild.php. Also note that there is HttpExceptionInterface, I suggest type-hinting against that.

I tried to reproduce the results with the script from #24 for the following paths:

core/authorize.php
core/install.php
core/modules/statistics/statistics.php
core/rebuild.php
index.php

Note that statistics.php and install.php deliver a zero-length (no body) page with status code 500. I think this is okay.

Regrettably I do not see much of a difference between a proper request and a forged one for HEAD. I guess that's probably because I use an SSD. So maybe somebody with a good old hard-disk could run the test script against the new patch and post the results?

@klausi indeed.

I've slightly adapted the test-script and inserted a call to wget for each URL in order to have a baseline. The patch provides an effective protection for all front-controller scripts. Note the simpletest mock-frontcontrollers (http.php and https.php) are already protected by the call to drupal_valid_test_ua() in TestKernel::createFromRequest().

Attached is the modified dos script as well as the output produced by running it on my machine.

So, this is ready except for the missing use statements and the usage of HttpExceptionInterface instead of HttpException in the catch clauses (#93).

Replaced HttpException with HttpExceptionInterface.

There was a small conflict when I rebased this morning; interdiff is between my git branches after the manual conflict resolution.

+++ b/core/tests/Drupal/Tests/Core/DrupalKernel/ValidateHostnameTest.php
@@ -0,0 +1,53 @@
+  public function testValidateHostame() {
+    // Verifies that DrupalKernel::validateHostname() prevents invalid
+    // characters.
+    $this->assertFalse($this->validHttpHost('security/.drupal.org:80'), 'HTTP_HOST with / is invalid');
+    $this->assertFalse($this->validHttpHost('security\\.drupal.org:80'), 'HTTP_HOST with \\ is invalid');
+    $this->assertFalse($this->validHttpHost('security<.drupal.org:80'), 'HTTP_HOST with &lt; is invalid');
+    $this->assertFalse($this->validHttpHost('security..drupal.org:80'), 'HTTP_HOST with .. is invalid');
+    // Verifies that host names are shorter than 1000 characters.
+    $this->assertFalse($this->validHttpHost(str_repeat('x', 1001)), 'HTTP_HOST with more than 1000 characters is invalid.');
+    $this->assertFalse($this->validHttpHost(str_repeat('.', 101)), 'HTTP_HOST with more than 100 subdomains is invalid.');
+    $this->assertFalse($this->validHttpHost(str_repeat(':', 101)), 'HTTP_HOST with more than 100 portseparators is invalid.');
+  }
+
+  /**
+   * Validates a spoofed HTTP_HOST.
+   *
+   * @param $hostname
+   *   The hostname to use for the HTTP_HOST.
+   *
+   * @return bool
+   *   TRUE if the spoofed hostname is valid, FALSE otherwise.
+   */
+  protected function validHttpHost($hostname) {
+    $server = array('HTTP_HOST' => $hostname);
+    $request = new Request(array(), array(), array(), array(), array(), $server);
+    return DrupalKernel::validateHostname($request);
+  }

This should be done with a @dataProvider, no?

@timplunkett pretty much did this quicker than me when I asked about DataProviders in IRC. The patch is essentially his. He also added the coverage for returning TRUE, I added in the IPv6 loopback test from BootstrapIPAddressTestCase.

Setting Needs Review so TestBot runs this immediately.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1255,4 +1259,51 @@ protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
   +    if (static::validateHostnameLength($http_host) === FALSE) {
   +      return FALSE;
   +    }
   ...
   +    return TRUE;
   

   you could also just do a return static::validateHostnameLength($http_host) and be done with it

2. +++ b/core/rebuild.php
   @@ -25,7 +27,15 @@
   +  $response = new Response('', $e->getStatusCode());
   +  $response->prepare($request)->send();
   
   +++ b/index.php
   @@ -24,6 +26,10 @@
   +  $response = new Response('', $e->getStatusCode());
   +  $response->prepare($request)->send();
   

   Are we 100% sure that we don't want to use $e->getMessage()?

#100-1, I left it that was for a cleaner patch for when/if Synfony handles the hostname length internally. I change this if desired.

#100-2, in #88 we removed the exception message, which may or may not line up with the status code we are sending back.

$code = $e->getStatusCode();
$content = Response::$statusTexts[$code];
$response = new Response($content, $code);
$response->prepare($request)->send();

This would just probably be better if we want content.

Are we 100% sure that we don't want to use $e->getMessage()?

We do not want to emit a body because if we did we'd need to take extra-care with escaping it and whatnot. The status text (Bad request) is already in the status header.

+++ b/core/tests/Drupal/Tests/Core/DrupalKernel/ValidateHostnameTest.php
@@ -0,0 +1,57 @@
+    $data[] = [str_repeat('.', 101), 'HTTP_HOST with more than 100 subdomains is invalid.'];

This test is not testing what you think it is.

$server = ['HTTP_HOST' => str_repeat('.', 101)];
$request = new Request([], [], [], [], [], $server);
$request->getHost();

Throws an invalid value exception. Something like ['HTTP_HOST' => str_repeat('a.', 101)]; is what you want.

Per some IRC with @klausi and @alexpott, I am going to rework the tests to diverge from the exact test cases from drupal_valid_http_host() in the Drupal 7 BootstrapIPAddressTestCase::testIPAddressHost, and to the cover the Drupal 8 cases better.

There are two parts of this. One are the bad hostname lengths that we check for in DrupalKernel::validateHostnameLength (which are the real cause of our DOS issues). The others are the exceptions that Request::getHost() will throw.

We are currently testing for one case, which are characters outside of what is defined by RFC 952/2181.

We are not testing the trusted hosts mechanism that the class defines. AFAIK, we don't support this in 8.0.x, but we may want to in the future. Should I add test coverage for these too, while I am at it?

Adjusted tests based on some IRC w/ @klausi and @alexpott.

-- Updated comment to match what Request->getHost() checks for (which is the exception that gets caught in ::validateHostname()
-- Updated the second batch of tests to better reflect real hostnames that we consider invalid in ::validateHostnameLength()
-- Added two tests for valid IPv4 and IPv6 hostnames to make sure they OK with these two methods.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 62ee12b and pushed to 8.0.x. Thanks!

PhpStorm now rightfully complains:
The variable 'request' might have not been defined.

+try {
+  $request = Request::createFromGlobals();
+  $kernel = DrupalKernel::createFromRequest($request, $autoloader, 'prod');
+  $kernel->prepareLegacyRequest($request);
+}
+catch (HttpExceptionInterface $e) {
+  $response = new Response('', $e->getStatusCode());
+  $response->prepare($request)->send();
+  exit;
+}

The IDE is always right.
What to do?

We could fix it like this:

$request = Request::createFromGlobals();
try {
  $kernel = DrupalKernel::createFromRequest($request, $autoloader, 'prod');
  $kernel->prepareLegacyRequest($request);
}
catch (HttpExceptionInterface $e) {
  $response = new Response('', $e->getStatusCode());
  $response->prepare($request)->send();
  exit;
}

It an exception is thrown in Request::createFromGlobals(), we will not catch it. Is this ok?
Maybe Request::createFromGlobals() never throws HttpExceptionInterface kind of exceptions. But it is always nicer to have IDE-proof logic.

@donquixote: Please bring that up in #2389811: Move all the logic out of index.php (again).

Cleaning up the issues that I am assigned to...

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1255,4 +1259,51 @@ protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
+   * @todo Adjust per resolution to https://github.com/symfony/symfony/issues/12349

This has been closed now. How can we remove this @todo now?

#2533218: Resolve todo for Symfony #12349 created.