tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

[CVE-2019-19326] Stop honouring X-HTTP-Method-Override header, X-Original-Url header and _method POST variable. Add SS_HTTPRequest::setHttpMethod().

Browse Source
This commit is contained in:
Maxime Rainville 2020-04-28 16:29:41 +12:00
parent d3b23e7024
commit 98926e4e6c
5 changed files with 110 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
control/Director.php
View File

@ -117,9 +117,7 @@ class Director implements TemplateGlobalProvider {
}
$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),

43
control/HTTPRequest.php
View File

@ -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 @@ class SS_HTTPRequest implements ArrayAccess {
}
/**
* 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']);

5
main.php
View File

@ -60,11 +60,6 @@ require_once('core/Constants.php');
// 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);

7
tests/FakeController.php
View File

@ -10,12 +10,7 @@ class FakeController extends Controller {
$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());

97
tests/control/HTTPRequestTest.php
View File

@ -51,8 +51,12 @@ class HTTPRequestTest extends SapphireTest {
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 @@ class HTTPRequestTest extends SapphireTest {
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 @@ class HTTPRequestTest extends SapphireTest {
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',