From a05b8b79534fcd46341ae7bfd28cb34e9ff88ced Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 23 Nov 2016 12:53:44 +0100 Subject: [PATCH] Harden cookies more appropriate This adds the __Host- prefix to the same-site cookies. This is a small but yet nice security hardening. See https://googlechrome.github.io/samples/cookie-prefixes/ for the implications. Fixes https://github.com/nextcloud/server/issues/1412 Signed-off-by: Lukas Reschke --- lib/base.php | 10 ++- lib/private/AppFramework/Http/Request.php | 33 +++++++++- tests/lib/AppFramework/Http/RequestTest.php | 70 +++++++++++++++++++++ 3 files changed, 110 insertions(+), 3 deletions(-) diff --git a/lib/base.php b/lib/base.php index d6c6e17eff..2f5517f461 100644 --- a/lib/base.php +++ b/lib/base.php @@ -493,10 +493,18 @@ class OC { '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: nc_sameSiteCookie%s=true; path=%s; httponly;' . $secureCookie . 'expires=Fri, 31-Dec-2100 23:59:59 GMT; SameSite=%s', + '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 diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index c7a3be163f..62d7fc7ed3 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -497,6 +497,31 @@ class Request implements \ArrayAccess, \Countable, IRequest { return true; } + /** + * Wrapper around session_get_cookie_params + * + * @return array + */ + protected function getCookieParams() { + return session_get_cookie_params(); + } + + /** + * Appends the __Host- prefix to the cookie if applicable + * + * @param string $name + * @return string + */ + protected function getProtectedCookieName($name) { + $cookieParams = $this->getCookieParams(); + $prefix = ''; + if($cookieParams['secure'] === true && $cookieParams['path'] === '/') { + $prefix = '__Host-'; + } + + return $prefix.$name; + } + /** * Checks if the strict cookie has been sent with the request if the request * is including any cookies. @@ -508,7 +533,9 @@ class Request implements \ArrayAccess, \Countable, IRequest { if(!$this->cookieCheckRequired()) { return true; } - if($this->getCookie('nc_sameSiteCookiestrict') === 'true' + + $cookieName = $this->getProtectedCookieName('nc_sameSiteCookiestrict'); + if($this->getCookie($cookieName) === 'true' && $this->passesLaxCookieCheck()) { return true; } @@ -526,7 +553,9 @@ class Request implements \ArrayAccess, \Countable, IRequest { if(!$this->cookieCheckRequired()) { return true; } - if($this->getCookie('nc_sameSiteCookielax') === 'true') { + + $cookieName = $this->getProtectedCookieName('nc_sameSiteCookielax'); + if($this->getCookie($cookieName) === 'true') { return true; } return false; diff --git a/tests/lib/AppFramework/Http/RequestTest.php b/tests/lib/AppFramework/Http/RequestTest.php index 1ba2086943..b1515b0efb 100644 --- a/tests/lib/AppFramework/Http/RequestTest.php +++ b/tests/lib/AppFramework/Http/RequestTest.php @@ -1500,6 +1500,76 @@ class RequestTest extends \Test\TestCase { $this->assertFalse($request->passesCSRFCheck()); } + public function testPassesStrictCookieCheckWithAllCookiesAndStrict() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName', 'getCookieParams']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + session_name() => 'asdf', + '__Host-nc_sameSiteCookiestrict' => 'true', + '__Host-nc_sameSiteCookielax' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $request + ->expects($this->any()) + ->method('getCookieParams') + ->willReturn([ + 'secure' => true, + 'path' => '/', + ]); + + $this->assertTrue($request->passesStrictCookieCheck()); + } + + public function testFailsStrictCookieCheckWithAllCookiesAndMissingStrict() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName', 'getCookieParams']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + session_name() => 'asdf', + 'nc_sameSiteCookiestrict' => 'true', + 'nc_sameSiteCookielax' => 'true', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $request + ->expects($this->any()) + ->method('getCookieParams') + ->willReturn([ + 'secure' => true, + 'path' => '/', + ]); + + $this->assertFalse($request->passesStrictCookieCheck()); + } + + public function testGetCookieParams() { + $request = $this->createMock(Request::class); + $actual = $this->invokePrivate($request, 'getCookieParams'); + $this->assertSame(session_get_cookie_params(), $actual); + } + public function testPassesStrictCookieCheckWithAllCookies() { /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request')