Skip to content

Commit

Permalink
[CVE-2019-19326] Stop honouring X-HTTP-Method-Override header, X-Orig…
Browse files Browse the repository at this point in the history 
…inal-Url header and _method POST variable. Add SS_HTTPRequest::setHttpMethod()
  • Loading branch information
Maxime Rainville authored and Garion Herman committed Jul 9, 2020
1 parent 01f93ef commit 107706c
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 49 deletions.
45 changes: 30 additions & 15 deletions src/Control/HTTPRequest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@
* The intention is that a single HTTPRequest object can be passed from one object to another, each object calling
* 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)
*/
class HTTPRequest implements ArrayAccess
{
Expand Down Expand Up @@ -156,7 +153,7 @@ class HTTPRequest implements ArrayAccess
*/
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;
$this->postVars = (array) $postVars;
Expand Down Expand Up @@ -830,6 +827,21 @@ public function httpMethod()
return $this->httpMethod;
}

/**
* Explicitly set the HTTP method for this request.
* @param string $method
* @return $this
*/
public function setHttpMethod($method)
{
if (!self::isValidHttpMethod($method)) {
user_error('HTTPRequest::setHttpMethod: Invalid HTTP method', E_USER_ERROR);
}

$this->httpMethod = strtoupper($method);
return $this;
}

/**
* Return the URL scheme (e.g. "http" or "https").
* Equivalent to PSR-7 getUri()->getScheme()
Expand All @@ -855,25 +867,28 @@ public function setScheme($scheme)
}

/**
* 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.
* The '_method' POST parameter overrules the custom HTTP header.
* @param string $method
* @return bool
*/
private static function isValidHttpMethod($method)
{
return in_array(strtoupper($method), ['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 4.4.7
*/
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('HTTPRequest::detect_method(): Invalid "_method" parameter', E_USER_ERROR);
}
return strtoupper($postVars['_method']);
Expand Down
10 changes: 0 additions & 10 deletions src/Control/HTTPRequestBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,6 @@ public static function extractRequestHeaders(array $server)
*/
public static function cleanEnvironment(array $variables)
{
// IIS will sometimes generate this.
if (!empty($variables['_SERVER']['HTTP_X_ORIGINAL_URL'])) {
$variables['_SERVER']['REQUEST_URI'] = $variables['_SERVER']['HTTP_X_ORIGINAL_URL'];
}

// Override REQUEST_METHOD
if (isset($variables['_SERVER']['X-HTTP-Method-Override'])) {
$variables['_SERVER']['REQUEST_METHOD'] = $variables['_SERVER']['X-HTTP-Method-Override'];
}

// Merge $_FILES into $_POST
$variables['_POST'] = array_merge((array)$variables['_POST'], (array)$variables['_FILES']);

Expand Down
101 changes: 77 additions & 24 deletions tests/php/Control/HTTPRequestTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,15 @@ public function testHttpMethodOverrides()
array(),
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 HTTPRequest(
Expand All @@ -71,9 +77,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 HTTPRequest(
Expand All @@ -82,31 +88,78 @@ 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 HTTPRequest(
'POST',
'admin/crm',
array(),
array('_method' => 'head')
);
$this->assertTrue(
$request->isHEAD(),
'POST with valid method override to HEAD'
);
public function detectMethodDataProvider()
{
return [
'Plain POST request' => ['POST', [], 'POST'],
'Plain GET request' => ['GET', [], 'GET'],
'Plain DELETE request' => ['DELETE', [], 'DELETE'],
'Plain PUT request' => ['PUT', [], 'PUT'],
'Plain HEAD request' => ['HEAD', [], 'HEAD'],

$request = new HTTPRequest(
'POST',
'admin/crm',
array('_method' => 'head')
);
$this->assertTrue(
$request->isPOST(),
'POST with invalid method override by GET parameters to HEAD'
);
'Request with GET method override' => ['POST', ['_method' => 'GET'], 'GET'],
'Request with HEAD method override' => ['POST', ['_method' => 'HEAD'], 'HEAD'],
'Request with DELETE method override' => ['POST', ['_method' => 'DELETE'], 'DELETE'],
'Request with PUT method override' => ['POST', ['_method' => 'PUT'], 'PUT'],
'Request with POST method override' => ['POST', ['_method' => 'POST'], 'POST'],

'Request with mixed case method override' => ['POST', ['_method' => 'gEt'], 'GET']
];
}

/**
* @dataProvider detectMethodDataProvider
*/
public function testDetectMethod($realMethod, $post, $expected)
{
$actual = HTTPRequest::detect_method($realMethod, $post);
$this->assertEquals($expected, $actual);
}

/**
* @expectedException PHPUnit_Framework_Error
*/
public function testBadDetectMethod()
{
HTTPRequest::detect_method('POST', ['_method' => 'Boom']);
}

public function setHttpMethodDataProvider()
{
return [
'POST request' => ['POST','POST'],
'GET request' => ['GET', 'GET'],
'DELETE request' => ['DELETE', 'DELETE'],
'PUT request' => ['PUT', 'PUT'],
'HEAD request' => ['HEAD', 'HEAD'],
'Mixed case POST' => ['gEt', 'GET'],
];
}

/**
* @dataProvider setHttpMethodDataProvider
*/
public function testSetHttpMethod($method, $expected)
{
$request = new 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 HTTPRequest('GET', '/hello');
$request->setHttpMethod('boom');
}

public function testRequestVars()
Expand Down

0 comments on commit 107706c

Please # to comment.