From 37a4282c7ae27c518ce7143be491a00a651e4f4a Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sat, 27 Jul 2019 16:04:51 +0200 Subject: [PATCH] Split up security middleware With upcoming work for the feature policy header. Splitting this in smaller classes that just do 1 thing makes sense. I rather have a few small classes that are tiny and do 1 thing right (and we all understand what is going on) than have big ones. Signed-off-by: Roeland Jago Douma --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../DependencyInjection/DIContainer.php | 10 +- .../Middleware/Security/CSPMiddleware.php | 80 ++++++++++ .../Security/SecurityMiddleware.php | 40 ----- .../Middleware/Security/CSPMiddlewareTest.php | 149 ++++++++++++++++++ .../Security/SecurityMiddlewareTest.php | 97 ------------ 7 files changed, 238 insertions(+), 140 deletions(-) create mode 100644 lib/private/AppFramework/Middleware/Security/CSPMiddleware.php create mode 100644 tests/lib/AppFramework/Middleware/Security/CSPMiddlewareTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index d8d9227cb07..019af95e296 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -457,6 +457,7 @@ return array( 'OC\\AppFramework\\Middleware\\PublicShare\\PublicShareMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 043846e04df..87d0b002800 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -491,6 +491,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\AppFramework\\Middleware\\PublicShare\\PublicShareMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.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', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 6d337bb9327..f47af340b38 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -220,13 +220,17 @@ class DIContainer extends SimpleContainer implements IAppContainer { $server->getUserSession()->isLoggedIn(), $server->getGroupManager()->isAdmin($this->getUserId()), $server->getUserSession()->getUser() !== null && $server->query(ISubAdmin::class)->isSubAdmin($server->getUserSession()->getUser()), - $server->getContentSecurityPolicyManager(), - $server->getCsrfTokenManager(), - $server->getContentSecurityPolicyNonceManager(), $server->getAppManager(), $server->getL10N('lib') ); $dispatcher->registerMiddleware($securityMiddleware); + $dispatcher->registerMiddleware( + new OC\AppFramework\Middleware\Security\CSPMiddleware( + $server->query(OC\Security\CSP\ContentSecurityPolicyManager::class), + $server->query(OC\Security\CSP\ContentSecurityPolicyNonceManager::class), + $server->query(OC\Security\CSRF\CsrfTokenManager::class) + ) + ); $dispatcher->registerMiddleware( new OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware( $c->query(IControllerMethodReflector::class), diff --git a/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php b/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php new file mode 100644 index 00000000000..0eaeda2b1bd --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php @@ -0,0 +1,80 @@ + + * + * @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\Security\CSP\ContentSecurityPolicyManager; +use OC\Security\CSP\ContentSecurityPolicyNonceManager; +use OC\Security\CSRF\CsrfTokenManager; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\ContentSecurityPolicy; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; +use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Middleware; + +class CSPMiddleware extends Middleware { + + /** @var ContentSecurityPolicyManager */ + private $contentSecurityPolicyManager; + /** @var ContentSecurityPolicyNonceManager */ + private $cspNonceManager; + /** @var CsrfTokenManager */ + private $csrfTokenManager; + + public function __construct(ContentSecurityPolicyManager $policyManager, + ContentSecurityPolicyNonceManager $cspNonceManager, + CsrfTokenManager $csrfTokenManager) { + $this->contentSecurityPolicyManager = $policyManager; + $this->cspNonceManager = $cspNonceManager; + $this->csrfTokenManager = $csrfTokenManager; + } + + /** + * Performs the default CSP modifications that may be injected by other + * applications + * + * @param Controller $controller + * @param string $methodName + * @param Response $response + * @return Response + */ + public function afterController($controller, $methodName, Response $response): Response { + $policy = !is_null($response->getContentSecurityPolicy()) ? $response->getContentSecurityPolicy() : new ContentSecurityPolicy(); + + if (get_class($policy) === EmptyContentSecurityPolicy::class) { + return $response; + } + + $defaultPolicy = $this->contentSecurityPolicyManager->getDefaultPolicy(); + $defaultPolicy = $this->contentSecurityPolicyManager->mergePolicies($defaultPolicy, $policy); + + if($this->cspNonceManager->browserSupportsCspV3()) { + $defaultPolicy->useJsNonce($this->csrfTokenManager->getToken()->getEncryptedValue()); + } + + $response->setContentSecurityPolicy($defaultPolicy); + + return $response; + } +} diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index fef3f226e15..4f380f07d91 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -84,12 +84,6 @@ class SecurityMiddleware extends Middleware { private $isAdminUser; /** @var bool */ private $isSubAdmin; - /** @var ContentSecurityPolicyManager */ - private $contentSecurityPolicyManager; - /** @var CsrfTokenManager */ - private $csrfTokenManager; - /** @var ContentSecurityPolicyNonceManager */ - private $cspNonceManager; /** @var IAppManager */ private $appManager; /** @var IL10N */ @@ -104,9 +98,6 @@ class SecurityMiddleware extends Middleware { bool $isLoggedIn, bool $isAdminUser, bool $isSubAdmin, - ContentSecurityPolicyManager $contentSecurityPolicyManager, - CsrfTokenManager $csrfTokenManager, - ContentSecurityPolicyNonceManager $cspNonceManager, IAppManager $appManager, IL10N $l10n ) { @@ -119,9 +110,6 @@ class SecurityMiddleware extends Middleware { $this->isLoggedIn = $isLoggedIn; $this->isAdminUser = $isAdminUser; $this->isSubAdmin = $isSubAdmin; - $this->contentSecurityPolicyManager = $contentSecurityPolicyManager; - $this->csrfTokenManager = $csrfTokenManager; - $this->cspNonceManager = $cspNonceManager; $this->appManager = $appManager; $this->l10n = $l10n; } @@ -203,34 +191,6 @@ class SecurityMiddleware extends Middleware { } } - /** - * Performs the default CSP modifications that may be injected by other - * applications - * - * @param Controller $controller - * @param string $methodName - * @param Response $response - * @return Response - */ - public function afterController($controller, $methodName, Response $response): Response { - $policy = !is_null($response->getContentSecurityPolicy()) ? $response->getContentSecurityPolicy() : new ContentSecurityPolicy(); - - if (get_class($policy) === EmptyContentSecurityPolicy::class) { - return $response; - } - - $defaultPolicy = $this->contentSecurityPolicyManager->getDefaultPolicy(); - $defaultPolicy = $this->contentSecurityPolicyManager->mergePolicies($defaultPolicy, $policy); - - if($this->cspNonceManager->browserSupportsCspV3()) { - $defaultPolicy->useJsNonce($this->csrfTokenManager->getToken()->getEncryptedValue()); - } - - $response->setContentSecurityPolicy($defaultPolicy); - - return $response; - } - /** * If an SecurityException is being caught, ajax requests return a JSON error * response and non ajax requests redirect to the index diff --git a/tests/lib/AppFramework/Middleware/Security/CSPMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CSPMiddlewareTest.php new file mode 100644 index 00000000000..9f2b4e399fb --- /dev/null +++ b/tests/lib/AppFramework/Middleware/Security/CSPMiddlewareTest.php @@ -0,0 +1,149 @@ + + * + * @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\Middleware\Security\CSPMiddleware; +use OC\Security\CSP\ContentSecurityPolicy; +use OC\Security\CSP\ContentSecurityPolicyManager; +use OC\Security\CSP\ContentSecurityPolicyNonceManager; +use OC\Security\CSRF\CsrfToken; +use OC\Security\CSRF\CsrfTokenManager; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; +use OCP\AppFramework\Http\Response; +use PHPUnit\Framework\MockObject\MockObject; + +class CSPMiddlewareTest extends \Test\TestCase { + + /** @var CSPMiddleware|MockObject */ + private $middleware; + /** @var Controller|MockObject */ + private $controller; + /** @var ContentSecurityPolicyManager|MockObject */ + private $contentSecurityPolicyManager; + /** @var CsrfTokenManager|MockObject */ + private $csrfTokenManager; + /** @var ContentSecurityPolicyNonceManager|MockObject */ + private $cspNonceManager; + + protected function setUp() { + parent::setUp(); + + $this->controller = $this->createMock(Controller::class); + $this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class); + $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); + $this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class); + $this->middleware = new CSPMiddleware( + $this->contentSecurityPolicyManager, + $this->cspNonceManager, + $this->csrfTokenManager + ); + } + + public function testAfterController() { + $this->cspNonceManager + ->expects($this->once()) + ->method('browserSupportsCspV3') + ->willReturn(false); + $response = $this->createMock(Response::class); + $defaultPolicy = new ContentSecurityPolicy(); + $defaultPolicy->addAllowedImageDomain('defaultpolicy'); + $currentPolicy = new ContentSecurityPolicy(); + $currentPolicy->addAllowedConnectDomain('currentPolicy'); + $mergedPolicy = new ContentSecurityPolicy(); + $mergedPolicy->addAllowedMediaDomain('mergedPolicy'); + $response + ->expects($this->exactly(2)) + ->method('getContentSecurityPolicy') + ->willReturn($currentPolicy); + $this->contentSecurityPolicyManager + ->expects($this->once()) + ->method('getDefaultPolicy') + ->willReturn($defaultPolicy); + $this->contentSecurityPolicyManager + ->expects($this->once()) + ->method('mergePolicies') + ->with($defaultPolicy, $currentPolicy) + ->willReturn($mergedPolicy); + $response->expects($this->once()) + ->method('setContentSecurityPolicy') + ->with($mergedPolicy); + + $this->middleware->afterController($this->controller, 'test', $response); + } + + public function testAfterControllerEmptyCSP() { + $response = $this->createMock(Response::class); + $emptyPolicy = new EmptyContentSecurityPolicy(); + $response->expects($this->any()) + ->method('getContentSecurityPolicy') + ->willReturn($emptyPolicy); + $response->expects($this->never()) + ->method('setContentSecurityPolicy'); + + $this->middleware->afterController($this->controller, 'test', $response); + } + + public function testAfterControllerWithContentSecurityPolicy3Support() { + $this->cspNonceManager + ->expects($this->once()) + ->method('browserSupportsCspV3') + ->willReturn(true); + $token = $this->createMock(CsrfToken::class); + $token + ->expects($this->once()) + ->method('getEncryptedValue') + ->willReturn('MyEncryptedToken'); + $this->csrfTokenManager + ->expects($this->once()) + ->method('getToken') + ->willReturn($token); + $response = $this->createMock(Response::class); + $defaultPolicy = new ContentSecurityPolicy(); + $defaultPolicy->addAllowedImageDomain('defaultpolicy'); + $currentPolicy = new ContentSecurityPolicy(); + $currentPolicy->addAllowedConnectDomain('currentPolicy'); + $mergedPolicy = new ContentSecurityPolicy(); + $mergedPolicy->addAllowedMediaDomain('mergedPolicy'); + $response + ->expects($this->exactly(2)) + ->method('getContentSecurityPolicy') + ->willReturn($currentPolicy); + $this->contentSecurityPolicyManager + ->expects($this->once()) + ->method('getDefaultPolicy') + ->willReturn($defaultPolicy); + $this->contentSecurityPolicyManager + ->expects($this->once()) + ->method('mergePolicies') + ->with($defaultPolicy, $currentPolicy) + ->willReturn($mergedPolicy); + $response->expects($this->once()) + ->method('setContentSecurityPolicy') + ->with($mergedPolicy); + + $this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response)); + } +} diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index ab243616be0..6a1adf03b2f 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -72,12 +72,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { private $navigationManager; /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ private $urlGenerator; - /** @var ContentSecurityPolicyManager|\PHPUnit_Framework_MockObject_MockObject */ - private $contentSecurityPolicyManager; - /** @var CsrfTokenManager|\PHPUnit_Framework_MockObject_MockObject */ - private $csrfTokenManager; - /** @var ContentSecurityPolicyNonceManager|\PHPUnit_Framework_MockObject_MockObject */ - private $cspNonceManager; /** @var IAppManager|\PHPUnit_Framework_MockObject_MockObject */ private $appManager; /** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */ @@ -92,9 +86,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->navigationManager = $this->createMock(INavigationManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->request = $this->createMock(IRequest::class); - $this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class); - $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); - $this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class); $this->l10n = $this->createMock(IL10N::class); $this->middleware = $this->getMiddleware(true, true, false); $this->secException = new SecurityException('hey', false); @@ -118,9 +109,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { $isLoggedIn, $isAdminUser, $isSubAdmin, - $this->contentSecurityPolicyManager, - $this->csrfTokenManager, - $this->cspNonceManager, $this->appManager, $this->l10n ); @@ -611,91 +599,6 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->assertTrue($response instanceof JSONResponse); } - public function testAfterController() { - $this->cspNonceManager - ->expects($this->once()) - ->method('browserSupportsCspV3') - ->willReturn(false); - $response = $this->createMock(Response::class); - $defaultPolicy = new ContentSecurityPolicy(); - $defaultPolicy->addAllowedImageDomain('defaultpolicy'); - $currentPolicy = new ContentSecurityPolicy(); - $currentPolicy->addAllowedConnectDomain('currentPolicy'); - $mergedPolicy = new ContentSecurityPolicy(); - $mergedPolicy->addAllowedMediaDomain('mergedPolicy'); - $response - ->expects($this->exactly(2)) - ->method('getContentSecurityPolicy') - ->willReturn($currentPolicy); - $this->contentSecurityPolicyManager - ->expects($this->once()) - ->method('getDefaultPolicy') - ->willReturn($defaultPolicy); - $this->contentSecurityPolicyManager - ->expects($this->once()) - ->method('mergePolicies') - ->with($defaultPolicy, $currentPolicy) - ->willReturn($mergedPolicy); - $response->expects($this->once()) - ->method('setContentSecurityPolicy') - ->with($mergedPolicy); - - $this->middleware->afterController($this->controller, 'test', $response); - } - - public function testAfterControllerEmptyCSP() { - $response = $this->createMock(Response::class); - $emptyPolicy = new EmptyContentSecurityPolicy(); - $response->expects($this->any()) - ->method('getContentSecurityPolicy') - ->willReturn($emptyPolicy); - $response->expects($this->never()) - ->method('setContentSecurityPolicy'); - - $this->middleware->afterController($this->controller, 'test', $response); - } - - public function testAfterControllerWithContentSecurityPolicy3Support() { - $this->cspNonceManager - ->expects($this->once()) - ->method('browserSupportsCspV3') - ->willReturn(true); - $token = $this->createMock(CsrfToken::class); - $token - ->expects($this->once()) - ->method('getEncryptedValue') - ->willReturn('MyEncryptedToken'); - $this->csrfTokenManager - ->expects($this->once()) - ->method('getToken') - ->willReturn($token); - $response = $this->createMock(Response::class); - $defaultPolicy = new ContentSecurityPolicy(); - $defaultPolicy->addAllowedImageDomain('defaultpolicy'); - $currentPolicy = new ContentSecurityPolicy(); - $currentPolicy->addAllowedConnectDomain('currentPolicy'); - $mergedPolicy = new ContentSecurityPolicy(); - $mergedPolicy->addAllowedMediaDomain('mergedPolicy'); - $response - ->expects($this->exactly(2)) - ->method('getContentSecurityPolicy') - ->willReturn($currentPolicy); - $this->contentSecurityPolicyManager - ->expects($this->once()) - ->method('getDefaultPolicy') - ->willReturn($defaultPolicy); - $this->contentSecurityPolicyManager - ->expects($this->once()) - ->method('mergePolicies') - ->with($defaultPolicy, $currentPolicy) - ->willReturn($mergedPolicy); - $response->expects($this->once()) - ->method('setContentSecurityPolicy') - ->with($mergedPolicy); - - $this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response)); - } - public function dataRestrictedApp() { return [ [false, false, false,], -- 2.39.5