From 7c078a81b4d55caa9f0381d11a27cfe5e5f1c31a Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 15 Sep 2016 11:26:24 +0200 Subject: [PATCH 1/3] Add trict CSP to OCS responses If a repsonse now explicitly has the Empty CSP set then the middleware won't touch it. --- .../AppFramework/Middleware/Security/SecurityMiddleware.php | 5 +++++ lib/private/AppFramework/OCS/BaseResponse.php | 3 ++- lib/public/AppFramework/Http/Response.php | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 3bfef2df02..5e253d0954 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -37,6 +37,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\CSP\ContentSecurityPolicyManager; use OCP\AppFramework\Http\ContentSecurityPolicy; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Middleware; @@ -182,6 +183,10 @@ class SecurityMiddleware extends Middleware { public function afterController($controller, $methodName, 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); diff --git a/lib/private/AppFramework/OCS/BaseResponse.php b/lib/private/AppFramework/OCS/BaseResponse.php index fa22498ac0..59b8660a38 100644 --- a/lib/private/AppFramework/OCS/BaseResponse.php +++ b/lib/private/AppFramework/OCS/BaseResponse.php @@ -23,6 +23,7 @@ namespace OC\AppFramework\OCS; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; use OCP\AppFramework\Http\Response; abstract class BaseResponse extends Response { @@ -67,7 +68,7 @@ abstract class BaseResponse extends Response { $this->setETag($dataResponse->getETag()); $this->setLastModified($dataResponse->getLastModified()); $this->setCookies($dataResponse->getCookies()); - $this->setContentSecurityPolicy($dataResponse->getContentSecurityPolicy()); + $this->setContentSecurityPolicy(new EmptyContentSecurityPolicy()); if ($format === 'json') { $this->addHeader( diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index 0b162b453a..8591d6abc6 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -248,18 +248,18 @@ class Response { /** * Set a Content-Security-Policy - * @param ContentSecurityPolicy $csp Policy to set for the response object + * @param EmptyContentSecurityPolicy $csp Policy to set for the response object * @return $this * @since 8.1.0 */ - public function setContentSecurityPolicy(ContentSecurityPolicy $csp) { + public function setContentSecurityPolicy(EmptyContentSecurityPolicy $csp) { $this->contentSecurityPolicy = $csp; return $this; } /** * Get the currently used Content-Security-Policy - * @return ContentSecurityPolicy|null Used Content-Security-Policy or null if + * @return EmptyContentSecurityPolicy|null Used Content-Security-Policy or null if * none specified. * @since 8.1.0 */ From 6dace7f6adf7721ee043c3a03fc7091eb5a7e82f Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 15 Sep 2016 12:12:30 +0200 Subject: [PATCH 2/3] Add tests --- .../Controller/OCSControllerTest.php | 22 ++++--- .../Security/SecurityMiddlewareTest.php | 59 +++++++++---------- 2 files changed, 43 insertions(+), 38 deletions(-) diff --git a/tests/lib/AppFramework/Controller/OCSControllerTest.php b/tests/lib/AppFramework/Controller/OCSControllerTest.php index c1f8e4a657..0d379a8822 100644 --- a/tests/lib/AppFramework/Controller/OCSControllerTest.php +++ b/tests/lib/AppFramework/Controller/OCSControllerTest.php @@ -26,6 +26,7 @@ namespace Test\AppFramework\Controller; use OC\AppFramework\Http\Request; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; use OCP\AppFramework\OCSController; use OCP\IConfig; use OCP\Security\ISecureRandom; @@ -92,8 +93,9 @@ class OCSControllerTest extends \Test\TestCase { $params = new DataResponse(['test' => 'hi']); - $out = $controller->buildResponse($params, 'xml')->render(); - $this->assertEquals($expected, $out); + $response = $controller->buildResponse($params, 'xml'); + $this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy())); + $this->assertEquals($expected, $response->render()); } public function testJSON() { @@ -111,8 +113,10 @@ class OCSControllerTest extends \Test\TestCase { '"totalitems":"","itemsperpage":""},"data":{"test":"hi"}}}'; $params = new DataResponse(['test' => 'hi']); - $out = $controller->buildResponse($params, 'json')->render(); - $this->assertEquals($expected, $out); + $response = $controller->buildResponse($params, 'json'); + $this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy())); + $this->assertEquals($expected, $response->render()); + $this->assertEquals($expected, $response->render()); } public function testXMLV2() { @@ -141,8 +145,9 @@ class OCSControllerTest extends \Test\TestCase { $params = new DataResponse(['test' => 'hi']); - $out = $controller->buildResponse($params, 'xml')->render(); - $this->assertEquals($expected, $out); + $response = $controller->buildResponse($params, 'xml'); + $this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy())); + $this->assertEquals($expected, $response->render()); } public function testJSONV2() { @@ -159,7 +164,8 @@ class OCSControllerTest extends \Test\TestCase { $expected = '{"ocs":{"meta":{"status":"ok","statuscode":200,"message":"OK"},"data":{"test":"hi"}}}'; $params = new DataResponse(['test' => 'hi']); - $out = $controller->buildResponse($params, 'json')->render(); - $this->assertEquals($expected, $out); + $response = $controller->buildResponse($params, 'json'); + $this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy())); + $this->assertEquals($expected, $response->render()); } } diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index bfd810bc6b..55bf3e46e0 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -37,13 +37,17 @@ use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\CSP\ContentSecurityPolicy; use OC\Security\CSP\ContentSecurityPolicyManager; use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\JSONResponse; +use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\TemplateResponse; +use OCP\IConfig; use OCP\ILogger; use OCP\INavigationManager; use OCP\IRequest; use OCP\IURLGenerator; +use OCP\Security\ISecureRandom; class SecurityMiddlewareTest extends \Test\TestCase { @@ -72,30 +76,13 @@ class SecurityMiddlewareTest extends \Test\TestCase { protected function setUp() { parent::setUp(); - $this->controller = $this->getMockBuilder('OCP\AppFramework\Controller') - ->disableOriginalConstructor() - ->getMock(); + $this->controller = $this->createMock(Controller::class); $this->reader = new ControllerMethodReflector(); - $this->logger = $this->getMockBuilder( - 'OCP\ILogger') - ->disableOriginalConstructor() - ->getMock(); - $this->navigationManager = $this->getMockBuilder( - 'OCP\INavigationManager') - ->disableOriginalConstructor() - ->getMock(); - $this->urlGenerator = $this->getMockBuilder( - 'OCP\IURLGenerator') - ->disableOriginalConstructor() - ->getMock(); - $this->request = $this->getMockBuilder( - 'OCP\IRequest') - ->disableOriginalConstructor() - ->getMock(); - $this->contentSecurityPolicyManager = $this->getMockBuilder( - 'OC\Security\CSP\ContentSecurityPolicyManager') - ->disableOriginalConstructor() - ->getMock(); + $this->logger = $this->createMock(ILogger::class); + $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->middleware = $this->getMiddleware(true, true); $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); @@ -459,8 +446,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' ] ], - $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), - $this->getMockBuilder('\OCP\IConfig')->getMock() + $this->createMock(ISecureRandom::class), + $this->createMock(IConfig::class) ); $this->middleware = $this->getMiddleware(false, false); $this->urlGenerator @@ -494,8 +481,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp', ], ], - $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), - $this->getMockBuilder('\OCP\IConfig')->getMock() + $this->createMock(ISecureRandom::class), + $this->createMock(IConfig::class) ); $this->middleware = $this->getMiddleware(false, false); @@ -540,8 +527,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' ] ], - $this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(), - $this->getMockBuilder('\OCP\IConfig')->getMock() + $this->createMock(ISecureRandom::class), + $this->createMock(IConfig::class) ); $this->middleware = $this->getMiddleware(false, false); $this->logger @@ -566,7 +553,7 @@ class SecurityMiddlewareTest extends \Test\TestCase { } public function testAfterController() { - $response = $this->getMockBuilder('\OCP\AppFramework\Http\Response')->disableOriginalConstructor()->getMock(); + $response = $this->createMock(Response::class); $defaultPolicy = new ContentSecurityPolicy(); $defaultPolicy->addAllowedImageDomain('defaultpolicy'); $currentPolicy = new ContentSecurityPolicy(); @@ -592,4 +579,16 @@ class SecurityMiddlewareTest extends \Test\TestCase { $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); + } } From e392f669c09a520e64ea82b7d22350871a05e089 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 15 Sep 2016 12:41:39 +0200 Subject: [PATCH 3/3] Make OCS intergration tests check for CSP * Very hacky in simple test but at least we test --- .../features/bootstrap/Sharing.php | 23 +++++++++++++++++++ build/integration/features/sharing-v1.feature | 2 ++ 2 files changed, 25 insertions(+) diff --git a/build/integration/features/bootstrap/Sharing.php b/build/integration/features/bootstrap/Sharing.php index 3a50b1917a..a4a9b846cf 100644 --- a/build/integration/features/bootstrap/Sharing.php +++ b/build/integration/features/bootstrap/Sharing.php @@ -514,5 +514,28 @@ trait Sharing { throw new \Exception('Expected the same link share to be returned'); } } + + /** + * @Then The following headers should be set + * @param \Behat\Gherkin\Node\TableNode $table + * @throws \Exception + */ + public function theFollowingHeadersShouldBeSet(\Behat\Gherkin\Node\TableNode $table) { + foreach($table->getTable() as $header) { + $headerName = $header[0]; + $expectedHeaderValue = $header[1]; + $returnedHeader = $this->response->getHeader($headerName); + if($returnedHeader !== $expectedHeaderValue) { + throw new \Exception( + sprintf( + "Expected value '%s' for header '%s', got '%s'", + $expectedHeaderValue, + $headerName, + $returnedHeader + ) + ); + } + } + } } diff --git a/build/integration/features/sharing-v1.feature b/build/integration/features/sharing-v1.feature index edd599da55..3c769fba3d 100644 --- a/build/integration/features/sharing-v1.feature +++ b/build/integration/features/sharing-v1.feature @@ -13,6 +13,8 @@ Feature: sharing | shareType | 0 | Then the OCS status code should be "100" And the HTTP status code should be "200" + And The following headers should be set + | Content-Security-Policy | default-src 'none' | Scenario: Creating a share with a group Given user "user0" exists