From 202530f4f3e984a8fb4302f2a6863d426671e9ad Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 15 Jun 2016 11:50:26 +0200 Subject: [PATCH] Soften the cookie check if no cookies are sent When no cookies are sent it is not required to perform any check for the strict or lax cookie, it does not provide any significant security advantage. It does however interfer with the Android client which requests thumbnails from the unofficial API at `/index.php/apps/files/api/v1/thumbnail/256/256/{filename}`. This endpoint expects the strict cookie to be existent to not leak the existence of files. The Android client authenticates against this endpoint using Basic Auth and without cookies in some cases at least. This will make these endpoints work again with such cases. To test this issue the following cURL command once without the patch and once with: > curl http://localhost/index.php/apps/files/api/v1/thumbnail/256/256/welcome.txt -u admin -v Without the patch the request is redirected (which the client does not obey) and with the patch the preview is returned. --- lib/private/appframework/http/request.php | 13 ++++- lib/public/irequest.php | 16 ++++--- tests/lib/appframework/http/RequestTest.php | 53 ++++++++++++++++----- 3 files changed, 61 insertions(+), 21 deletions(-) diff --git a/lib/private/appframework/http/request.php b/lib/private/appframework/http/request.php index 3e1dd45c45..1ab52fb3ca 100644 --- a/lib/private/appframework/http/request.php +++ b/lib/private/appframework/http/request.php @@ -468,12 +468,16 @@ class Request implements \ArrayAccess, \Countable, IRequest { } /** - * Checks if the strict cookie has been sent with the request + * Checks if the strict cookie has been sent with the request if the request + * is including any cookies. * * @return bool * @since 9.1.0 */ public function passesStrictCookieCheck() { + if(count($this->cookies) === 0) { + return true; + } if($this->getCookie('nc_sameSiteCookiestrict') === 'true' && $this->passesLaxCookieCheck()) { return true; @@ -483,12 +487,17 @@ class Request implements \ArrayAccess, \Countable, IRequest { } /** - * Checks if the lax cookie has been sent with the request + * Checks if the lax cookie has been sent with the request if the request + * is including any cookies. * * @return bool * @since 9.1.0 */ public function passesLaxCookieCheck() { + if(count($this->cookies) === 0) { + return true; + } + if($this->getCookie('nc_sameSiteCookielax') === 'true') { return true; } diff --git a/lib/public/irequest.php b/lib/public/irequest.php index 6a8ee0c5df..ad207fe934 100644 --- a/lib/public/irequest.php +++ b/lib/public/irequest.php @@ -144,18 +144,20 @@ interface IRequest { public function passesCSRFCheck(); /** - * Checks if the strict cookie has been sent with the request - * - * @return bool - * @since 9.0.0 - */ + * Checks if the strict cookie has been sent with the request if the request + * is including any cookies. + * + * @return bool + * @since 9.0.0 + */ public function passesStrictCookieCheck(); /** - * Checks if the lax cookie has been sent with the request + * Checks if the lax cookie has been sent with the request if the request + * is including any cookies. * * @return bool - * @since 9.1.0 + * @since 9.0.0 */ public function passesLaxCookieCheck(); diff --git a/tests/lib/appframework/http/RequestTest.php b/tests/lib/appframework/http/RequestTest.php index 787f410e0c..335a7e9c8e 100644 --- a/tests/lib/appframework/http/RequestTest.php +++ b/tests/lib/appframework/http/RequestTest.php @@ -1404,7 +1404,7 @@ class RequestTest extends \Test\TestCase { $this->assertTrue($request->passesCSRFCheck()); } - public function testFailsCSRFCheckWithGetAndWithoutCookies() { + public function testPassesCSRFCheckWithGetAndWithoutCookies() { /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) @@ -1421,13 +1421,14 @@ class RequestTest extends \Test\TestCase { ]) ->getMock(); $this->csrfTokenManager - ->expects($this->never()) - ->method('isTokenValid'); + ->expects($this->once()) + ->method('isTokenValid') + ->willReturn(true); - $this->assertFalse($request->passesCSRFCheck()); + $this->assertTrue($request->passesCSRFCheck()); } - public function testFailsCSRFCheckWithPostAndWithoutCookies() { + public function testPassesCSRFCheckWithPostAndWithoutCookies() { /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) @@ -1444,13 +1445,14 @@ class RequestTest extends \Test\TestCase { ]) ->getMock(); $this->csrfTokenManager - ->expects($this->never()) - ->method('isTokenValid'); + ->expects($this->once()) + ->method('isTokenValid') + ->willReturn(true); - $this->assertFalse($request->passesCSRFCheck()); + $this->assertTrue($request->passesCSRFCheck()); } - public function testFailsCSRFCheckWithHeaderAndWithoutCookies() { + public function testPassesCSRFCheckWithHeaderAndWithoutCookies() { /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') ->setMethods(['getScriptName']) @@ -1467,10 +1469,11 @@ class RequestTest extends \Test\TestCase { ]) ->getMock(); $this->csrfTokenManager - ->expects($this->never()) - ->method('isTokenValid'); + ->expects($this->once()) + ->method('isTokenValid') + ->willReturn(true); - $this->assertFalse($request->passesCSRFCheck()); + $this->assertTrue($request->passesCSRFCheck()); } public function testFailsCSRFCheckWithHeaderAndNotAllChecksPassing() { @@ -1523,6 +1526,32 @@ class RequestTest extends \Test\TestCase { $this->assertTrue($request->passesStrictCookieCheck()); } + public function testFailsSRFCheckWithPostAndWithCookies() { + /** @var Request $request */ + $request = $this->getMockBuilder('\OC\AppFramework\Http\Request') + ->setMethods(['getScriptName']) + ->setConstructorArgs([ + [ + 'post' => [ + 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', + ], + 'cookies' => [ + 'foo' => 'bar', + ], + ], + $this->secureRandom, + $this->config, + $this->csrfTokenManager, + $this->stream + ]) + ->getMock(); + $this->csrfTokenManager + ->expects($this->never()) + ->method('isTokenValid'); + + $this->assertFalse($request->passesCSRFCheck()); + } + public function testFailStrictCookieCheckWithOnlyLaxCookie() { /** @var Request $request */ $request = $this->getMockBuilder('\OC\AppFramework\Http\Request')