Merge pull request #18254 from owncloud/mitigate-breach

Add mitigation against BREACH
owncloud · Aug 24, 2015 · 40b1054 · 40b1054
2 parents 510010e + df2ce8a
commit 40b1054
Show file tree

Hide file tree

Showing 17 changed files with 160 additions and 32 deletions.
diff --git a/core/templates/layout.user.php b/core/templates/layout.user.php
@@ -100,7 +100,7 @@
 					<?php p($l->t('Search'));?>
 				</label>
 				<input id="searchbox" class="svg" type="search" name="query"
-					value="<?php if(isset($_POST['query'])) {p($_POST['query']);};?>"
+					value=""
 					autocomplete="off" tabindex="5">
 			</form>
 		</div></header>

diff --git a/lib/base.php b/lib/base.php
@@ -134,18 +134,7 @@ public static function initPaths() {
 		OC_Config::$object = new \OC\Config(self::$configDir);
 
 		OC::$SUBURI = str_replace("\\", "/", substr(realpath($_SERVER["SCRIPT_FILENAME"]), strlen(OC::$SERVERROOT)));
-		/**
-		 * FIXME: The following line is required because of a cyclic dependency
-		 *        on IRequest.
-		 */
-		$params = [
-			'server' => [
-				'SCRIPT_NAME' => $_SERVER['SCRIPT_NAME'],
-				'SCRIPT_FILENAME' => $_SERVER['SCRIPT_FILENAME'],
-			],
-		];
-		$fakeRequest = new \OC\AppFramework\Http\Request($params, null, new \OC\AllConfig(new \OC\SystemConfig()));
-		$scriptName = $fakeRequest->getScriptName();
+		$scriptName = $_SERVER['SCRIPT_NAME'];
 		if (substr($scriptName, -1) == '/') {
 			$scriptName .= 'index.php';
 			//make sure suburi follows the same rules as scriptName

diff --git a/lib/private/appframework/http/request.php b/lib/private/appframework/http/request.php
@@ -32,6 +32,7 @@
 use OC\Security\TrustedDomainHelper;
 use OCP\IConfig;
 use OCP\IRequest;
+use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 
 /**
@@ -67,6 +68,8 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 	protected $config;
 	/** @var string */
 	protected $requestId = '';
+	/** @var ICrypto */
+	protected $crypto;
 
 	/**
 	 * @param array $vars An associative array with the following optional values:
@@ -80,17 +83,20 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 	 *        - string 'method' the request method (GET, POST etc)
 	 *        - string|false 'requesttoken' the requesttoken or false when not available
 	 * @param ISecureRandom $secureRandom
+	 * @param ICrypto $crypto
 	 * @param IConfig $config
 	 * @param string $stream
 	 * @see http://www.php.net/manual/en/reserved.variables.php
 	 */
 	public function __construct(array $vars=array(),
 								ISecureRandom $secureRandom = null,
+								ICrypto $crypto,
 								IConfig $config,
 								$stream='php://input') {
 		$this->inputStream = $stream;
 		$this->items['params'] = array();
 		$this->secureRandom = $secureRandom;
+		$this->crypto = $crypto;
 		$this->config = $config;
 
 		if(!array_key_exists('method', $vars)) {
@@ -415,8 +421,22 @@ public function passesCSRFCheck() {
 			return false;
 		}
 
+		// Decrypt token to prevent BREACH like attacks
+		$token = explode(':', $token);
+		if (count($token) !== 2) {
+			return false;
+		}
+
+		$encryptedToken = $token[0];
+		$secret = $token[1];
+		try {
+			$decryptedToken = $this->crypto->decrypt($encryptedToken, $secret);
+		} catch (\Exception $e) {
+			return false;
+		}
+
 		// Check if the token is valid
-		if(\OCP\Security\StringUtils::equals($token, $this->items['requesttoken'])) {
+		if(\OCP\Security\StringUtils::equals($decryptedToken, $this->items['requesttoken'])) {
 			return true;
 		} else {
 			return false;

diff --git a/lib/private/server.php b/lib/private/server.php
@@ -410,6 +410,7 @@ public function __construct($webRoot) {
 					'requesttoken' => $requestToken,
 				],
 				$this->getSecureRandom(),
+				$this->getCrypto(),
 				$this->getConfig(),
 				$stream
 			);

diff --git a/lib/private/util.php b/lib/private/util.php
@@ -1057,7 +1057,8 @@ public static function getInstanceId() {
 	/**
 	 * Register an get/post call. Important to prevent CSRF attacks.
 	 *
-	 * @return string Generated token.
+	 * @return string The encrypted CSRF token, the shared secret is appended after the `:`.
+	 *
 	 * @description
 	 * Creates a 'request token' (random) and stores it inside the session.
 	 * Ever subsequent (ajax) request must use such a valid token to succeed,
@@ -1074,7 +1075,10 @@ public static function callRegister() {
 			// Valid token already exists, send it
 			$requestToken = \OC::$server->getSession()->get('requesttoken');
 		}
-		return ($requestToken);
+
+		// Encrypt the token to mitigate breach-like attacks
+		$sharedSecret = \OC::$server->getSecureRandom()->getMediumStrengthGenerator()->generate(10);
+		return \OC::$server->getCrypto()->encrypt($requestToken, $sharedSecret) . ':' . $sharedSecret;
 	}
 
 	/**

diff --git a/tests/lib/appframework/controller/ApiControllerTest.php b/tests/lib/appframework/controller/ApiControllerTest.php
@@ -38,6 +38,7 @@ public function testCors() {
         $request = new Request(
             ['server' => ['HTTP_ORIGIN' => 'test']],
             $this->getMock('\OCP\Security\ISecureRandom'),
+            $this->getMock('\OCP\Security\ICrypto'),
             $this->getMock('\OCP\IConfig')
         );
         $this->controller = new ChildApiController('app', $request, 'verbs',

diff --git a/tests/lib/appframework/controller/ControllerTest.php b/tests/lib/appframework/controller/ControllerTest.php
@@ -76,6 +76,7 @@ protected function setUp(){
 				'method' => 'hi',
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 

diff --git a/tests/lib/appframework/controller/OCSControllerTest.php b/tests/lib/appframework/controller/OCSControllerTest.php
@@ -43,6 +43,7 @@ public function testCors() {
 				],
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$controller = new ChildOCSController('app', $request, 'verbs',
@@ -64,6 +65,7 @@ public function testXML() {
 		$controller = new ChildOCSController('app', new Request(
 			[],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		));
 		$expected = "<?xml version=\"1.0\"?>\n" .
@@ -96,6 +98,7 @@ public function testXMLDataResponse() {
 		$controller = new ChildOCSController('app', new Request(
 			[],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		));
 		$expected = "<?xml version=\"1.0\"?>\n" .
@@ -128,6 +131,7 @@ public function testJSON() {
 		$controller = new ChildOCSController('app', new Request(
 			[],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		));
 		$expected = '{"ocs":{"meta":{"status":"failure","statuscode":400,"message":"OK",' .

diff --git a/tests/lib/appframework/dependencyinjection/DIContainerTest.php b/tests/lib/appframework/dependencyinjection/DIContainerTest.php
@@ -74,6 +74,7 @@ public function testMiddlewareDispatcherIncludesSecurityMiddleware(){
 		$this->container['Request'] = new Request(
 			['method' => 'GET'],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$security = $this->container['SecurityMiddleware'];

diff --git a/tests/lib/appframework/http/DispatcherTest.php b/tests/lib/appframework/http/DispatcherTest.php
@@ -295,6 +295,7 @@ public function testControllerParametersInjected() {
 				'method' => 'POST'
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->dispatcher = new Dispatcher(
@@ -322,6 +323,7 @@ public function testControllerParametersInjectedDefaultOverwritten() {
 				'method' => 'POST',
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->dispatcher = new Dispatcher(
@@ -352,6 +354,7 @@ public function testResponseTransformedByUrlFormat() {
 				'method' => 'GET'
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->dispatcher = new Dispatcher(
@@ -381,6 +384,7 @@ public function testResponseTransformsDataResponse() {
 				'method' => 'GET'
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->dispatcher = new Dispatcher(
@@ -411,6 +415,7 @@ public function testResponseTransformedByAcceptHeader() {
 				'method' => 'PUT'
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->dispatcher = new Dispatcher(
@@ -443,6 +448,7 @@ public function testResponsePrimarilyTransformedByParameterFormat() {
 				'method' => 'POST'
 			],
 			$this->getMock('\OCP\Security\ISecureRandom'),
+			$this->getMock('\OCP\Security\ICrypto'),
 			$this->getMock('\OCP\IConfig')
 		);
 		$this->dispatcher = new Dispatcher(