Merge branch '3.7' into 3

silverstripe · Jul 14, 2020 · bc9e384 · bc9e384
2 parents f8311c9 + f2b8946
commit bc9e384
Show file tree

Hide file tree

Showing 10 changed files with 209 additions and 50 deletions.
diff --git 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
@@ -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/docs/en/04_Changelogs/3.7.4.md b/docs/en/04_Changelogs/3.7.4.md
@@ -38,4 +38,4 @@ This fork is not a supported module and SilverStripe does not commit to maintain
  * 2019-02-25 [adbc560bd](https://github.com/silverstripe/silverstripe-framework/commit/adbc560bd70ba2e071f94a41a084768819196ee7) Address PR feedback. (Maxime Rainville)
  * 2019-02-21 [4ec1a682c](https://github.com/silverstripe/silverstripe-framework/commit/4ec1a682cf354e2425ef4fd6598c7de8e807bcc7) Renable the ability to do dynamic assignment with DBField (Maxime Rainville)
  * 2019-02-19 [ab5f09a9f](https://github.com/silverstripe/silverstripe-framework/commit/ab5f09a9f3ec12333c748dd68bfc504b5e509bfc) Updated unit test were targeting Float/Int which don't exist on PHP7 (#8810) (Maxime Rainville)
-<!--- Changes above this line will be automatically regenerated -->
+<!--- Changes above this line will be automatically regenerated -->
diff --git a/docs/en/04_Changelogs/3.7.5.md b/docs/en/04_Changelogs/3.7.5.md
@@ -0,0 +1,95 @@
+# 3.7.5
+
+* [CVE-2019-19326 Web Cache Poisoning](#CVE-2019-19326)
+* [CVE-2020-9311 Malicious user profile information can cause login form XSS](#CVE-2020-9311)
+
+## CVE-2019-19326 Web Cache Poisoning {#CVE-2019-19326}
+
+Silverstripe sites using HTTP cache headers and HTTP caching proxies (e.g. CDNs) can be susceptible to web cache poisoning through the:
+* `X-Original-Url` HTTP header
+* `X-HTTP-Method-Override` HTTP header
+* `_method` POST variable.
+
+In order to remedy this vulnerability, Silverstripe Framework 3.7.5 removes native support for these features. While this is technically a semantic versioning breakage, these features are inherently insecure and date back to a time when browsers didn't natively support the full range of HTTP methods. Sites who still require these features will have highly unusual requirements that are best served by a tailored solution.
+
+### Re-enabling the support for removed features
+
+These features are best implemented by defining a `RequestFilter`. Request Filters are similar to the more modern concept of "middleware" as defined by the PSR-15 standard and supported by Silverstripe 4.
+
+The following example illustrate how to implement a `RequestFilter` that restore support for the `X-Original-Url` header and the `_method` POST parameter for request originating from a trusted proxy.
+
+```php
+<?php
+
+/**
+ * This is meant to illustrate how to implement a RequestFilter. It assumes your
+ * trusted proxy will strip the insecure data from any requests. If you blindly
+ * copy-paste this in in your code base, you'll simply replicate the vulnerability.
+ */
+class InsecureRequestProcessor implements RequestFilter
+{
+
+    public function preRequest(SS_HTTPRequest $request, Session $session, DataModel $model)
+    {
+        if (TRUSTED_PROXY) {
+            $originalUrl = $request->getHeader('X-Original-Url');
+            if ($originalUrl) {
+                $request->setUrl($originalUrl);
+                $_SERVER['REQUEST_URI'] = $originalUrl;
+            }
+
+            $methodOverride = $request->postVar('_method');
+            $validMethods = ['GET', 'POST', 'PUT', 'DELETE', 'HEAD'];
+            if ($methodOverride && in_array(strtoupper($methodOverride), $validMethods)) {
+                $request->setMethod($methodOverride);
+            }
+        }
+
+        return true;
+    }
+
+    public function postRequest(SS_HTTPRequest $request, SS_HTTPResponse $response, DataModel $model)
+    {
+        return true;
+    }
+}
+```
+
+To learn more about re-implementing support for the disabled features:
+* read [How to implement a Request Filter](/developer_guides/controllers/requestfilters) on the Silverstripe documentation
+* read [how to configure trusted proxies](/developer_guides/security/secure_coding/#request-hostname-forgery) on the Silverstripe documentation
+* review [api:RequestFilter] interface
+
+To learn more about middleware:
+* read the [PSR-15: HTTP Server Request Handlers](https://www.php-fig.org/psr/psr-15/) standard
+* read the [Silverstripe 4 documentation about HTTP Middlewares](https://docs.silverstripe.org/en/4/developer_guides/controllers/middlewares/) standard.
+
+[Review the CVE-2019-19326 public disclosure](https://www.silverstripe.org/download/security-releases/cve-2019-19326)
+
+## CVE-2020-9311 Malicious user profile information can cause login form XSS {#CVE-2020-9311}
+
+Malicious users with a valid Silverstripe login (usually CMS access) can craft profile information which can lead to XSS for other users through specially crafted login form URLs.
+
+[Review the CVE-2020-9311 public disclosure](https://www.silverstripe.org/download/security-releases/cve-2020-9311)
+
+
+<!--- Changes below this line will be automatically regenerated -->
+
+## Change Log
+
+### Security
+
+ * 2020-07-09 [c96e9d2fe](https://github.com/silverstripe/silverstripe-framework/commit/c96e9d2fe5e0fbea1da4059264e4da269889f55d) Add public disclosure statement to changelog (Maxime Rainville) - See [cve-2020-9311](https://www.silverstripe.org/download/security-releases/cve-2020-9311)
+ * 2020-05-04 [074b28cf9](https://github.com/silverstripe/silverstripe-framework/commit/074b28cf937821a0d5627d3f19836ede1d662395) Add changelog for CVE-2019-19326 (Maxime Rainville) - See [cve-2019-19326](https://www.silverstripe.org/download/security-releases/cve-2019-19326)
+ * 2020-04-28 [98926e4e6](https://github.com/silverstripe/silverstripe-framework/commit/98926e4e6c26d1d43bb1faf516d15bdb2739556e) Stop honouring X-HTTP-Method-Override header, X-Original-Url header and _method POST variable. Add SS_HTTPRequest::setHttpMethod(). (Maxime Rainville) - See [cve-2019-19326](https://www.silverstripe.org/download/security-releases/cve-2019-19326)
+ * 2020-04-23 [d3b23e702](https://github.com/silverstripe/silverstripe-framework/commit/d3b23e7024add23de1cb643a44e30d249c2b7cd6) Escape First Name when displaying re-login screen (Maxime Rainville) - See [cve-2020-9311](https://www.silverstripe.org/download/security-releases/cve-2020-9311)
+
+### Features and Enhancements
+
+ * 2019-11-18 [54e7223d9](https://github.com/silverstripe/silverstripe-framework/commit/54e7223d981eee7f00244ad9a79187ee3f2f063a) Docs rebuild for compliance with Gatsby (#9316) (Aaron Carlino)
+
+### Bugfixes
+
+ * 2020-04-01 [6c8dc0fd9](https://github.com/silverstripe/silverstripe-framework/commit/6c8dc0fd9957d0f497ccc3c700c0d805aff1269e) Fix deprecated php syntax (Dan Hensby)
+ * 2019-11-19 [42ab51230](https://github.com/silverstripe/silverstripe-framework/commit/42ab512306196d1010808adbe728f1fe179519aa) Fix broken callout tags (Aaron Carlino)
+<!--- Changes above this line will be automatically regenerated -->
diff --git 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/model/ArrayList.php b/model/ArrayList.php
@@ -475,7 +475,7 @@ public function canFilterBy($by) {
 			return false;
 		}
 
-		return array_key_exists($by, $firstRecord);
+        return is_array($firstRecord) ? array_key_exists($by, $firstRecord) : property_exists($by, $firstRecord);
 	}
 
 	/**

diff --git a/security/CMSSecurity.php b/security/CMSSecurity.php
@@ -86,7 +86,7 @@ public function getTitle() {
 				'CMSSecurity.TimedOutTitleMember',
 				'Hey {name}!<br />Your session has timed out.',
 				'Title for CMS popup login form for a known user',
-				array('name' => $member->FirstName)
+				array('name' => Convert::raw2xml($member->FirstName))
 			);
 		} else {
 			return _t(

diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php
@@ -139,7 +139,7 @@ protected function getMessageFromSession() {
 			$this->message = _t(
 				'Member.LOGGEDINAS',
 				"You're logged in as {name}.",
-				array('name' => $member->{$this->loggedInAsField})
+				array('name' => Convert::raw2xml($member->{$this->loggedInAsField}))
 			);
 		}
 

diff --git 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
@@ -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',