Skip to content

Commit

Permalink
FIX Allow users to specify allowed hosts
Browse files Browse the repository at this point in the history
Allow users to explicitly state which Hosts are allowed to be requested via
this application instance to avoid Host: header forgery attacks.
  • Loading branch information
Marcus Nyeholt committed May 28, 2015
1 parent b21fd84 commit 9c8fa51
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 0 deletions.
8 changes: 8 additions & 0 deletions core/Constants.php
Expand Up @@ -181,6 +181,14 @@ function stripslashes_recursively(&$array) {
}
}

if (defined('SS_ALLOWED_HOSTS')) {
$all_allowed_hosts = explode(',', SS_ALLOWED_HOSTS);
if (!in_array($_SERVER['HTTP_HOST'], $all_allowed_hosts)) {

This comment has been minimized.

Copy link
@kinglozzer

kinglozzer May 28, 2015

Member

A few questions:

  • Do we need an isset($_SERVER['HTTP_HOST']) here?
  • Does this work with command-line, or do people have to conditionally define this constant in their environment file?
  • Is this not better solved at an .htaccess level?
  • Should this have been part of the 3.0 release as well?

This comment has been minimized.

Copy link
@dhensby

dhensby Jun 11, 2015

Contributor

bump how thoroughly has this been tested under the circumstances outlined by @kinglozzer ?

This comment has been minimized.

Copy link
@dhensby

dhensby Jul 7, 2015

Contributor

@kinglozzer do you want to do some investigation into this?

This comment has been minimized.

Copy link
@kinglozzer

kinglozzer Jul 7, 2015

Member

@dhensby Well, I’m pretty sure we need the isset($_SERVER['HTTP_HOST']), I don’t really know what we should do in the event it’s not present though. How big a security risk does this pose? Should we need to block all requests without an HTTP_HOST set?

Just above this code block we try to extract a URL from $_FILE_TO_URL_MAPPING, if that’s not set, but SS_ALLOWED_HOSTS is defined, then all CLI requests will fail with a 400 error I think? Does sake require $_FILE_TO_URL_MAPPING?

Ping @nyeholt @tractorcow, I feel like you guys may have the answers here :)

This comment has been minimized.

Copy link
@dhensby

dhensby Jul 7, 2015

Contributor

I imagine the first check should be are we cli, then we check the host.

The cli does throw errors if mapping isn't set, but ultimately this protection is not required for cli requests

This comment has been minimized.

Copy link
@tractorcow

tractorcow Jul 7, 2015

Contributor

Fix at #4379

header('HTTP/1.1 400 Invalid Host', true, 400);
die();
}
}

/**
* Define system paths
*/
Expand Down
11 changes: 11 additions & 0 deletions docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md
Expand Up @@ -476,6 +476,17 @@ as well as the login form.

## Request hostname forgery

To prevent a forged hostname appearing being used by the application, SilverStripe
allows the configure of a whitelist of hosts that are allowed to access the system. By defining
this whitelist in your _ss_environment.php file, any request presenting a `Host` header that is
_not_ in this list will be blocked with a HTTP 400 error:

:::php
define('SS_ALLOWED_HOSTS', 'www.mysite.com,mysite.com,subdomain.mysite.com');

Please note that if this configuration is defined, you _must_ include _all_ subdomains (eg www.)
that will be accessing the site.

When SilverStripe is run behind a reverse proxy, it's normally necessary for this proxy to
use the `X-Forwarded-Host` request header to tell the webserver which hostname was originally
requested. However, when SilverStripe is not run behind a proxy, this header can still be
Expand Down

0 comments on commit 9c8fa51

Please sign in to comment.