From 1914e7082a12056574f991d882089e308e31edb8 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 8 Sep 2016 17:14:32 +0200 Subject: [PATCH 1/2] Add exemptions for incompatible UAs Some user agents are notorious and don't really properly follow HTTP specifications. For those, have an automated opt-out. Since the protection for remote.php is applied in base.php as starting point we need to opt out here. --- lib/base.php | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/base.php b/lib/base.php index e148dd50ff861..5a6d072496e80 100644 --- a/lib/base.php +++ b/lib/base.php @@ -506,8 +506,22 @@ private static function sendSameSiteCookies() { * also we can't directly interfere with PHP's session mechanism. */ private static function performSameSiteCookieProtection() { + $request = \OC::$server->getRequest(); + + // Some user agents are notorious and don't really properly follow HTTP + // specifications. For those, have an automated opt-out. Since the protection + // for remote.php is applied in base.php as starting point we need to opt out + // here. + $incompatibleUserAgents = [ + // OS X Finder + '/^WebDAVFS/', + ]; + if($request->isUserAgent($incompatibleUserAgents)) { + return; + } + + if(count($_COOKIE) > 0) { - $request = \OC::$server->getRequest(); $requestUri = $request->getScriptName(); $processingScript = explode('/', $requestUri); $processingScript = $processingScript[count($processingScript)-1]; From 2ae08d6fc2704895eb6a7071359a54b06c8cefed Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 9 Aug 2016 19:09:53 +0200 Subject: [PATCH 2/2] Match only for actual session cookie OVH has implemented load balancing in a very questionable way where the reverse proxy actually internally adds some cookies which would trigger a security exception. To work around this, this change only checks for the session cookie. --- lib/private/AppFramework/Http/Request.php | 17 ++++- tests/lib/AppFramework/Http/RequestTest.php | 78 ++++++++++++++++++++- 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index 4a0c26a54f290..dae0ee50503f7 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -485,6 +485,19 @@ public function passesCSRFCheck() { return $this->csrfTokenManager->isTokenValid($token); } + /** + * Whether the cookie checks are required + * + * @return bool + */ + private function cookieCheckRequired() { + if($this->getCookie(session_name()) === null && $this->getCookie('oc_token') === null) { + return false; + } + + return true; + } + /** * Checks if the strict cookie has been sent with the request if the request * is including any cookies. @@ -493,7 +506,7 @@ public function passesCSRFCheck() { * @since 9.1.0 */ public function passesStrictCookieCheck() { - if(count($this->cookies) === 0) { + if(!$this->cookieCheckRequired()) { return true; } if($this->getCookie('nc_sameSiteCookiestrict') === 'true' @@ -511,7 +524,7 @@ public function passesStrictCookieCheck() { * @since 9.1.0 */ public function passesLaxCookieCheck() { - if(count($this->cookies) === 0) { + if(!$this->cookieCheckRequired()) { return true; } if($this->getCookie('nc_sameSiteCookielax') === 'true') { diff --git a/tests/lib/AppFramework/Http/RequestTest.php b/tests/lib/AppFramework/Http/RequestTest.php index a3433e558d85d..420b73a22d9dd 100644 --- a/tests/lib/AppFramework/Http/RequestTest.php +++ b/tests/lib/AppFramework/Http/RequestTest.php @@ -1469,6 +1469,7 @@ public function testFailsCSRFCheckWithHeaderAndNotAllChecksPassing() { 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], 'cookies' => [ + session_name() => 'asdf', 'nc_sameSiteCookiestrict' => 'true', ], ], @@ -1495,6 +1496,7 @@ public function testPassesStrictCookieCheckWithAllCookies() { 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], 'cookies' => [ + session_name() => 'asdf', 'nc_sameSiteCookiestrict' => 'true', 'nc_sameSiteCookielax' => 'true', ], @@ -1509,7 +1511,76 @@ public function testPassesStrictCookieCheckWithAllCookies() { $this->assertTrue($request->passesStrictCookieCheck()); } - public function testFailsSRFCheckWithPostAndWithCookies() { + public function testPassesStrictCookieCheckWithRandomCookies() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'RandomCookie' => 'asdf', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertTrue($request->passesStrictCookieCheck()); + } + + public function testFailsStrictCookieCheckWithSessionCookie() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + session_name() => 'asdf', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertFalse($request->passesStrictCookieCheck()); + } + + public function testFailsStrictCookieCheckWithRememberMeCookie() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'server' => [ + 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'oc_token' => 'asdf', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + + $this->assertFalse($request->passesStrictCookieCheck()); + } + + public function testFailsCSRFCheckWithPostAndWithCookies() { /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) @@ -1519,6 +1590,7 @@ public function testFailsSRFCheckWithPostAndWithCookies() { 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], 'cookies' => [ + session_name() => 'asdf', 'foo' => 'bar', ], ], @@ -1545,6 +1617,7 @@ public function testFailStrictCookieCheckWithOnlyLaxCookie() { 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], 'cookies' => [ + session_name() => 'asdf', 'nc_sameSiteCookielax' => 'true', ], ], @@ -1568,6 +1641,7 @@ public function testFailStrictCookieCheckWithOnlyStrictCookie() { 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], 'cookies' => [ + session_name() => 'asdf', 'nc_sameSiteCookiestrict' => 'true', ], ], @@ -1591,6 +1665,7 @@ public function testPassesLaxCookieCheck() { 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], 'cookies' => [ + session_name() => 'asdf', 'nc_sameSiteCookielax' => 'true', ], ], @@ -1614,6 +1689,7 @@ public function testFailsLaxCookieCheckWithOnlyStrictCookie() { 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', ], 'cookies' => [ + session_name() => 'asdf', 'nc_sameSiteCookiestrict' => 'true', ], ],