From a05b8b79534fcd46341ae7bfd28cb34e9ff88ced Mon Sep 17 00:00:00 2001
From: Lukas Reschke <lukas@statuscode.ch>
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 <lukas@statuscode.ch>
---
 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 d6c6e17eff9..2f5517f4614 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 c7a3be163fe..62d7fc7ed30 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 1ba20869439..b1515b0efb5 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')
-- 
2.39.5