From 009d0c550ca4d23d8670528c4117293da34c51b9 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 14 May 2025 11:36:09 +0200 Subject: [PATCH] fix: Move CSRF check from base to PublicAuth for public.php This currently prevent directly accessing a ressource when clicking on a link on a third party site. Example, clicking on `https://example.com/public.php/dav/files/pqLWcA269zfzXez/?accept=zip` in a GitHub comment. Skipping the check is an issue with password protected shares, as it allows third party sites to request the ressource when the user already entered the password, aka CSRF. So after removing the check from `base.php`, we need to add the it again in the `PublicAuth` plugin. We also add a redirect to be helpful to the user. **Warning**: this adds the limitation that clicking on a direct download link for password protected shares will redirect you to the password form, and then to the main share view. Fix #52482 Signed-off-by: Louis Chemineau --- apps/dav/appinfo/v2/publicremote.php | 4 ++- apps/dav/lib/Connector/Sabre/PublicAuth.php | 35 +++++++++++++------ .../unit/Connector/Sabre/PublicAuthTest.php | 32 ++++++++--------- lib/base.php | 8 ++--- 4 files changed, 47 insertions(+), 32 deletions(-) diff --git a/apps/dav/appinfo/v2/publicremote.php b/apps/dav/appinfo/v2/publicremote.php index fbb3ddd2cb3..e089fa7bb62 100644 --- a/apps/dav/appinfo/v2/publicremote.php +++ b/apps/dav/appinfo/v2/publicremote.php @@ -29,6 +29,7 @@ use OCP\IPreview; use OCP\IRequest; use OCP\ISession; use OCP\ITagManager; +use OCP\IURLGenerator; use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Security\Bruteforce\IThrottler; @@ -56,7 +57,8 @@ $authBackend = new PublicAuth( Server::get(IManager::class), $session, Server::get(IThrottler::class), - Server::get(LoggerInterface::class) + Server::get(LoggerInterface::class), + Server::get(IURLGenerator::class), ); $authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend); diff --git a/apps/dav/lib/Connector/Sabre/PublicAuth.php b/apps/dav/lib/Connector/Sabre/PublicAuth.php index b5d9ce3db72..2ca1c25e2f6 100644 --- a/apps/dav/lib/Connector/Sabre/PublicAuth.php +++ b/apps/dav/lib/Connector/Sabre/PublicAuth.php @@ -14,6 +14,7 @@ namespace OCA\DAV\Connector\Sabre; use OCP\Defaults; use OCP\IRequest; use OCP\ISession; +use OCP\IURLGenerator; use OCP\Security\Bruteforce\IThrottler; use OCP\Security\Bruteforce\MaxDelayReached; use OCP\Share\Exceptions\ShareNotFound; @@ -23,6 +24,7 @@ use Psr\Log\LoggerInterface; use Sabre\DAV\Auth\Backend\AbstractBasic; use Sabre\DAV\Exception\NotAuthenticated; use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\Exception\PreconditionFailed; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\HTTP; use Sabre\HTTP\RequestInterface; @@ -45,6 +47,7 @@ class PublicAuth extends AbstractBasic { private ISession $session, private IThrottler $throttler, private LoggerInterface $logger, + private IURLGenerator $urlGenerator, ) { // setup realm $defaults = new Defaults(); @@ -52,10 +55,6 @@ class PublicAuth extends AbstractBasic { } /** - * @param RequestInterface $request - * @param ResponseInterface $response - * - * @return array * @throws NotAuthenticated * @throws MaxDelayReached * @throws ServiceUnavailable @@ -64,6 +63,10 @@ class PublicAuth extends AbstractBasic { try { $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION); + if (count($_COOKIE) > 0 && !$this->request->passesStrictCookieCheck() && $this->getShare()->getPassword() !== null) { + throw new PreconditionFailed('Strict cookie check failed'); + } + $auth = new HTTP\Auth\Basic( $this->realm, $request, @@ -80,6 +83,15 @@ class PublicAuth extends AbstractBasic { } catch (NotAuthenticated|MaxDelayReached $e) { $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); throw $e; + } catch (PreconditionFailed $e) { + $response->setHeader( + 'Location', + $this->urlGenerator->linkToRoute( + 'files_sharing.share.showShare', + [ 'token' => $this->getToken() ], + ), + ); + throw $e; } catch (\Exception $e) { $class = get_class($e); $msg = $e->getMessage(); @@ -90,7 +102,6 @@ class PublicAuth extends AbstractBasic { /** * Extract token from request url - * @return string * @throws NotFound */ private function getToken(): string { @@ -107,7 +118,7 @@ class PublicAuth extends AbstractBasic { /** * Check token validity - * @return array + * * @throws NotFound * @throws NotAuthenticated */ @@ -155,15 +166,13 @@ class PublicAuth extends AbstractBasic { protected function validateUserPass($username, $password) { $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION); - $token = $this->getToken(); try { - $share = $this->shareManager->getShareByToken($token); + $share = $this->getShare(); } catch (ShareNotFound $e) { $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); return false; } - $this->share = $share; \OC_User::setIncognitoMode(true); // check if the share is password protected @@ -206,7 +215,13 @@ class PublicAuth extends AbstractBasic { } public function getShare(): IShare { - assert($this->share !== null); + $token = $this->getToken(); + + if ($this->share === null) { + $share = $this->shareManager->getShareByToken($token); + $this->share = $share; + } + return $this->share; } } diff --git a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php index 00bddf2e69c..67e7f6af675 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php @@ -10,10 +10,12 @@ namespace OCA\DAV\Tests\unit\Connector; use OCA\DAV\Connector\Sabre\PublicAuth; use OCP\IRequest; use OCP\ISession; +use OCP\IURLGenerator; use OCP\Security\Bruteforce\IThrottler; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IShare; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; /** @@ -25,21 +27,15 @@ use Psr\Log\LoggerInterface; */ class PublicAuthTest extends \Test\TestCase { - /** @var ISession|MockObject */ - private $session; - /** @var IRequest|MockObject */ - private $request; - /** @var IManager|MockObject */ - private $shareManager; - /** @var PublicAuth */ - private $auth; - /** @var IThrottler|MockObject */ - private $throttler; - /** @var LoggerInterface|MockObject */ - private $logger; + private ISession&MockObject $session; + private IRequest&MockObject $request; + private IManager&MockObject $shareManager; + private PublicAuth $auth; + private IThrottler&MockObject $throttler; + private LoggerInterface&MockObject $logger; + private IURLGenerator&MockObject $urlGenerator; - /** @var string */ - private $oldUser; + private string $oldUser; protected function setUp(): void { parent::setUp(); @@ -49,6 +45,7 @@ class PublicAuthTest extends \Test\TestCase { $this->shareManager = $this->createMock(IManager::class); $this->throttler = $this->createMock(IThrottler::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->auth = new PublicAuth( $this->request, @@ -56,6 +53,7 @@ class PublicAuthTest extends \Test\TestCase { $this->session, $this->throttler, $this->logger, + $this->urlGenerator, ); // Store current user @@ -137,7 +135,7 @@ class PublicAuthTest extends \Test\TestCase { $this->session->method('exists')->with('public_link_authenticated')->willReturn(true); $this->session->method('get')->with('public_link_authenticated')->willReturn('42'); - + $result = $this->invokePrivate($this->auth, 'checkToken'); $this->assertSame([true, 'principals/GX9HSGQrGE'], $result); } @@ -158,7 +156,7 @@ class PublicAuthTest extends \Test\TestCase { ->willReturn($share); $this->session->method('exists')->with('public_link_authenticated')->willReturn(false); - + $this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class); $this->invokePrivate($this->auth, 'checkToken'); } @@ -180,7 +178,7 @@ class PublicAuthTest extends \Test\TestCase { $this->session->method('exists')->with('public_link_authenticated')->willReturn(false); $this->session->method('get')->with('public_link_authenticated')->willReturn('43'); - + $this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class); $this->invokePrivate($this->auth, 'checkToken'); } diff --git a/lib/base.php b/lib/base.php index 45058db1600..2aa833df3b5 100644 --- a/lib/base.php +++ b/lib/base.php @@ -550,10 +550,10 @@ class OC { $processingScript = explode('/', $requestUri); $processingScript = $processingScript[count($processingScript) - 1]; - // index.php routes are handled in the middleware - // and cron.php does not need any authentication at all - if ($processingScript === 'index.php' - || $processingScript === 'cron.php') { + if ($processingScript === 'index.php' // index.php routes are handled in the middleware + || $processingScript === 'cron.php' // and cron.php does not need any authentication at all + || $processingScript === 'public.php' // For public.php, auth for password protected shares is done in the PublicAuth plugin + ) { return; }