From 8313a3fcb3b24bf9e36f48581f64336623ae1ead Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 13 Aug 2015 07:36:42 +0200 Subject: [PATCH] Add mitigation against BREACH While BREACH requires the following three factors to be effectively exploitable we should add another mitigation: 1. Application must support HTTP compression 2. Response most reflect user-controlled input 3. Response should contain sensitive data Especially part 2 is with ownCloud not really given since user-input is usually only echoed if a CSRF token has been passed. To reduce the risk even further it is however sensible to encrypt the CSRF token with a shared secret. Since this will change on every request an attack such as BREACH is not feasible anymore against the CSRF token at least. --- lib/base.php | 13 +- lib/private/appframework/http/request.php | 22 +++- lib/private/server.php | 1 + lib/private/util.php | 8 +- .../controller/ApiControllerTest.php | 1 + .../controller/ControllerTest.php | 1 + .../controller/OCSControllerTest.php | 4 + .../dependencyinjection/DIContainerTest.php | 1 + .../lib/appframework/http/DispatcherTest.php | 6 + tests/lib/appframework/http/RequestTest.php | 117 +++++++++++++++--- .../middleware/MiddlewareDispatcherTest.php | 1 + .../middleware/MiddlewareTest.php | 1 + .../security/CORSMiddlewareTest.php | 10 ++ .../security/SecurityMiddlewareTest.php | 1 + .../middleware/sessionmiddlewaretest.php | 1 + tests/lib/util.php | 2 +- 16 files changed, 159 insertions(+), 31 deletions(-) diff --git a/lib/base.php b/lib/base.php index c0f3e50142..07a1e8dfee 100644 --- a/lib/base.php +++ b/lib/base.php @@ -134,18 +134,7 @@ class OC { 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 index aaad286e84..a210943917 100644 --- a/lib/private/appframework/http/request.php +++ b/lib/private/appframework/http/request.php @@ -32,6 +32,7 @@ namespace OC\AppFramework\Http; 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 @@ class Request implements \ArrayAccess, \Countable, IRequest { 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 index 618431ff2d..9eea7c4537 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -409,6 +409,7 @@ class Server extends SimpleContainer implements IServerContainer { 'requesttoken' => $requestToken, ], $this->getSecureRandom(), + $this->getCrypto(), $this->getConfig(), $stream ); diff --git a/lib/private/util.php b/lib/private/util.php index 501dbf5c4c..edd375b5c3 100644 --- a/lib/private/util.php +++ b/lib/private/util.php @@ -1057,7 +1057,8 @@ class OC_Util { /** * 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 @@ class OC_Util { // 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 index 137e5950f6..573fe7f3ba 100644 --- a/tests/lib/appframework/controller/ApiControllerTest.php +++ b/tests/lib/appframework/controller/ApiControllerTest.php @@ -38,6 +38,7 @@ class ApiControllerTest extends \Test\TestCase { $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 index 0d7716da41..243014a91a 100644 --- a/tests/lib/appframework/controller/ControllerTest.php +++ b/tests/lib/appframework/controller/ControllerTest.php @@ -76,6 +76,7 @@ class ControllerTest extends \Test\TestCase { '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 index 92b092cf0e..292a56e3ca 100644 --- a/tests/lib/appframework/controller/OCSControllerTest.php +++ b/tests/lib/appframework/controller/OCSControllerTest.php @@ -43,6 +43,7 @@ class OCSControllerTest extends \Test\TestCase { ], ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $controller = new ChildOCSController('app', $request, 'verbs', @@ -64,6 +65,7 @@ class OCSControllerTest extends \Test\TestCase { $controller = new ChildOCSController('app', new Request( [], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') )); $expected = "\n" . @@ -96,6 +98,7 @@ class OCSControllerTest extends \Test\TestCase { $controller = new ChildOCSController('app', new Request( [], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') )); $expected = "\n" . @@ -128,6 +131,7 @@ class OCSControllerTest extends \Test\TestCase { $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 index 0cbdddbb20..598e70beff 100644 --- a/tests/lib/appframework/dependencyinjection/DIContainerTest.php +++ b/tests/lib/appframework/dependencyinjection/DIContainerTest.php @@ -74,6 +74,7 @@ class DIContainerTest extends \Test\TestCase { $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 index 02c86df8e7..c25fd7b6f8 100644 --- a/tests/lib/appframework/http/DispatcherTest.php +++ b/tests/lib/appframework/http/DispatcherTest.php @@ -295,6 +295,7 @@ class DispatcherTest extends \Test\TestCase { 'method' => 'POST' ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->dispatcher = new Dispatcher( @@ -322,6 +323,7 @@ class DispatcherTest extends \Test\TestCase { 'method' => 'POST', ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->dispatcher = new Dispatcher( @@ -352,6 +354,7 @@ class DispatcherTest extends \Test\TestCase { 'method' => 'GET' ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->dispatcher = new Dispatcher( @@ -381,6 +384,7 @@ class DispatcherTest extends \Test\TestCase { 'method' => 'GET' ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->dispatcher = new Dispatcher( @@ -411,6 +415,7 @@ class DispatcherTest extends \Test\TestCase { 'method' => 'PUT' ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->dispatcher = new Dispatcher( @@ -443,6 +448,7 @@ class DispatcherTest extends \Test\TestCase { 'method' => 'POST' ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->dispatcher = new Dispatcher( diff --git a/tests/lib/appframework/http/RequestTest.php b/tests/lib/appframework/http/RequestTest.php index 10a9e486c9..deb2890986 100644 --- a/tests/lib/appframework/http/RequestTest.php +++ b/tests/lib/appframework/http/RequestTest.php @@ -10,6 +10,7 @@ namespace OC\AppFramework\Http; +use OC\Security\Crypto; use OCP\Security\ISecureRandom; use OCP\IConfig; @@ -53,6 +54,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -85,6 +87,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -96,8 +99,8 @@ class RequestTest extends \Test\TestCase { /** - * @expectedException \RuntimeException - */ + * @expectedException \RuntimeException + */ public function testImmutableArrayAccess() { $vars = array( 'get' => array('name' => 'John Q. Public', 'nickname' => 'Joey'), @@ -107,6 +110,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -115,8 +119,8 @@ class RequestTest extends \Test\TestCase { } /** - * @expectedException \RuntimeException - */ + * @expectedException \RuntimeException + */ public function testImmutableMagicAccess() { $vars = array( 'get' => array('name' => 'John Q. Public', 'nickname' => 'Joey'), @@ -126,6 +130,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -134,8 +139,8 @@ class RequestTest extends \Test\TestCase { } /** - * @expectedException \LogicException - */ + * @expectedException \LogicException + */ public function testGetTheMethodRight() { $vars = array( 'get' => array('name' => 'John Q. Public', 'nickname' => 'Joey'), @@ -145,6 +150,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -161,6 +167,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -182,6 +189,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -206,6 +214,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -230,6 +239,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -250,6 +260,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -274,6 +285,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -303,6 +315,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -324,6 +337,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( $vars, $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -347,6 +361,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( [], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -358,6 +373,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( [], \OC::$server->getSecureRandom(), + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -382,6 +398,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -410,6 +427,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -438,6 +456,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -470,6 +489,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -497,6 +517,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( [], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -506,10 +527,10 @@ class RequestTest extends \Test\TestCase { public function testGetServerProtocolWithProtoValid() { $this->config - ->expects($this->exactly(2)) - ->method('getSystemValue') - ->with('overwriteprotocol') - ->will($this->returnValue('')); + ->expects($this->exactly(2)) + ->method('getSystemValue') + ->with('overwriteprotocol') + ->will($this->returnValue('')); $requestHttps = new Request( [ @@ -518,6 +539,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -528,6 +550,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -551,6 +574,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -571,6 +595,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -587,6 +612,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( [], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -607,6 +633,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -628,6 +655,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -716,6 +744,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -732,6 +761,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -749,6 +779,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -766,6 +797,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -793,6 +825,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( [], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -814,6 +847,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -840,6 +874,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -866,6 +901,7 @@ class RequestTest extends \Test\TestCase { ], ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -882,6 +918,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( [], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -909,6 +946,7 @@ class RequestTest extends \Test\TestCase { $request = new Request( [], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -924,6 +962,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -944,6 +983,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -964,6 +1004,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -986,6 +1027,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -1008,6 +1050,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -1030,6 +1073,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -1052,6 +1096,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -1105,6 +1150,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ); @@ -1144,6 +1190,7 @@ class RequestTest extends \Test\TestCase { ] ], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ]) @@ -1157,17 +1204,25 @@ class RequestTest extends \Test\TestCase { } public function testPassesCSRFCheckWithGet() { + $crypto = $this->getMock('\OCP\Security\ICrypto'); + $crypto + ->expects($this->once()) + ->method('decrypt') + ->with('1c637c4147e40a8a8f09428ec2059cebea3480c27b402b4e793c69710a731513|wlXxNUaFqHuQnZr5|e6ab49c9e0e20c8d3607e02f1d8e6ec17ad6020ae10b7d64ab4b0a6318c0875940943a6aa303dc090fea0b4cd5b9fb8bcbecac4308a2bd15d9f369cdc22121a4', 'secret') + ->will($this->returnValue('MyStoredRequestToken')); + /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) ->setConstructorArgs([ [ 'get' => [ - 'requesttoken' => 'MyStoredRequestToken', + 'requesttoken' => '1c637c4147e40a8a8f09428ec2059cebea3480c27b402b4e793c69710a731513|wlXxNUaFqHuQnZr5|e6ab49c9e0e20c8d3607e02f1d8e6ec17ad6020ae10b7d64ab4b0a6318c0875940943a6aa303dc090fea0b4cd5b9fb8bcbecac4308a2bd15d9f369cdc22121a4:secret', ], 'requesttoken' => 'MyStoredRequestToken', ], $this->secureRandom, + $crypto, $this->config, $this->stream ]) @@ -1177,17 +1232,25 @@ class RequestTest extends \Test\TestCase { } public function testPassesCSRFCheckWithPost() { + $crypto = $this->getMock('\OCP\Security\ICrypto'); + $crypto + ->expects($this->once()) + ->method('decrypt') + ->with('1c637c4147e40a8a8f09428ec2059cebea3480c27b402b4e793c69710a731513|wlXxNUaFqHuQnZr5|e6ab49c9e0e20c8d3607e02f1d8e6ec17ad6020ae10b7d64ab4b0a6318c0875940943a6aa303dc090fea0b4cd5b9fb8bcbecac4308a2bd15d9f369cdc22121a4', 'secret') + ->will($this->returnValue('MyStoredRequestToken')); + /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) ->setConstructorArgs([ [ 'post' => [ - 'requesttoken' => 'MyStoredRequestToken', + 'requesttoken' => '1c637c4147e40a8a8f09428ec2059cebea3480c27b402b4e793c69710a731513|wlXxNUaFqHuQnZr5|e6ab49c9e0e20c8d3607e02f1d8e6ec17ad6020ae10b7d64ab4b0a6318c0875940943a6aa303dc090fea0b4cd5b9fb8bcbecac4308a2bd15d9f369cdc22121a4:secret', ], 'requesttoken' => 'MyStoredRequestToken', ], $this->secureRandom, + $crypto, $this->config, $this->stream ]) @@ -1197,17 +1260,24 @@ class RequestTest extends \Test\TestCase { } public function testPassesCSRFCheckWithHeader() { + $crypto = $this->getMock('\OCP\Security\ICrypto'); + $crypto + ->expects($this->once()) + ->method('decrypt') + ->with('1c637c4147e40a8a8f09428ec2059cebea3480c27b402b4e793c69710a731513|wlXxNUaFqHuQnZr5|e6ab49c9e0e20c8d3607e02f1d8e6ec17ad6020ae10b7d64ab4b0a6318c0875940943a6aa303dc090fea0b4cd5b9fb8bcbecac4308a2bd15d9f369cdc22121a4', 'secret') + ->will($this->returnValue('MyStoredRequestToken')); /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) ->setConstructorArgs([ [ 'server' => [ - 'HTTP_REQUESTTOKEN' => 'MyStoredRequestToken', + 'HTTP_REQUESTTOKEN' => '1c637c4147e40a8a8f09428ec2059cebea3480c27b402b4e793c69710a731513|wlXxNUaFqHuQnZr5|e6ab49c9e0e20c8d3607e02f1d8e6ec17ad6020ae10b7d64ab4b0a6318c0875940943a6aa303dc090fea0b4cd5b9fb8bcbecac4308a2bd15d9f369cdc22121a4:secret', ], 'requesttoken' => 'MyStoredRequestToken', ], $this->secureRandom, + $crypto, $this->config, $this->stream ]) @@ -1216,18 +1286,34 @@ class RequestTest extends \Test\TestCase { $this->assertTrue($request->passesCSRFCheck()); } - public function testPassesCSRFCheckWithInvalidToken() { + public function invalidTokenDataProvider() { + return [ + ['InvalidSentToken'], + ['InvalidSentToken:InvalidSecret'], + [null], + [''], + ]; + } + + /** + * @dataProvider invalidTokenDataProvider + * @param string $invalidToken + */ + public function testPassesCSRFCheckWithInvalidToken($invalidToken) { + $crypto = new Crypto($this->config, $this->secureRandom); + /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) ->setConstructorArgs([ [ 'server' => [ - 'HTTP_REQUESTTOKEN' => 'MyInvalidSentToken', + 'HTTP_REQUESTTOKEN' => $invalidToken, ], 'requesttoken' => 'MyStoredRequestToken', ], $this->secureRandom, + $crypto, $this->config, $this->stream ]) @@ -1243,6 +1329,7 @@ class RequestTest extends \Test\TestCase { ->setConstructorArgs([ [], $this->secureRandom, + $this->getMock('\OCP\Security\ICrypto'), $this->config, $this->stream ]) diff --git a/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php b/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php index a873152579..eab45b03c9 100644 --- a/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php +++ b/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php @@ -133,6 +133,7 @@ class MiddlewareDispatcherTest extends \Test\TestCase { new Request( ['method' => 'GET'], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ) ] diff --git a/tests/lib/appframework/middleware/MiddlewareTest.php b/tests/lib/appframework/middleware/MiddlewareTest.php index 33f04e1383..8e077b37e2 100644 --- a/tests/lib/appframework/middleware/MiddlewareTest.php +++ b/tests/lib/appframework/middleware/MiddlewareTest.php @@ -61,6 +61,7 @@ class MiddlewareTest extends \Test\TestCase { new Request( [], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ) ] diff --git a/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php index ca526fb859..f5e6106dc3 100644 --- a/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php +++ b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php @@ -42,6 +42,7 @@ class CORSMiddlewareTest extends \Test\TestCase { ] ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->reflector->reflect($this, __FUNCTION__); @@ -61,6 +62,7 @@ class CORSMiddlewareTest extends \Test\TestCase { ] ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $middleware = new CORSMiddleware($request, $this->reflector, $this->session); @@ -78,6 +80,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $request = new Request( [], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->reflector->reflect($this, __FUNCTION__); @@ -101,6 +104,7 @@ class CORSMiddlewareTest extends \Test\TestCase { ] ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->reflector->reflect($this, __FUNCTION__); @@ -119,6 +123,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $request = new Request( [], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->reflector->reflect($this, __FUNCTION__); @@ -144,6 +149,7 @@ class CORSMiddlewareTest extends \Test\TestCase { 'PHP_AUTH_PW' => 'pass' ]], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->session->expects($this->once()) @@ -169,6 +175,7 @@ class CORSMiddlewareTest extends \Test\TestCase { 'PHP_AUTH_PW' => 'pass' ]], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->session->expects($this->once()) @@ -190,6 +197,7 @@ class CORSMiddlewareTest extends \Test\TestCase { 'PHP_AUTH_PW' => 'pass' ]], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $middleware = new CORSMiddleware($request, $this->reflector, $this->session); @@ -206,6 +214,7 @@ class CORSMiddlewareTest extends \Test\TestCase { 'PHP_AUTH_PW' => 'pass' ]], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $middleware = new CORSMiddleware($request, $this->reflector, $this->session); @@ -226,6 +235,7 @@ class CORSMiddlewareTest extends \Test\TestCase { 'PHP_AUTH_PW' => 'pass' ]], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $middleware = new CORSMiddleware($request, $this->reflector, $this->session); diff --git a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php index 347a0423ea..3b4d7987e9 100644 --- a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php +++ b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php @@ -322,6 +322,7 @@ class SecurityMiddlewareTest extends \Test\TestCase { ] ], $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->middleware = $this->getMiddleware(true, true); diff --git a/tests/lib/appframework/middleware/sessionmiddlewaretest.php b/tests/lib/appframework/middleware/sessionmiddlewaretest.php index 11c1600f51..06390e96d4 100644 --- a/tests/lib/appframework/middleware/sessionmiddlewaretest.php +++ b/tests/lib/appframework/middleware/sessionmiddlewaretest.php @@ -36,6 +36,7 @@ class SessionMiddlewareTest extends \Test\TestCase { $this->request = new Request( [], $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), + $this->getMock('\OCP\Security\ICrypto'), $this->getMock('\OCP\IConfig') ); $this->reflector = new ControllerMethodReflector(); diff --git a/tests/lib/util.php b/tests/lib/util.php index e52a9fcc61..b9b8062653 100644 --- a/tests/lib/util.php +++ b/tests/lib/util.php @@ -91,7 +91,7 @@ class Test_Util extends \Test\TestCase { function testCallRegister() { $result = strlen(OC_Util::callRegister()); - $this->assertEquals(30, $result); + $this->assertEquals(221, $result); } function testSanitizeHTML() {