From 75137dbab28c0efd28b07e50044a50c5af4e46aa Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 25 May 2015 14:31:20 +1200 Subject: [PATCH] Ensure only trusted proxy servers have control over certain HTTP headers --- control/Director.php | 6 ++- control/HTTPRequest.php | 4 +- core/Constants.php | 31 +++++++++++++- core/startup/ParameterConfirmationToken.php | 6 ++- .../09_Security/04_Secure_Coding.md | 41 +++++++++++++++++++ 5 files changed, 81 insertions(+), 7 deletions(-) diff --git a/control/Director.php b/control/Director.php index 7651ec2ef..07948d631 100644 --- a/control/Director.php +++ b/control/Director.php @@ -495,14 +495,16 @@ class Director implements TemplateGlobalProvider { if ($protocol = Config::inst()->get('Director', 'alternate_protocol')) { $return = ($protocol == 'https'); } else if( - isset($_SERVER['HTTP_X_FORWARDED_PROTO']) + TRUSTED_PROXY + && isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTO']) == 'https' ) { // Convention for (non-standard) proxy signaling a HTTPS forward, // see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields $return = true; } else if( - isset($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) + TRUSTED_PROXY + && isset($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) == 'https' ) { // Less conventional proxy header diff --git a/control/HTTPRequest.php b/control/HTTPRequest.php index 4ccef56d3..9b8506baa 100644 --- a/control/HTTPRequest.php +++ b/control/HTTPRequest.php @@ -653,10 +653,10 @@ class SS_HTTPRequest implements ArrayAccess { * @return string */ public function getIP() { - if (!empty($_SERVER['HTTP_CLIENT_IP'])) { + if (TRUSTED_PROXY && !empty($_SERVER['HTTP_CLIENT_IP'])) { //check ip from share internet return $_SERVER['HTTP_CLIENT_IP']; - } elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { + } elseif (TRUSTED_PROXY && !empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { //to check ip is pass from proxy return $_SERVER['HTTP_X_FORWARDED_FOR']; } elseif(isset($_SERVER['REMOTE_ADDR'])) { diff --git a/core/Constants.php b/core/Constants.php index 574773fc9..b971764c2 100644 --- a/core/Constants.php +++ b/core/Constants.php @@ -23,6 +23,8 @@ * - FRAMEWORK_ADMIN_PATH: Absolute filepath, e.g. "/var/www/my-webroot/framework/admin" * - THIRDPARTY_DIR: Path relative to webroot, e.g. "framework/thirdparty" * - THIRDPARTY_PATH: Absolute filepath, e.g. "/var/www/my-webroot/framework/thirdparty" + * - TRUSTED_PROXY: true or false, depending on whether the X-Forwarded-* HTTP + * headers from the given client are trustworthy (e.g. from a reverse proxy). * * @package framework * @subpackage core @@ -84,6 +86,32 @@ function stripslashes_recursively(&$array) { } } +/** + * Validate whether the request comes directly from a trusted server or not + * This is necessary to validate whether or not the values of X-Forwarded- + * or Client-IP HTTP headers can be trusted + */ +if(!defined('TRUSTED_PROXY')) { + $trusted = true; // will be false by default in a future release + + if(getenv('BlockUntrustedIPs') || defined('SS_TRUSTED_PROXY_IPS')) { + $trusted = false; + + if(defined('SS_TRUSTED_PROXY_IPS') && SS_TRUSTED_PROXY_IPS !== 'none') { + if(SS_TRUSTED_PROXY_IPS === '*') { + $trusted = true; + } elseif(isset($_SERVER['REMOTE_ADDR'])) { + $trusted = in_array($_SERVER['REMOTE_ADDR'], explode(',', SS_TRUSTED_PROXY_IPS)); + } + } + } + + /** + * Declare whether or not the connecting server is a trusted proxy + */ + define('TRUSTED_PROXY', $trusted); +} + /** * A blank HTTP_HOST value is used to detect command-line execution. * We update the $_SERVER variable to contain data consistent with the rest of the application. @@ -146,7 +174,8 @@ if(!isset($_SERVER['HTTP_HOST'])) { /** * Fix HTTP_HOST from reverse proxies */ - if (isset($_SERVER['HTTP_X_FORWARDED_HOST'])) { + if (TRUSTED_PROXY && isset($_SERVER['HTTP_X_FORWARDED_HOST'])) { + // Get the first host, in case there's multiple separated through commas $_SERVER['HTTP_HOST'] = strtok($_SERVER['HTTP_X_FORWARDED_HOST'], ','); } diff --git a/core/startup/ParameterConfirmationToken.php b/core/startup/ParameterConfirmationToken.php index 35eb88a70..772d40cdf 100644 --- a/core/startup/ParameterConfirmationToken.php +++ b/core/startup/ParameterConfirmationToken.php @@ -114,14 +114,16 @@ class ParameterConfirmationToken { // Are we http or https? Replicates Director::is_https() without its dependencies/ $proto = 'http'; if( - isset($_SERVER['HTTP_X_FORWARDED_PROTO']) + TRUSTED_PROXY + && isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTO']) == 'https' ) { // Convention for (non-standard) proxy signaling a HTTPS forward, // see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields $proto = 'https'; } else if( - isset($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) + TRUSTED_PROXY + && isset($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) == 'https' ) { // Less conventional proxy header diff --git a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md b/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md index 3694548b4..4f9bd8f8f 100644 --- a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md +++ b/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md @@ -474,6 +474,47 @@ This is a recommended option to secure any controller which displays or submits sensitive user input, and is enabled by default in all CMS controllers, as well as the login form. +## Request hostname forgery + +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 +used by attackers to fool the server into mistaking its own identity. + +The risk of this kind of attack causing damage is especially high on sites which utilise caching +mechanisms, as rewritten urls could persist between requests in order to misdirect other users +into visiting external sites. + +In order to prevent this kind of attack, it's necessary to whitelist trusted proxy +server IPs using the SS_TRUSTED_PROXY_IPS define in your _ss_environment.php. + + + :::php + define('SS_TRUSTED_PROXY_IPS', '127.0.0.1,192.168.0.1'); + + +If there is no proxy server, 'none' can be used to distrust all clients. +If only trusted servers will make requests then you can use '*' to trust all clients. +Otherwise a comma separated list of individual IP addresses should be declared. + +This behaviour is enabled whenever SS_TRUSTED_PROXY_IPS is defined, or if the +`BlockUntrustedProxyHeaders` environment variable is declared. From 3.1.13 onwards +this environment variable is included in the installer by default. + + + + # Ensure that X-Forwarded-Host is only allowed to determine the request + # hostname for servers ips defined by SS_TRUSTED_PROXY_IPS in your _ss_environment.php + # Note that in a future release this setting will be always on. + SetEnv BlockUntrustedProxyHeaders true + + + +In a future release this behaviour will be changed to be on by default, and this environment +variable will be no longer necessary, thus it will be necessary to always set +SS_TRUSTED_PROXY_IPS if using a proxy. + + ## Related * [http://silverstripe.org/security-releases/](http://silverstripe.org/security-releases/)