From a299fa38a9172f16e4bc48d4bd4f9807cec2f737 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 20 Jul 2016 17:37:30 +0200 Subject: [PATCH] [master] Port Same-Site Cookies to master Fixes https://github.com/nextcloud/server/issues/50 --- apps/files/lib/Controller/ApiController.php | 1 + core/js/config.php | 4 + lib/base.php | 80 +++++ lib/private/AppFramework/Http/Request.php | 40 +++ .../StrictCookieMissingException.php | 36 ++ .../Security/SecurityMiddleware.php | 11 +- lib/private/legacy/eventsource.php | 4 + lib/private/legacy/json.php | 5 + lib/public/IRequest.php | 18 + lib/public/Util.php | 5 + tests/lib/AppFramework/Http/RequestTest.php | 309 ++++++++++++++++-- .../Security/SecurityMiddlewareTest.php | 222 +++++++++---- 12 files changed, 637 insertions(+), 98 deletions(-) create mode 100644 lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php diff --git a/apps/files/lib/Controller/ApiController.php b/apps/files/lib/Controller/ApiController.php index 9a46cdd0fe..a442d7ea90 100644 --- a/apps/files/lib/Controller/ApiController.php +++ b/apps/files/lib/Controller/ApiController.php @@ -86,6 +86,7 @@ class ApiController extends Controller { * * @NoAdminRequired * @NoCSRFRequired + * @StrictCookieRequired * * @param int $x * @param int $y diff --git a/core/js/config.php b/core/js/config.php index 197047ed8b..c2e6213e8f 100644 --- a/core/js/config.php +++ b/core/js/config.php @@ -32,6 +32,10 @@ * */ +if(!\OC::$server->getRequest()->passesStrictCookieCheck()) { + die(); +} + // Set the content type to Javascript header("Content-type: text/javascript"); diff --git a/lib/base.php b/lib/base.php index be9de93f73..afa13cd104 100644 --- a/lib/base.php +++ b/lib/base.php @@ -469,6 +469,84 @@ class OC { @ini_set('gd.jpeg_ignore_warning', 1); } + /** + * Send the same site cookies + */ + private static function sendSameSiteCookies() { + $cookieParams = session_get_cookie_params(); + $secureCookie = ($cookieParams['secure'] === true) ? 'secure; ' : ''; + $policies = [ + 'lax', + 'strict', + ]; + foreach($policies as $policy) { + header( + sprintf( + 'Set-Cookie: nc_sameSiteCookie%s=true; path=%s; httponly;' . $secureCookie . 'expires=Fri, 31-Dec-2100 23:59:59 GMT; SameSite=%s', + $policy, + $cookieParams['path'], + $policy + ), + false + ); + } + } + + /** + * Same Site cookie to further mitigate CSRF attacks. This cookie has to + * be set in every request if cookies are sent to add a second level of + * defense against CSRF. + * + * If the cookie is not sent this will set the cookie and reload the page. + * We use an additional cookie since we want to protect logout CSRF and + * also we can't directly interfere with PHP's session mechanism. + */ + private static function performSameSiteCookieProtection() { + if(count($_COOKIE) > 0) { + $request = \OC::$server->getRequest(); + $requestUri = $request->getScriptName(); + $processingScript = explode('/', $requestUri); + $processingScript = $processingScript[count($processingScript)-1]; + // FIXME: In a SAML scenario we don't get any strict or lax cookie + // send for the ACS endpoint. Since we have some legacy code in Nextcloud + // (direct PHP files) the enforcement of lax cookies is performed here + // instead of the middleware. + // + // This means we cannot exclude some routes from the cookie validation, + // which normally is not a problem but is a little bit cumbersome for + // this use-case. + // Once the old legacy PHP endpoints have been removed we can move + // the verification into a middleware and also adds some exemptions. + // + // Questions about this code? Ask Lukas ;-) + $currentUrl = substr(explode('?',$request->getRequestUri(), 2)[0], strlen(\OC::$WEBROOT)); + if($currentUrl === '/index.php/apps/user_saml/saml/acs') { + return; + } + // For the "index.php" endpoint only a lax cookie is required. + if($processingScript === 'index.php') { + if(!$request->passesLaxCookieCheck()) { + self::sendSameSiteCookies(); + header('Location: '.$_SERVER['REQUEST_URI']); + exit(); + } + } else { + // All other endpoints require the lax and the strict cookie + if(!$request->passesStrictCookieCheck()) { + self::sendSameSiteCookies(); + // Debug mode gets access to the resources without strict cookie + // due to the fact that the SabreDAV browser also lives there. + if(!\OC::$server->getConfig()->getSystemValue('debug', false)) { + http_response_code(\OCP\AppFramework\Http::STATUS_SERVICE_UNAVAILABLE); + exit(); + } + } + } + } elseif(!isset($_COOKIE['nc_sameSiteCookielax']) || !isset($_COOKIE['nc_sameSiteCookiestrict'])) { + self::sendSameSiteCookies(); + } + } + public static function init() { // calculate the root directories OC::$SERVERROOT = str_replace("\\", '/', substr(__DIR__, 0, -4)); @@ -572,6 +650,8 @@ class OC { ini_set('session.cookie_secure', true); } + self::performSameSiteCookieProtection(); + if (!defined('OC_CONSOLE')) { $errors = OC_Util::checkServer(\OC::$server->getConfig()); if (count($errors) > 0) { diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index d9cf191925..8fc99f125b 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -465,6 +465,10 @@ class Request implements \ArrayAccess, \Countable, IRequest { return false; } + if(!$this->passesStrictCookieCheck()) { + return false; + } + if (isset($this->items['get']['requesttoken'])) { $token = $this->items['get']['requesttoken']; } elseif (isset($this->items['post']['requesttoken'])) { @@ -480,6 +484,42 @@ class Request implements \ArrayAccess, \Countable, IRequest { return $this->csrfTokenManager->isTokenValid($token); } + /** + * Checks if the strict cookie has been sent with the request if the request + * is including any cookies. + * + * @return bool + * @since 9.1.0 + */ + public function passesStrictCookieCheck() { + if(count($this->cookies) === 0) { + return true; + } + if($this->getCookie('nc_sameSiteCookiestrict') === 'true' + && $this->passesLaxCookieCheck()) { + return true; + } + return false; + } + + /** + * Checks if the lax cookie has been sent with the request if the request + * is including any cookies. + * + * @return bool + * @since 9.1.0 + */ + public function passesLaxCookieCheck() { + if(count($this->cookies) === 0) { + return true; + } + if($this->getCookie('nc_sameSiteCookielax') === 'true') { + return true; + } + return false; + } + + /** * Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging * If `mod_unique_id` is installed this value will be taken. diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php new file mode 100644 index 0000000000..9ccaed4566 --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php @@ -0,0 +1,36 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\AppFramework\Middleware\Security\Exceptions; + +use OCP\AppFramework\Http; + +/** + * Class StrictCookieMissingException is thrown when the strict cookie has not + * been sent with the request but is required. + * + * @package OC\AppFramework\Middleware\Security\Exceptions + */ +class StrictCookieMissingException extends SecurityException { + public function __construct() { + parent::__construct('Strict Cookie has not been found in request.', Http::STATUS_PRECONDITION_FAILED); + } +} diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index a3ece262e1..98117751e2 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -32,6 +32,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException; use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; +use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\CSP\ContentSecurityPolicyManager; use OCP\AppFramework\Http\ContentSecurityPolicy; @@ -134,6 +135,12 @@ class SecurityMiddleware extends Middleware { } } + // Check for strict cookie requirement + if($this->reflector->hasAnnotation('StrictCookieRequired') || !$this->reflector->hasAnnotation('NoCSRFRequired')) { + if(!$this->request->passesStrictCookieCheck()) { + throw new StrictCookieMissingException(); + } + } // CSRF check - also registers the CSRF token since the session may be closed later Util::callRegister(); if(!$this->reflector->hasAnnotation('NoCSRFRequired')) { @@ -186,7 +193,9 @@ class SecurityMiddleware extends Middleware { */ public function afterException($controller, $methodName, \Exception $exception) { if($exception instanceof SecurityException) { - + if($exception instanceof StrictCookieMissingException) { + return new RedirectResponse(\OC::$WEBROOT); + } if (stripos($this->request->getHeader('Accept'),'html') === false) { $response = new JSONResponse( array('message' => $exception->getMessage()), diff --git a/lib/private/legacy/eventsource.php b/lib/private/legacy/eventsource.php index 51040e7be7..70e9847d23 100644 --- a/lib/private/legacy/eventsource.php +++ b/lib/private/legacy/eventsource.php @@ -76,6 +76,10 @@ class OC_EventSource implements \OCP\IEventSource { } else { header("Content-Type: text/event-stream"); } + if(!\OC::$server->getRequest()->passesStrictCookieCheck()) { + header('Location: '.\OC::$WEBROOT); + exit(); + } if (!(\OC::$server->getRequest()->passesCSRFCheck())) { $this->send('error', 'Possible CSRF attack. Connection will be closed.'); $this->close(); diff --git a/lib/private/legacy/json.php b/lib/private/legacy/json.php index 1dde63602b..557e1d7701 100644 --- a/lib/private/legacy/json.php +++ b/lib/private/legacy/json.php @@ -79,6 +79,11 @@ class OC_JSON{ * @deprecated Use annotation based CSRF checks from the AppFramework instead */ public static function callCheck() { + if(!\OC::$server->getRequest()->passesStrictCookieCheck()) { + header('Location: '.\OC::$WEBROOT); + exit(); + } + if( !(\OC::$server->getRequest()->passesCSRFCheck())) { $l = \OC::$server->getL10N('lib'); self::error(array( 'data' => array( 'message' => $l->t('Token expired. Please reload page.'), 'error' => 'token_expired' ))); diff --git a/lib/public/IRequest.php b/lib/public/IRequest.php index 4db1c18b9c..46e67d1415 100644 --- a/lib/public/IRequest.php +++ b/lib/public/IRequest.php @@ -157,6 +157,24 @@ interface IRequest { */ public function passesCSRFCheck(); + /** + * Checks if the strict cookie has been sent with the request if the request + * is including any cookies. + * + * @return bool + * @since 9.0.0 + */ + public function passesStrictCookieCheck(); + + /** + * Checks if the lax cookie has been sent with the request if the request + * is including any cookies. + * + * @return bool + * @since 9.0.0 + */ + public function passesLaxCookieCheck(); + /** * Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging * If `mod_unique_id` is installed this value will be taken. diff --git a/lib/public/Util.php b/lib/public/Util.php index 687f4e78f6..9422dbac66 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -513,6 +513,11 @@ class Util { * @deprecated 9.0.0 Use annotations based on the app framework. */ public static function callCheck() { + if(!\OC::$server->getRequest()->passesStrictCookieCheck()) { + header('Location: '.\OC::$WEBROOT); + exit(); + } + if (!(\OC::$server->getRequest()->passesCSRFCheck())) { exit(); } diff --git a/tests/lib/AppFramework/Http/RequestTest.php b/tests/lib/AppFramework/Http/RequestTest.php index ddc2403d86..8df81afeb3 100644 --- a/tests/lib/AppFramework/Http/RequestTest.php +++ b/tests/lib/AppFramework/Http/RequestTest.php @@ -1,7 +1,7 @@ [ - 'HTTP_USER_AGENT' => $testAgent, - ] - ], - $this->secureRandom, - $this->config, - $this->csrfTokenManager, - $this->stream + [ + 'server' => [ + 'HTTP_USER_AGENT' => $testAgent, + ] + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream ); $this->assertSame($matches, $request->isUserAgent($userAgent)); @@ -762,11 +762,11 @@ class RequestTest extends \Test\TestCase { */ public function testUndefinedUserAgent($testAgent, $userAgent, $matches) { $request = new Request( - [], - $this->secureRandom, - $this->config, - $this->csrfTokenManager, - $this->stream + [], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream ); $this->assertFalse($request->isUserAgent($userAgent)); @@ -1322,6 +1322,10 @@ class RequestTest extends \Test\TestCase { 'get' => [ 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], + 'cookies' => [ + 'nc_sameSiteCookiestrict' => 'true', + 'nc_sameSiteCookielax' => 'true', + ], ], $this->secureRandom, $this->config, @@ -1348,6 +1352,10 @@ class RequestTest extends \Test\TestCase { 'post' => [ 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], + 'cookies' => [ + 'nc_sameSiteCookiestrict' => 'true', + 'nc_sameSiteCookielax' => 'true', + ], ], $this->secureRandom, $this->config, @@ -1357,15 +1365,93 @@ class RequestTest extends \Test\TestCase { ->getMock(); $token = new CsrfToken('AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds'); $this->csrfTokenManager - ->expects($this->once()) - ->method('isTokenValid') - ->with($token) - ->willReturn(true); + ->expects($this->once()) + ->method('isTokenValid') + ->with($token) + ->willReturn(true); $this->assertTrue($request->passesCSRFCheck()); } public function testPassesCSRFCheckWithHeader() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'nc_sameSiteCookiestrict' => 'true', + 'nc_sameSiteCookielax' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $token = new CsrfToken('AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds'); + $this->csrfTokenManager + ->expects($this->once()) + ->method('isTokenValid') + ->with($token) + ->willReturn(true); + + $this->assertTrue($request->passesCSRFCheck()); + } + + public function testPassesCSRFCheckWithGetAndWithoutCookies() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'get' => [ + 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $this->csrfTokenManager + ->expects($this->once()) + ->method('isTokenValid') + ->willReturn(true); + + $this->assertTrue($request->passesCSRFCheck()); + } + + public function testPassesCSRFCheckWithPostAndWithoutCookies() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'post' => [ + 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $this->csrfTokenManager + ->expects($this->once()) + ->method('isTokenValid') + ->willReturn(true); + + $this->assertTrue($request->passesCSRFCheck()); + } + + public function testPassesCSRFCheckWithHeaderAndWithoutCookies() { /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) @@ -1381,16 +1467,182 @@ class RequestTest extends \Test\TestCase { $this->stream ]) ->getMock(); - $token = new CsrfToken('AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds'); $this->csrfTokenManager - ->expects($this->once()) - ->method('isTokenValid') - ->with($token) - ->willReturn(true); + ->expects($this->once()) + ->method('isTokenValid') + ->willReturn(true); $this->assertTrue($request->passesCSRFCheck()); } + public function testFailsCSRFCheckWithHeaderAndNotAllChecksPassing() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'nc_sameSiteCookiestrict' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $this->csrfTokenManager + ->expects($this->never()) + ->method('isTokenValid'); + + $this->assertFalse($request->passesCSRFCheck()); + } + + public function testPassesStrictCookieCheckWithAllCookies() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'nc_sameSiteCookiestrict' => 'true', + 'nc_sameSiteCookielax' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertTrue($request->passesStrictCookieCheck()); + } + + public function testFailsSRFCheckWithPostAndWithCookies() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'post' => [ + 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'foo' => 'bar', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $this->csrfTokenManager + ->expects($this->never()) + ->method('isTokenValid'); + + $this->assertFalse($request->passesCSRFCheck()); + } + + public function testFailStrictCookieCheckWithOnlyLaxCookie() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'nc_sameSiteCookielax' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertFalse($request->passesStrictCookieCheck()); + } + + public function testFailStrictCookieCheckWithOnlyStrictCookie() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'nc_sameSiteCookiestrict' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertFalse($request->passesStrictCookieCheck()); + } + + public function testPassesLaxCookieCheck() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'nc_sameSiteCookielax' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertTrue($request->passesLaxCookieCheck()); + } + + public function testFailsLaxCookieCheckWithOnlyStrictCookie() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'nc_sameSiteCookiestrict' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertFalse($request->passesLaxCookieCheck()); + } + /** * @return array */ @@ -1426,10 +1678,10 @@ class RequestTest extends \Test\TestCase { $token = new CsrfToken($invalidToken); $this->csrfTokenManager - ->expects($this->any()) - ->method('isTokenValid') - ->with($token) - ->willReturn(false); + ->expects($this->any()) + ->method('isTokenValid') + ->with($token) + ->willReturn(false); $this->assertFalse($request->passesCSRFCheck()); } @@ -1449,5 +1701,4 @@ class RequestTest extends \Test\TestCase { $this->assertFalse($request->passesCSRFCheck()); } - } diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index a4f203bacd..487b83c0be 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -31,6 +31,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryExcept use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; +use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Middleware\Security\SecurityMiddleware; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\CSP\ContentSecurityPolicy; @@ -57,28 +58,28 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->controller = $this->getMockBuilder('OCP\AppFramework\Controller') ->disableOriginalConstructor() - ->getMock(); + ->getMock(); $this->reader = new ControllerMethodReflector(); $this->logger = $this->getMockBuilder( - 'OCP\ILogger') - ->disableOriginalConstructor() - ->getMock(); + 'OCP\ILogger') + ->disableOriginalConstructor() + ->getMock(); $this->navigationManager = $this->getMockBuilder( - 'OCP\INavigationManager') - ->disableOriginalConstructor() - ->getMock(); + 'OCP\INavigationManager') + ->disableOriginalConstructor() + ->getMock(); $this->urlGenerator = $this->getMockBuilder( - 'OCP\IURLGenerator') - ->disableOriginalConstructor() - ->getMock(); + 'OCP\IURLGenerator') + ->disableOriginalConstructor() + ->getMock(); $this->request = $this->getMockBuilder( - 'OCP\IRequest') - ->disableOriginalConstructor() - ->getMock(); + 'OCP\IRequest') + ->disableOriginalConstructor() + ->getMock(); $this->contentSecurityPolicyManager = $this->getMockBuilder( - 'OC\Security\CSP\ContentSecurityPolicyManager') - ->disableOriginalConstructor() - ->getMock(); + 'OC\Security\CSP\ContentSecurityPolicyManager') + ->disableOriginalConstructor() + ->getMock(); $this->middleware = $this->getMiddleware(true, true); $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); @@ -211,8 +212,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { */ public function testNoChecks(){ $this->request->expects($this->never()) - ->method('passesCSRFCheck') - ->will($this->returnValue(false)); + ->method('passesCSRFCheck') + ->will($this->returnValue(false)); $sec = $this->getMiddleware(false, false); @@ -256,7 +257,9 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->request->expects($this->once()) ->method('passesCSRFCheck') ->will($this->returnValue(false)); - + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->will($this->returnValue(true)); $this->reader->reflect(__CLASS__, __FUNCTION__); $this->middleware->beforeController(__CLASS__, __FUNCTION__); } @@ -275,19 +278,81 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->middleware->beforeController(__CLASS__, __FUNCTION__); } - /** * @PublicPage */ - public function testFailCsrfCheck(){ + public function testPassesCsrfCheck(){ $this->request->expects($this->once()) ->method('passesCSRFCheck') ->will($this->returnValue(true)); + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->will($this->returnValue(true)); $this->reader->reflect(__CLASS__, __FUNCTION__); $this->middleware->beforeController(__CLASS__, __FUNCTION__); } + /** + * @PublicPage + * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException + */ + public function testFailCsrfCheck(){ + $this->request->expects($this->once()) + ->method('passesCSRFCheck') + ->will($this->returnValue(false)); + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->will($this->returnValue(true)); + + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); + } + + /** + * @PublicPage + * @StrictCookieRequired + * @expectedException \OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException + */ + public function testStrictCookieRequiredCheck() { + $this->request->expects($this->never()) + ->method('passesCSRFCheck'); + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->will($this->returnValue(false)); + + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); + } + + + /** + * @PublicPage + * @NoCSRFRequired + */ + public function testNoStrictCookieRequiredCheck() { + $this->request->expects($this->never()) + ->method('passesStrictCookieCheck') + ->will($this->returnValue(false)); + + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); + } + + /** + * @PublicPage + * @NoCSRFRequired + * @StrictCookieRequired + */ + public function testPassesStrictCookieRequiredCheck() { + $this->request + ->expects($this->once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); + + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); + } /** * @NoCSRFRequired @@ -331,41 +396,64 @@ class SecurityMiddlewareTest extends \Test\TestCase { public function testAfterExceptionReturnsRedirectForNotLoggedInUser() { $this->request = new Request( - [ - 'server' => - [ - 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', - 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' - ] - ], - $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), - $this->getMockBuilder('\OCP\IConfig')->getMock() + [ + 'server' => + [ + 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', + 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' + ] + ], + $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), + $this->getMockBuilder('\OCP\IConfig')->getMock() ); $this->middleware = $this->getMiddleware(false, false); $this->urlGenerator - ->expects($this->once()) - ->method('linkToRoute') - ->with( - 'core.login.showLoginForm', - [ - 'redirect_url' => 'owncloud%2Findex.php%2Fapps%2Fspecialapp', - ] - ) - ->will($this->returnValue('http://localhost/index.php/login?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp')); + ->expects($this->once()) + ->method('linkToRoute') + ->with( + 'core.login.showLoginForm', + [ + 'redirect_url' => 'owncloud%2Findex.php%2Fapps%2Fspecialapp', + ] + ) + ->will($this->returnValue('http://localhost/index.php/login?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp')); $this->logger - ->expects($this->once()) - ->method('debug') - ->with('Current user is not logged in'); + ->expects($this->once()) + ->method('debug') + ->with('Current user is not logged in'); $response = $this->middleware->afterException( - $this->controller, - 'test', - new NotLoggedInException() + $this->controller, + 'test', + new NotLoggedInException() ); - $expected = new RedirectResponse('http://localhost/index.php/login?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp'); $this->assertEquals($expected , $response); } + public function testAfterExceptionRedirectsToWebRootAfterStrictCookieFail() { + $this->request = new Request( + [ + 'server' => [ + 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', + 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp', + ], + ], + $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), + $this->getMockBuilder('\OCP\IConfig')->getMock() + ); + + $this->middleware = $this->getMiddleware(false, false); + $response = $this->middleware->afterException( + $this->controller, + 'test', + new StrictCookieMissingException() + ); + + $expected = new RedirectResponse(\OC::$WEBROOT); + $this->assertEquals($expected , $response); + } + + /** * @return array */ @@ -389,36 +477,34 @@ class SecurityMiddlewareTest extends \Test\TestCase { */ public function testAfterExceptionReturnsTemplateResponse(SecurityException $exception) { $this->request = new Request( - [ - 'server' => - [ - 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', - 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' - ] - ], - $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), - $this->getMockBuilder('\OCP\IConfig')->getMock() + [ + 'server' => + [ + 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', + 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' + ] + ], + $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), + $this->getMockBuilder('\OCP\IConfig')->getMock() ); $this->middleware = $this->getMiddleware(false, false); $this->logger - ->expects($this->once()) - ->method('debug') - ->with($exception->getMessage()); + ->expects($this->once()) + ->method('debug') + ->with($exception->getMessage()); $response = $this->middleware->afterException( - $this->controller, - 'test', - $exception + $this->controller, + 'test', + $exception ); - $expected = new TemplateResponse('core', '403', ['file' => $exception->getMessage()], 'guest'); $expected->setStatus($exception->getCode()); $this->assertEquals($expected , $response); } - public function testAfterAjaxExceptionReturnsJSONError(){ $response = $this->middleware->afterException($this->controller, 'test', - $this->secAjaxException); + $this->secAjaxException); $this->assertTrue($response instanceof JSONResponse); } @@ -440,10 +526,10 @@ class SecurityMiddlewareTest extends \Test\TestCase { ->method('getDefaultPolicy') ->willReturn($defaultPolicy); $this->contentSecurityPolicyManager - ->expects($this->once()) - ->method('mergePolicies') - ->with($defaultPolicy, $currentPolicy) - ->willReturn($mergedPolicy); + ->expects($this->once()) + ->method('mergePolicies') + ->with($defaultPolicy, $currentPolicy) + ->willReturn($mergedPolicy); $response->expects($this->once()) ->method('setContentSecurityPolicy') ->with($mergedPolicy);