From c257cd57d46143b6007f3c2cb80576c7320dc19e Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 22 Sep 2017 12:21:44 +0200 Subject: [PATCH] Handle SameSiteCookie check for index.php in AppFramework Middleware Signed-off-by: Roeland Jago Douma --- lib/base.php | 26 ++-- lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + .../DependencyInjection/DIContainer.php | 8 ++ lib/private/AppFramework/Http/Request.php | 2 +- .../LaxSameSiteCookieFailedException.php | 38 +++++ .../Security/SameSiteCookieMiddleware.php | 106 ++++++++++++++ tests/lib/AppFramework/Http/RequestTest.php | 14 +- .../Security/SameSiteCookieMiddlewareTest.php | 133 ++++++++++++++++++ 9 files changed, 313 insertions(+), 18 deletions(-) create mode 100644 lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php create mode 100644 lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php create mode 100644 tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php diff --git a/lib/base.php b/lib/base.php index 29778f02a4..76069303a5 100644 --- a/lib/base.php +++ b/lib/base.php @@ -559,24 +559,20 @@ class OC { if($currentUrl === '/index.php/apps/user_saml/saml/acs' || $currentUrl === '/apps/user_saml/saml/acs') { return; } - // For the "index.php" endpoint only a lax cookie is required. + // index.php routes are handled in the middleware if($processingScript === 'index.php') { - if(!$request->passesLaxCookieCheck()) { - self::sendSameSiteCookies(); - header('Location: '.$_SERVER['REQUEST_URI']); + return; + } + + // 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(); } - } 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(); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 92793969c1..29bca415ed 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -306,12 +306,14 @@ return array( 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', + 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\LaxSameSiteCookieFailedException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotAdminException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotAdminException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotConfirmedException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotConfirmedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotLoggedInException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\SecurityException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/SecurityException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\StrictCookieMissingException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php', 'OC\\AppFramework\\Middleware\\Security\\RateLimitingMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\SameSiteCookieMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\SecurityMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php', 'OC\\AppFramework\\Middleware\\SessionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/SessionMiddleware.php', 'OC\\AppFramework\\OCS\\BaseResponse' => $baseDir . '/lib/private/AppFramework/OCS/BaseResponse.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index e8ce53061a..4cfbbcc2e3 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -336,12 +336,14 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', + 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\LaxSameSiteCookieFailedException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotAdminException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotAdminException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotConfirmedException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotConfirmedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotLoggedInException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\SecurityException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/SecurityException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\StrictCookieMissingException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php', 'OC\\AppFramework\\Middleware\\Security\\RateLimitingMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\SameSiteCookieMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\SecurityMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php', 'OC\\AppFramework\\Middleware\\SessionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/SessionMiddleware.php', 'OC\\AppFramework\\OCS\\BaseResponse' => __DIR__ . '/../../..' . '/lib/private/AppFramework/OCS/BaseResponse.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index e33ffdca96..2290f0d004 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -291,9 +291,17 @@ class DIContainer extends SimpleContainer implements IAppContainer { ); }); + $this->registerService(OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware::class, function (SimpleContainer $c) { + return new OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware( + $c['Request'], + $c['ControllerMethodReflector'] + ); + }); + $middleWares = &$this->middleWares; $this->registerService('MiddlewareDispatcher', function($c) use (&$middleWares) { $dispatcher = new MiddlewareDispatcher(); + $dispatcher->registerMiddleware($c[OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware::class]); $dispatcher->registerMiddleware($c['CORSMiddleware']); $dispatcher->registerMiddleware($c['OCSMiddleware']); $dispatcher->registerMiddleware($c['SecurityMiddleware']); diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index 7e21434c73..d2347ba4b2 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -507,7 +507,7 @@ class Request implements \ArrayAccess, \Countable, IRequest { * * @return array */ - protected function getCookieParams() { + public function getCookieParams() { return session_get_cookie_params(); } diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php new file mode 100644 index 0000000000..5161886d02 --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php @@ -0,0 +1,38 @@ + + * + * @author Roeland Jago Douma + * + * @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 LaxSameSiteCookieFailedException is thrown when a request doesn't pass + * the required LaxCookie check on index.php + * + * @package OC\AppFramework\Middleware\Security\Exceptions + */ +class LaxSameSiteCookieFailedException extends SecurityException { + public function __construct() { + parent::__construct('Lax Same Site Cookie is invalid in request.', Http::STATUS_PRECONDITION_FAILED); + } +} diff --git a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php new file mode 100644 index 0000000000..22a9246d66 --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php @@ -0,0 +1,106 @@ + + * + * @author Roeland Jago Douma + * + * @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; + +use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException; +use OC\AppFramework\Utility\ControllerMethodReflector; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Middleware; + +class SameSiteCookieMiddleware extends Middleware { + + /** @var Request */ + private $request; + + /** @var ControllerMethodReflector */ + private $reflector; + + public function __construct(Request $request, + ControllerMethodReflector $reflector) { + $this->request = $request; + $this->reflector = $reflector; + } + + public function beforeController($controller, $methodName) { + $requestUri = $this->request->getScriptName(); + $processingScript = explode('/', $requestUri); + $processingScript = $processingScript[count($processingScript)-1]; + + if ($processingScript !== 'index.php') { + return; + } + + $noSSC = $this->reflector->hasAnnotation('NoSameSiteCookieRequired'); + if ($noSSC) { + return; + } + + if (!$this->request->passesLaxCookieCheck()) { + throw new LaxSameSiteCookieFailedException(); + } + } + + public function afterException($controller, $methodName, \Exception $exception) { + if ($exception instanceof LaxSameSiteCookieFailedException) { + $respone = new Response(); + $respone->setStatus(Http::STATUS_FOUND); + $respone->addHeader('Location', $this->request->getRequestUri()); + + $this->setSameSiteCookie(); + + return $respone; + } + + throw $exception; + } + + protected function setSameSiteCookie() { + $cookieParams = $this->request->getCookieParams(); + $secureCookie = ($cookieParams['secure'] === true) ? 'secure; ' : ''; + $policies = [ + 'lax', + 'strict', + ]; + + // Append __Host to the cookie if it meets the requirements + $cookiePrefix = ''; + if($cookieParams['secure'] === true && $cookieParams['path'] === '/') { + $cookiePrefix = '__Host-'; + } + + foreach($policies as $policy) { + header( + sprintf( + 'Set-Cookie: %snc_sameSiteCookie%s=true; path=%s; httponly;' . $secureCookie . 'expires=Fri, 31-Dec-2100 23:59:59 GMT; SameSite=%s', + $cookiePrefix, + $policy, + $cookieParams['path'], + $policy + ), + false + ); + } + } +} diff --git a/tests/lib/AppFramework/Http/RequestTest.php b/tests/lib/AppFramework/Http/RequestTest.php index f80bffcb48..40698ef8d7 100644 --- a/tests/lib/AppFramework/Http/RequestTest.php +++ b/tests/lib/AppFramework/Http/RequestTest.php @@ -1568,8 +1568,18 @@ class RequestTest extends \Test\TestCase { } public function testGetCookieParams() { - $request = $this->createMock(Request::class); - $actual = $this->invokePrivate($request, 'getCookieParams'); + /** @var Request $request */ + $request = $this->getMockBuilder(Request::class) + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $actual = $request->getCookieParams(); $this->assertSame(session_get_cookie_params(), $actual); } diff --git a/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php new file mode 100644 index 0000000000..bd1568bcd6 --- /dev/null +++ b/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php @@ -0,0 +1,133 @@ + + * + * @author Roeland Jago Douma + * + * @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 Test\AppFramework\Middleware\Security; + +use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException; +use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; +use OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware; +use OC\AppFramework\Utility\ControllerMethodReflector; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use Test\TestCase; + +class SameSiteCookieMiddlewareTest extends TestCase { + + /** @var SameSiteCookieMiddleware */ + private $middleware; + + /** @var Request|\PHPUnit_Framework_MockObject_MockObject */ + private $request; + + /** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject */ + private $reflector; + + public function setUp() { + parent::setUp(); + + $this->request = $this->createMock(Request::class); + $this->reflector = $this->createMock(ControllerMethodReflector::class); + $this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector); + } + + public function testBeforeControllerNoIndex() { + $this->request->method('getScriptName') + ->willReturn('/ocs/v2.php'); + + $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + } + + public function testBeforeControllerIndexHasAnnotation() { + $this->request->method('getScriptName') + ->willReturn('/index.php'); + + $this->reflector->method('hasAnnotation') + ->with('NoSameSiteCookieRequired') + ->willReturn(true); + + $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + } + + public function testBeforeControllerIndexNoAnnotationPassingCheck() { + $this->request->method('getScriptName') + ->willReturn('/index.php'); + + $this->reflector->method('hasAnnotation') + ->with('NoSameSiteCookieRequired') + ->willReturn(false); + + $this->request->method('passesLaxCookieCheck') + ->willReturn(true); + + $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + } + + public function testBeforeControllerIndexNoAnnotationFailingCheck() { + $this->expectException(LaxSameSiteCookieFailedException::class); + + $this->request->method('getScriptName') + ->willReturn('/index.php'); + + $this->reflector->method('hasAnnotation') + ->with('NoSameSiteCookieRequired') + ->willReturn(false); + + $this->request->method('passesLaxCookieCheck') + ->willReturn(false); + + $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + } + + public function testAfterExceptionNoLaxCookie() { + $ex = new SecurityException(); + + try { + $this->middleware->afterException($this->createMock(Controller::class), 'foo', $ex); + $this->fail(); + } catch (\Exception $e) { + $this->assertSame($ex, $e); + } + } + + public function testAfterExceptionLaxCookie() { + $ex = new LaxSameSiteCookieFailedException(); + + $this->request->method('getRequestUri') + ->willReturn('/myrequri'); + + $middleware = $this->getMockBuilder(SameSiteCookieMiddleware::class) + ->setConstructorArgs([$this->request, $this->reflector]) + ->setMethods(['setSameSiteCookie']) + ->getMock(); + + $middleware->expects($this->once()) + ->method('setSameSiteCookie'); + + $resp = $middleware->afterException($this->createMock(Controller::class), 'foo', $ex); + + $this->assertSame(Http::STATUS_FOUND, $resp->getStatus()); + + $headers = $resp->getHeaders(); + $this->assertSame('/myrequri', $headers['Location']); + } +}