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 <[email protected]>

Author: artonge

apps/dav/appinfo/v2/publicremote.php

@@ -26,6 +26,7 @@
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\ITagManager;
+use OCP\IURLGenerator;
 use OCP\IUserSession;
 use OCP\L10N\IFactory;
 use OCP\Security\Bruteforce\IThrottler;
@@ -53,7 +54,8 @@
 	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);
 

apps/dav/lib/Connector/Sabre/PublicAuth.php

@@ -14,6 +14,7 @@
 use OCP\Defaults;
 use OCP\IRequest;
 use OCP\ISession;
+use OCP\IURLGenerator;
 use OCP\Security\Bruteforce\IThrottler;
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\IManager;
@@ -22,6 +23,7 @@
 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;
@@ -44,24 +46,25 @@ public function __construct(
 		private ISession $session,
 		private IThrottler $throttler,
 		private LoggerInterface $logger,
+		private IURLGenerator $urlGenerator,
 	) {
 		// setup realm
 		$defaults = new Defaults();
 		$this->realm = $defaults->getName();
 	}
 
 	/**
-	 * @param RequestInterface $request
-	 * @param ResponseInterface $response
-	 *
-	 * @return array
 	 * @throws NotAuthenticated
 	 * @throws ServiceUnavailable
 	 */
 	public function check(RequestInterface $request, ResponseInterface $response): array {
 		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,
@@ -77,6 +80,15 @@ public function check(RequestInterface $request, ResponseInterface $response): a
 			return $this->checkToken();
 		} catch (NotAuthenticated $e) {
 			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();
@@ -87,7 +99,6 @@ public function check(RequestInterface $request, ResponseInterface $response): a
 
 	/**
 	 * Extract token from request url
-	 * @return string
 	 * @throws NotFound
 	 */
 	private function getToken(): string {
@@ -104,7 +115,7 @@ private function getToken(): string {
 
 	/**
 	 * Check token validity
-	 * @return array
+	 *
 	 * @throws NotFound
 	 * @throws NotAuthenticated
 	 */
@@ -152,15 +163,13 @@ private function checkToken(): array {
 	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
@@ -203,7 +212,13 @@ protected function validateUserPass($username, $password) {
 	}
 
 	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;
 	}
 }

apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php

@@ -10,10 +10,12 @@
 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 @@
  */
 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;
-
-	/** @var string */
-	private $oldUser;
+	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;
+
+	private string $oldUser;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -49,13 +45,15 @@ protected function setUp(): void {
 		$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,
 			$this->shareManager,
 			$this->session,
 			$this->throttler,
 			$this->logger,
+			$this->urlGenerator,
 		);
 
 		// Store current user
@@ -137,7 +135,7 @@ public function testCheckTokenAlreadyAuthenticated(): void {
 
 		$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 @@ public function testCheckTokenPasswordNotAuthenticated(): void {
 			->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 @@ public function testCheckTokenPasswordAuthenticatedWrongShare(): void {
 
 		$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');
 	}

lib/base.php

@@ -548,10 +548,10 @@ private static function performSameSiteCookieProtection(\OCP\IConfig $config): v
 			$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;
 			}