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 <lukas@statuscode.ch>

diff --git a/lib/base.php b/lib/base.php
index d6c6e17eff9b88f86a4f560adb1005afeed002f8..2f5517f461444ba12c3cc0296137b873c5aaf75a 100644 (file)
--- 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 c7a3be163fe6d52b9f4101d101d5a7d1c4f49c8c..62d7fc7ed307a488e6420b320657b03dc6c5fb90 100644 (file)
--- 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 1ba208694398c817af5f37ea518b95a2a645c757..b1515b0efb5f5e703cc6731d03dc3e6cfc13771b 100644 (file)
--- 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')