From 98926e4e6c26d1d43bb1faf516d15bdb2739556e Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Tue, 28 Apr 2020 16:29:41 +1200 Subject: [PATCH] [CVE-2019-19326] Stop honouring X-HTTP-Method-Override header, X-Original-Url header and _method POST variable. Add SS_HTTPRequest::setHttpMethod(). --- control/Director.php | 4 +- control/HTTPRequest.php | 43 +++++++++----- main.php | 5 -- tests/FakeController.php | 7 +-- tests/control/HTTPRequestTest.php | 97 +++++++++++++++++++++++++------ 5 files changed, 110 insertions(+), 46 deletions(-) diff --git a/control/Director.php b/control/Director.php index 3f62ecc39c7..f5a10a9c4d4 100644 --- a/control/Director.php +++ b/control/Director.php @@ -117,9 +117,7 @@ public static function direct($url, DataModel $model) { } $req = new SS_HTTPRequest( - (isset($_SERVER['X-HTTP-Method-Override'])) - ? $_SERVER['X-HTTP-Method-Override'] - : $_SERVER['REQUEST_METHOD'], + $_SERVER['REQUEST_METHOD'], $url, $_GET, ArrayLib::array_merge_recursive((array) $_POST, (array) $_FILES), diff --git a/control/HTTPRequest.php b/control/HTTPRequest.php index a5eefc4ce39..536ad2353b4 100644 --- a/control/HTTPRequest.php +++ b/control/HTTPRequest.php @@ -11,9 +11,6 @@ * match() to get the information that they need out of the URL. This is generally handled by * {@link RequestHandler::handleRequest()}. * - * @todo Accept X_HTTP_METHOD_OVERRIDE http header and $_REQUEST['_method'] to override request types (useful for - * webclients not supporting PUT and DELETE) - * * @package framework * @subpackage control */ @@ -106,7 +103,7 @@ class SS_HTTPRequest implements ArrayAccess { * Construct a SS_HTTPRequest from a URL relative to the site root. */ public function __construct($httpMethod, $url, $getVars = array(), $postVars = array(), $body = null) { - $this->httpMethod = strtoupper(self::detect_method($httpMethod, $postVars)); + $this->httpMethod = strtoupper($httpMethod); $this->setUrl($url); $this->getVars = (array) $getVars; @@ -726,24 +723,40 @@ public function httpMethod() { } /** - * Gets the "real" HTTP method for a request. - * - * Used to work around browser limitations of form - * submissions to GET and POST, by overriding the HTTP method - * with a POST parameter called "_method" for PUT, DELETE, HEAD. - * Using GET for the "_method" override is not supported, - * as GET should never carry out state changes. - * Alternatively you can use a custom HTTP header 'X-HTTP-Method-Override' - * to override the original method in {@link Director::direct()}. - * The '_method' POST parameter overrules the custom HTTP header. + * Explicitly set the HTTP method for this request. + * @param string $method + * @return $this + */ + public function setHttpMethod($method) { + if(!self::isValidHttpMethod($method)) { + user_error('SS_HTTPRequest::setHttpMethod: Invalid HTTP method', E_USER_ERROR); + } + + $this->httpMethod = strtoupper($method); + return $this; + } + + /** + * @param string $method + * @return bool + */ + private static function isValidHttpMethod($method) { + return in_array(strtoupper($method), array('GET','POST','PUT','DELETE','HEAD')); + } + + /** + * Gets the "real" HTTP method for a request. This method is no longer used to mitigate the risk of web cache + * poisoning. * + * @see https://www.silverstripe.org/download/security-releases/CVE-2019-19326 * @param string $origMethod Original HTTP method from the browser request * @param array $postVars * @return string HTTP method (all uppercase) + * @deprecated 3.7.5 */ public static function detect_method($origMethod, $postVars) { if(isset($postVars['_method'])) { - if(!in_array(strtoupper($postVars['_method']), array('GET','POST','PUT','DELETE','HEAD'))) { + if (!self::isValidHttpMethod($postVars['_method'])) { user_error('Director::direct(): Invalid "_method" parameter', E_USER_ERROR); } return strtoupper($postVars['_method']); diff --git a/main.php b/main.php index 4a56ca1e0cb..e0f2fc04e6c 100644 --- a/main.php +++ b/main.php @@ -60,11 +60,6 @@ // we handle our own cache headers in this application session_cache_limiter(''); -// IIS will sometimes generate this. -if(!empty($_SERVER['HTTP_X_ORIGINAL_URL'])) { - $_SERVER['REQUEST_URI'] = $_SERVER['HTTP_X_ORIGINAL_URL']; -} - // Enable the entity loader to be able to load XML in Zend_Locale_Data libxml_disable_entity_loader(false); diff --git a/tests/FakeController.php b/tests/FakeController.php index b54af6a6edc..4489cd471a4 100644 --- a/tests/FakeController.php +++ b/tests/FakeController.php @@ -10,12 +10,7 @@ public function __construct() { $this->pushCurrent(); - $request = new SS_HTTPRequest( - (isset($_SERVER['X-HTTP-Method-Override'])) - ? $_SERVER['X-HTTP-Method-Override'] - : $_SERVER['REQUEST_METHOD'], - '/' - ); + $request = new SS_HTTPRequest($_SERVER['REQUEST_METHOD'], '/'); $this->setRequest($request); $this->setResponse(new SS_HTTPResponse()); diff --git a/tests/control/HTTPRequestTest.php b/tests/control/HTTPRequestTest.php index e1b1fc3cafd..58dfb999ffe 100644 --- a/tests/control/HTTPRequestTest.php +++ b/tests/control/HTTPRequestTest.php @@ -51,8 +51,12 @@ public function testHttpMethodOverrides() { array('_method' => 'DELETE') ); $this->assertTrue( + $request->isPOST(), + '_method override is no longer honored.' + ); + $this->assertFalse( $request->isDELETE(), - 'POST with valid method override to DELETE' + 'DELETE _method override is not honored.' ); $request = new SS_HTTPRequest( @@ -61,9 +65,9 @@ public function testHttpMethodOverrides() { array(), array('_method' => 'put') ); - $this->assertTrue( + $this->assertFalse( $request->isPUT(), - 'POST with valid method override to PUT' + 'PUT _method override is not honored.' ); $request = new SS_HTTPRequest( @@ -72,33 +76,92 @@ public function testHttpMethodOverrides() { array(), array('_method' => 'head') ); - $this->assertTrue( + $this->assertFalse( $request->isHEAD(), - 'POST with valid method override to HEAD ' + 'HEAD _method override is not honored.' ); $request = new SS_HTTPRequest( 'POST', 'admin/crm', - array(), - array('_method' => 'head') + array('_method' => 'delete') ); - $this->assertTrue( - $request->isHEAD(), - 'POST with valid method override to HEAD' + $this->assertFalse( + $request->isDELETE(), + 'DELETE _method override is not honored.' ); + } - $request = new SS_HTTPRequest( - 'POST', - 'admin/crm', - array('_method' => 'head') + public function detectMethodDataProvider() + { + return array( + 'Plain POST request' => array('POST', array(), 'POST'), + 'Plain GET request' => array('GET', array(), 'GET'), + 'Plain DELETE request' => array('DELETE', array(), 'DELETE'), + 'Plain PUT request' => array('PUT', array(), 'PUT'), + 'Plain HEAD request' => array('HEAD', array(), 'HEAD'), + + 'Request with GET method override' => array('POST', array('_method' => 'GET'), 'GET'), + 'Request with HEAD method override' => array('POST', array('_method' => 'HEAD'), 'HEAD'), + 'Request with DELETE method override' => array('POST', array('_method' => 'DELETE'), 'DELETE'), + 'Request with PUT method override' => array('POST', array('_method' => 'PUT'), 'PUT'), + 'Request with POST method override' => array('POST', array('_method' => 'POST'), 'POST'), + + 'Request with mixed case method override' => array('POST', array('_method' => 'gEt'), 'GET'), ); - $this->assertTrue( - $request->isPOST(), - 'POST with invalid method override by GET parameters to HEAD' + } + + + /** + * @dataProvider detectMethodDataProvider + */ + public function testDetectMethod($realMethod, $post, $expected) + { + $actual = SS_HTTPRequest::detect_method($realMethod, $post); + $this->assertEquals($expected, $actual); + } + + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testBadDetectMethod() + { + SS_HTTPRequest::detect_method('POST', array('_method' => 'Boom')); + } + + public function setHttpMethodDataProvider() + { + return array( + 'POST request' => array('POST','POST'), + 'GET request' => array('GET', 'GET'), + 'DELETE request' => array('DELETE', 'DELETE'), + 'PUT request' => array('PUT', 'PUT'), + 'HEAD request' => array('HEAD', 'HEAD'), + 'Mixed case POST' => array('gEt', 'GET'), ); } + /** + * @dataProvider setHttpMethodDataProvider + */ + public function testSetHttpMethod($method, $expected) + { + $request = new SS_HTTPRequest('GET', '/hello'); + $returnedRequest = $request->setHttpMethod($method); + $this->assertEquals($expected, $request->httpMethod()); + $this->assertEquals($request, $returnedRequest); + } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testBadSetHttpMethod() + { + $request = new SS_HTTPRequest('GET', '/hello'); + $request->setHttpMethod('boom'); + } + public function testRequestVars() { $getVars = array( 'first' => 'a',