diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index 24a81e9af9400..a5f2061863fe2 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -43,6 +43,7 @@ \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), \OC::$server->getBruteForceThrottler(), + \OC::$server->get(\OC\Security\CSRF\CsrfValidator::class), 'principals/' ); $principalBackend = new Principal( diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index e7faa9314e2a6..586ff57ef90da 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -48,6 +48,7 @@ \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), \OC::$server->getBruteForceThrottler(), + \OC::$server->get(\OC\Security\CSRF\CsrfValidator::class), 'principals/' ); $principalBackend = new Principal( diff --git a/apps/dav/appinfo/v1/webdav.php b/apps/dav/appinfo/v1/webdav.php index 8dbe846f3ffbd..b982ab9548dad 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -59,6 +59,7 @@ \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), \OC::$server->getBruteForceThrottler(), + \OC::$server->get(\OC\Security\CSRF\CsrfValidator::class), 'principals/' ); $authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend); diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php index 69e3946bb1152..28de6efc85730 100644 --- a/apps/dav/lib/Connector/Sabre/Auth.php +++ b/apps/dav/lib/Connector/Sabre/Auth.php @@ -37,6 +37,7 @@ use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OC\Authentication\TwoFactorAuth\Manager; use OC\Security\Bruteforce\Throttler; +use OC\Security\CSRF\CsrfValidator; use OC\User\LoginException; use OC\User\Session; use OCA\DAV\Connector\Sabre\Exception\PasswordLoginForbidden; @@ -61,17 +62,21 @@ class Auth extends AbstractBasic { private Manager $twoFactorManager; private Throttler $throttler; + private CsrfValidator $csrfValidator; + public function __construct(ISession $session, Session $userSession, IRequest $request, Manager $twoFactorManager, Throttler $throttler, + CsrfValidator $csrfValidator, string $principalPrefix = 'principals/users/') { $this->session = $session; $this->userSession = $userSession; $this->twoFactorManager = $twoFactorManager; $this->request = $request; $this->throttler = $throttler; + $this->csrfValidator = $csrfValidator; $this->principalPrefix = $principalPrefix; // setup realm @@ -191,7 +196,7 @@ private function requiresCSRFCheck(): bool { private function auth(RequestInterface $request, ResponseInterface $response): array { $forcedLogout = false; - if (!$this->request->passesCSRFCheck() && + if (!$this->csrfValidator->validate($this->request) && $this->requiresCSRFCheck()) { // In case of a fail with POST we need to recheck the credentials if ($this->request->getMethod() === 'POST') { diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 909bcaa71e8f9..d412d2db034f3 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -35,6 +35,8 @@ */ namespace OCA\DAV; +use OC\Security\Bruteforce\Throttler; +use OC\Security\CSRF\CsrfValidator; use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\BulkUpload\BulkUploadPlugin; use OCA\DAV\CalDAV\BirthdayService; @@ -120,7 +122,8 @@ public function __construct(IRequest $request, string $baseUri) { \OC::$server->getUserSession(), \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), - \OC::$server->getBruteForceThrottler() + \OC::$server->get(Throttler::class), + \OC::$server->get(CsrfValidator::class) ); // Set URL explicitly due to reverse-proxy situations diff --git a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php index b3b3341240acd..59453acf69c79 100644 --- a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php @@ -31,6 +31,7 @@ use OC\Authentication\TwoFactorAuth\Manager; use OC\Security\Bruteforce\Throttler; +use OC\Security\CSRF\CsrfValidator; use OC\User\Session; use OCP\IRequest; use OCP\ISession; @@ -59,6 +60,7 @@ class AuthTest extends TestCase { private $twoFactorManager; /** @var Throttler */ private $throttler; + private CsrfValidator $csrfValidator; protected function setUp(): void { parent::setUp(); @@ -74,12 +76,14 @@ protected function setUp(): void { $this->throttler = $this->getMockBuilder(Throttler::class) ->disableOriginalConstructor() ->getMock(); + $this->csrfValidator = $this->createMock(CsrfValidator::class); $this->auth = new \OCA\DAV\Connector\Sabre\Auth( $this->session, $this->userSession, $this->request, $this->twoFactorManager, - $this->throttler + $this->throttler, + $this->csrfValidator, ); } @@ -270,9 +274,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGet(): void ->expects($this->any()) ->method('getUser') ->willReturn($user); - $this->request + $this->csrfValidator ->expects($this->once()) - ->method('passesCSRFCheck') + ->method('validate') ->willReturn(false); $expectedResponse = [ @@ -317,9 +321,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndCorrectlyDavAu ->expects($this->any()) ->method('getUser') ->willReturn($user); - $this->request + $this->csrfValidator ->expects($this->once()) - ->method('passesCSRFCheck') + ->method('validate') ->willReturn(false); $this->auth->check($request, $response); } @@ -362,9 +366,9 @@ public function testAuthenticateAlreadyLoggedInWithoutTwoFactorChallengePassed() ->expects($this->any()) ->method('getUser') ->willReturn($user); - $this->request + $this->csrfValidator ->expects($this->once()) - ->method('passesCSRFCheck') + ->method('validate') ->willReturn(true); $this->twoFactorManager->expects($this->once()) ->method('needsSecondFactor') @@ -411,9 +415,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndIncorrectlyDav ->expects($this->any()) ->method('getUser') ->willReturn($user); - $this->request + $this->csrfValidator ->expects($this->once()) - ->method('passesCSRFCheck') + ->method('validate') ->willReturn(false); $this->auth->check($request, $response); } @@ -452,9 +456,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGetAndDeskt ->expects($this->any()) ->method('getUser') ->willReturn($user); - $this->request + $this->csrfValidator ->expects($this->once()) - ->method('passesCSRFCheck') + ->method('validate') ->willReturn(false); $this->auth->check($request, $response); @@ -521,9 +525,9 @@ public function testAuthenticateAlreadyLoggedInWithCsrfTokenForGet(): void { ->expects($this->any()) ->method('getUser') ->willReturn($user); - $this->request + $this->csrfValidator ->expects($this->once()) - ->method('passesCSRFCheck') + ->method('validate') ->willReturn(true); $response = $this->auth->check($request, $response); diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 5f94c8f8a32aa..d36e489687dd6 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -39,6 +39,7 @@ use OC\Authentication\Login\LoginData; use OC\Authentication\WebAuthn\Manager as WebAuthnManager; use OC\Security\Bruteforce\Throttler; +use OC\Security\CSRF\CsrfValidator; use OC\User\Session; use OC_App; use OCP\AppFramework\Controller; @@ -79,6 +80,7 @@ public function __construct( private WebAuthnManager $webAuthnManager, private IManager $manager, private IL10N $l10n, + private CsrfValidator $csrfValidator, ) { parent::__construct($appName, $request); } @@ -276,7 +278,7 @@ public function tryLogin(Chain $loginChain, string $redirect_url = null, string $timezone = '', string $timezone_offset = ''): RedirectResponse { - if (!$this->request->passesCSRFCheck()) { + if (!$this->csrfValidator->validate($this->request)) { if ($this->userSession->isLoggedIn()) { // If the user is already logged in and the CSRF check does not pass then // simply redirect the user to the correct page as required. This is the diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 25c402b308497..ababc5525b1f3 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1564,6 +1564,7 @@ 'OC\\Security\\CSRF\\CsrfToken' => $baseDir . '/lib/private/Security/CSRF/CsrfToken.php', 'OC\\Security\\CSRF\\CsrfTokenGenerator' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenGenerator.php', 'OC\\Security\\CSRF\\CsrfTokenManager' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenManager.php', + 'OC\\Security\\CSRF\\CsrfValidator' => $baseDir . '/lib/private/Security/CSRF/CsrfValidator.php', 'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => $baseDir . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php', 'OC\\Security\\Certificate' => $baseDir . '/lib/private/Security/Certificate.php', 'OC\\Security\\CertificateManager' => $baseDir . '/lib/private/Security/CertificateManager.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index cb1ae9e537b3a..e6733053c19a6 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1597,6 +1597,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Security\\CSRF\\CsrfToken' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfToken.php', 'OC\\Security\\CSRF\\CsrfTokenGenerator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenGenerator.php', 'OC\\Security\\CSRF\\CsrfTokenManager' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenManager.php', + 'OC\\Security\\CSRF\\CsrfValidator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfValidator.php', 'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php', 'OC\\Security\\Certificate' => __DIR__ . '/../../..' . '/lib/private/Security/Certificate.php', 'OC\\Security\\CertificateManager' => __DIR__ . '/../../..' . '/lib/private/Security/CertificateManager.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 9a9740b7bccc4..90d4849fb2ba9 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -233,7 +233,8 @@ public function __construct(string $appName, array $urlParams = [], ServerContai $c->get(IRequest::class), $c->get(IControllerMethodReflector::class), $c->get(IUserSession::class), - $c->get(OC\Security\Bruteforce\Throttler::class) + $c->get(OC\Security\Bruteforce\Throttler::class), + $c->get(OC\Security\CSRF\CsrfValidator::class), ) ); $dispatcher->registerMiddleware( @@ -257,7 +258,8 @@ public function __construct(string $appName, array $urlParams = [], ServerContai $server->getAppManager(), $server->getL10N('lib'), $c->get(AuthorizedGroupMapper::class), - $server->get(IUserSession::class) + $server->get(IUserSession::class), + $c->get(OC\Security\CSRF\CsrfValidator::class), ); $dispatcher->registerMiddleware($securityMiddleware); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index e177a612d96c3..27ff794b24c8f 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -30,6 +30,7 @@ use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OC\Security\Bruteforce\Throttler; +use OC\Security\CSRF\CsrfValidator; use OC\User\Session; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; @@ -56,6 +57,7 @@ class CORSMiddleware extends Middleware { private $session; /** @var Throttler */ private $throttler; + private CsrfValidator $csrfValidator; /** * @param IRequest $request @@ -66,11 +68,13 @@ class CORSMiddleware extends Middleware { public function __construct(IRequest $request, ControllerMethodReflector $reflector, Session $session, - Throttler $throttler) { + Throttler $throttler, + CsrfValidator $csrfValidator) { $this->request = $request; $this->reflector = $reflector; $this->session = $session; $this->throttler = $throttler; + $this->csrfValidator = $csrfValidator; } /** @@ -94,7 +98,7 @@ public function beforeController($controller, $methodName) { $pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null; // Allow to use the current session if a CSRF token is provided - if ($this->request->passesCSRFCheck()) { + if ($this->csrfValidator->validate($this->request)) { return; } $this->session->logout(); diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 04f79361bc821..3718b4d326a9a 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -44,6 +44,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Security\CSRF\CsrfValidator; use OC\Settings\AuthorizedGroupMapper; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; @@ -102,6 +103,7 @@ class SecurityMiddleware extends Middleware { private $groupAuthorizationMapper; /** @var IUserSession */ private $userSession; + private CsrfValidator $csrfValidator; public function __construct(IRequest $request, ControllerMethodReflector $reflector, @@ -115,7 +117,8 @@ public function __construct(IRequest $request, IAppManager $appManager, IL10N $l10n, AuthorizedGroupMapper $mapper, - IUserSession $userSession + IUserSession $userSession, + CsrfValidator $csrfValidator, ) { $this->navigationManager = $navigationManager; $this->request = $request; @@ -130,6 +133,7 @@ public function __construct(IRequest $request, $this->l10n = $l10n; $this->groupAuthorizationMapper = $mapper; $this->userSession = $userSession; + $this->csrfValidator = $csrfValidator; } /** @@ -215,7 +219,7 @@ public function beforeController($controller, $methodName) { * Additionally we allow Bearer authenticated requests to pass on OCS routes. * This allows oauth apps (e.g. moodle) to use the OCS endpoints */ - if (!$this->request->passesCSRFCheck() && !( + if (!$this->csrfValidator->validate($this->request) && !( $controller instanceof OCSController && ( $this->request->getHeader('OCS-APIREQUEST') === 'true' || str_starts_with($this->request->getHeader('Authorization'), 'Bearer ') diff --git a/lib/private/EventSourceFactory.php b/lib/private/EventSourceFactory.php index 197c8bf9e6ca7..d3cb4170b6d9a 100644 --- a/lib/private/EventSourceFactory.php +++ b/lib/private/EventSourceFactory.php @@ -22,16 +22,15 @@ namespace OC; +use OC\Security\CSRF\CsrfValidator; use OCP\IEventSource; use OCP\IEventSourceFactory; use OCP\IRequest; class EventSourceFactory implements IEventSourceFactory { - private IRequest $request; - - - public function __construct(IRequest $request) { - $this->request = $request; + public function __construct( + private IRequest $request, + private CsrfValidator $csrfValidator) { } /** @@ -41,6 +40,9 @@ public function __construct(IRequest $request) { * @since 28.0.0 */ public function create(): IEventSource { - return new \OC_EventSource($this->request); + return new \OC_EventSource( + $this->request, + $this->csrfValidator + ); } } diff --git a/lib/private/Security/CSRF/CsrfValidator.php b/lib/private/Security/CSRF/CsrfValidator.php new file mode 100644 index 0000000000000..3b3dae3ce19cd --- /dev/null +++ b/lib/private/Security/CSRF/CsrfValidator.php @@ -0,0 +1,52 @@ + + * + * @author Daniel Kesselberg + * + * @license GNU AGPL version 3 or any later version + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Security\CSRF; + +use OCP\IRequest; + +class CsrfValidator { + public function __construct( + private CsrfTokenManager $csrfTokenManager) { + } + + public function validate(IRequest $request): bool { + if (!$request->passesStrictCookieCheck()) { + return false; + } + + $token = $request->getParam('requesttoken', ''); + if ($token === '') { + $token = $request->getHeader('REQUESTTOKEN'); + } + if ($token === '') { + return false; + } + + $token = new CsrfToken($token); + + return $this->csrfTokenManager->isTokenValid($token); + } +} diff --git a/lib/private/legacy/OC_EventSource.php b/lib/private/legacy/OC_EventSource.php index cd72ba1f2d558..90ab9c069c0dd 100644 --- a/lib/private/legacy/OC_EventSource.php +++ b/lib/private/legacy/OC_EventSource.php @@ -1,5 +1,6 @@ request = $request; + $this->csrfValidator = $csrfValidator; } protected function init() { @@ -84,7 +87,7 @@ protected function init() { header('Location: '.\OC::$WEBROOT); exit(); } - if (!$this->request->passesCSRFCheck()) { + if (!$this->csrfValidator->validate($this->request)) { $this->send('error', 'Possible CSRF attack. Connection will be closed.'); $this->close(); exit(); diff --git a/lib/private/legacy/OC_JSON.php b/lib/private/legacy/OC_JSON.php index b9cfb8210e09f..3a4999b7c9917 100644 --- a/lib/private/legacy/OC_JSON.php +++ b/lib/private/legacy/OC_JSON.php @@ -1,4 +1,8 @@ getRequest()->passesStrictCookieCheck()) { + $request = OC::$server->get(IRequest::class); + + if (!$request->passesStrictCookieCheck()) { header('Location: '.\OC::$WEBROOT); exit(); } - if (!\OC::$server->getRequest()->passesCSRFCheck()) { + $csrfValidator = OC::$server->get(CsrfValidator::class); + + if (!$csrfValidator->validate($request)) { $l = \OC::$server->getL10N('lib'); self::error([ 'data' => [ 'message' => $l->t('Token expired. Please reload page.'), 'error' => 'token_expired' ]]); exit(); diff --git a/lib/public/IRequest.php b/lib/public/IRequest.php index bc1f88504a834..4e6dc158563c4 100644 --- a/lib/public/IRequest.php +++ b/lib/public/IRequest.php @@ -201,6 +201,7 @@ public function getCookie(string $key); * * @return bool true if CSRF check passed * @since 6.0.0 + * @deprecated 28.0.0 use CsrfValidator.validate instead */ public function passesCSRFCheck(): bool; diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 6044440bdaf0e..159e2af41fa10 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -30,6 +30,9 @@ use OC\Authentication\TwoFactorAuth\Manager; use OC\Core\Controller\LoginController; use OC\Security\Bruteforce\Throttler; +use OC\Security\CSRF\CsrfToken; +use OC\Security\CSRF\CsrfTokenManager; +use OC\Security\CSRF\CsrfValidator; use OC\User\Session; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; @@ -89,6 +92,9 @@ class LoginControllerTest extends TestCase { /** @var IL10N|MockObject */ private $l; + private CsrfTokenManager $csrfTokenManager; + private CsrfValidator $csrfValidator; + protected function setUp(): void { parent::setUp(); $this->request = $this->createMock(IRequest::class); @@ -109,6 +115,8 @@ protected function setUp(): void { ->willReturnCallback(function ($text, $parameters = []) { return vsprintf($text, $parameters); }); + $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); + $this->csrfValidator = new CsrfValidator($this->csrfTokenManager); $this->request->method('getRemoteAddress') @@ -132,7 +140,8 @@ protected function setUp(): void { $this->initialStateService, $this->webAuthnManager, $this->notificationManager, - $this->l + $this->l, + $this->csrfValidator, ); } @@ -424,9 +433,16 @@ public function testLoginWithInvalidCredentials(): void { $password = 'secret'; $loginPageUrl = '/login?redirect_url=/apps/files'; $loginChain = $this->createMock(LoginChain::class); - $this->request - ->expects($this->once()) - ->method('passesCSRFCheck') + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) ->willReturn(true); $loginData = new LoginData( $this->request, @@ -459,9 +475,16 @@ public function testLoginWithValidCredentials() { $user = 'MyUserName'; $password = 'secret'; $loginChain = $this->createMock(LoginChain::class); - $this->request - ->expects($this->once()) - ->method('passesCSRFCheck') + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) ->willReturn(true); $loginData = new LoginData( $this->request, @@ -491,9 +514,16 @@ public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn(): void { $password = 'secret'; $originalUrl = 'another%20url'; $loginChain = $this->createMock(LoginChain::class); - $this->request - ->expects($this->once()) - ->method('passesCSRFCheck') + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) ->willReturn(false); $this->userSession ->method('isLoggedIn') @@ -521,9 +551,16 @@ public function testLoginWithoutPassedCsrfCheckAndLoggedIn() { $originalUrl = 'another url'; $redirectUrl = 'http://localhost/another url'; $loginChain = $this->createMock(LoginChain::class); - $this->request - ->expects($this->once()) - ->method('passesCSRFCheck') + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) ->willReturn(false); $this->userSession ->method('isLoggedIn') @@ -553,9 +590,16 @@ public function testLoginWithValidCredentialsAndRedirectUrl() { $password = 'secret'; $redirectUrl = 'https://next.cloud/apps/mail'; $loginChain = $this->createMock(LoginChain::class); - $this->request - ->expects($this->once()) - ->method('passesCSRFCheck') + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) ->willReturn(true); $loginData = new LoginData( $this->request, @@ -584,9 +628,16 @@ public function testLoginWithValidCredentialsAndRedirectUrl() { public function testToNotLeakLoginName() { $loginChain = $this->createMock(LoginChain::class); - $this->request - ->expects($this->once()) - ->method('passesCSRFCheck') + $this->request->expects($this->once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) ->willReturn(true); $loginPageUrl = '/login?redirect_url=/apps/files'; $loginData = new LoginData( diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php index 7c48f7e2712b6..36c1d7d442dda 100644 --- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php @@ -16,6 +16,8 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\Bruteforce\Throttler; +use OC\Security\CSRF\CsrfTokenManager; +use OC\Security\CSRF\CsrfValidator; use OC\User\Session; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\Response; @@ -31,6 +33,8 @@ class CORSMiddlewareTest extends \Test\TestCase { private $session; /** @var Throttler */ private $throttler; + private CsrfTokenManager $csrfTokenManager; + private CsrfValidator $csrfValidator; /** @var CORSMiddlewareController */ private $controller; @@ -39,6 +43,8 @@ protected function setUp(): void { $this->reflector = new ControllerMethodReflector(); $this->session = $this->createMock(Session::class); $this->throttler = $this->createMock(Throttler::class); + $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); + $this->csrfValidator = new CsrfValidator($this->csrfTokenManager); $this->controller = new CORSMiddlewareController( 'test', $this->createMock(IRequest::class) @@ -66,7 +72,7 @@ public function testSetCORSAPIHeader(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -83,7 +89,7 @@ public function testNoAnnotationNoCORSHEADER(): void { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $response = $middleware->afterController($this->controller, __FUNCTION__, new Response()); $headers = $response->getHeaders(); @@ -107,7 +113,7 @@ public function testNoOriginHeaderNoCORSHEADER(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -137,7 +143,7 @@ public function testCorsIgnoredIfWithCredentialsHeaderPresent(string $method): v $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $response = new Response(); $response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE'); @@ -163,7 +169,7 @@ public function testNoCORSOnAnonymousPublicPage(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(false); @@ -197,7 +203,7 @@ public function testCORSShouldNeverAllowCookieAuth(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(true); @@ -238,7 +244,7 @@ public function testCORSShouldRelogin(string $method): void { ->with($this->equalTo('user'), $this->equalTo('pass')) ->willReturn(true); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $middleware->beforeController($this->controller, $method); } @@ -271,7 +277,7 @@ public function testCORSShouldFailIfPasswordLoginIsForbidden(string $method): vo ->with($this->equalTo('user'), $this->equalTo('pass')) ->will($this->throwException(new \OC\Authentication\Exceptions\PasswordLoginForbiddenException)); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $middleware->beforeController($this->controller, $method); } @@ -304,7 +310,7 @@ public function testCORSShouldNotAllowCookieAuth(string $method): void { ->with($this->equalTo('user'), $this->equalTo('pass')) ->willReturn(false); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $middleware->beforeController($this->controller, $method); } @@ -318,7 +324,7 @@ public function testAfterExceptionWithSecurityExceptionNoStatus() { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception')); $expected = new JSONResponse(['message' => 'A security exception'], 500); @@ -334,7 +340,7 @@ public function testAfterExceptionWithSecurityExceptionWithStatus() { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception', 501)); $expected = new JSONResponse(['message' => 'A security exception'], 501); @@ -353,7 +359,7 @@ public function testAfterExceptionWithRegularException() { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->csrfValidator); $middleware->afterException($this->controller, __FUNCTION__, new \Exception('A regular exception')); } } diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 7c59a8c1452b8..4664570ab6afc 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -32,6 +32,9 @@ use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Middleware\Security\SecurityMiddleware; use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Security\CSRF\CsrfToken; +use OC\Security\CSRF\CsrfTokenManager; +use OC\Security\CSRF\CsrfValidator; use OC\Settings\AuthorizedGroupMapper; use OCP\App\IAppManager; use OCP\AppFramework\Http\JSONResponse; @@ -76,6 +79,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { private $userSession; /** @var AuthorizedGroupMapper|\PHPUnit\Framework\MockObject\MockObject */ private $authorizedGroupMapper; + private CsrfTokenManager $csrfTokenManager; + private CsrfValidator $csrfValidator; protected function setUp(): void { parent::setUp(); @@ -92,6 +97,8 @@ protected function setUp(): void { $this->navigationManager = $this->createMock(INavigationManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->l10n = $this->createMock(IL10N::class); + $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); + $this->csrfValidator = new CsrfValidator($this->csrfTokenManager); $this->middleware = $this->getMiddleware(true, true, false); $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); @@ -116,7 +123,8 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA $this->appManager, $this->l10n, $this->authorizedGroupMapper, - $this->userSession + $this->userSession, + $this->csrfValidator, ); } @@ -322,12 +330,18 @@ private function securityCheck($method, $expects, $shouldFail = false) { public function testCsrfCheck(string $method): void { $this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException::class); - $this->request->expects($this->once()) - ->method('passesCSRFCheck') - ->willReturn(false); - $this->request->expects($this->once()) + $this->request->expects($this->exactly(2)) ->method('passesStrictCookieCheck') ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn(''); + $this->request->expects($this->once()) + ->method('getHeader') + ->with('REQUESTTOKEN') + ->willReturn(''); + $this->reader->reflect($this->controller, $method); $this->middleware->beforeController($this->controller, $method); } @@ -348,11 +362,20 @@ public function testNoCsrfCheck(string $method) { * @dataProvider dataPublicPage */ public function testPassesCsrfCheck(string $method): void { - $this->request->expects($this->once()) - ->method('passesCSRFCheck') + $this->request->expects($this->exactly(2)) + ->method('passesStrictCookieCheck') ->willReturn(true); $this->request->expects($this->once()) - ->method('passesStrictCookieCheck') + ->method('getParam') + ->with('requesttoken', '') + ->willReturn(''); + $this->request->expects($this->once()) + ->method('getHeader') + ->with('REQUESTTOKEN') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) ->willReturn(true); $this->reader->reflect($this->controller, $method); @@ -365,12 +388,21 @@ public function testPassesCsrfCheck(string $method): void { public function testFailCsrfCheck(string $method): void { $this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException::class); - $this->request->expects($this->once()) - ->method('passesCSRFCheck') - ->willReturn(false); - $this->request->expects($this->once()) + $this->request->expects($this->exactly(2)) ->method('passesStrictCookieCheck') ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn(''); + $this->request->expects($this->once()) + ->method('getHeader') + ->with('REQUESTTOKEN') + ->willReturn('foobar'); + $this->csrfTokenManager->expects($this->once()) + ->method('isTokenValid') + ->with(new CsrfToken('foobar')) + ->willReturn(false); $this->reader->reflect($this->controller, $method); $this->middleware->beforeController($this->controller, $method); @@ -387,6 +419,12 @@ public function testStrictCookieRequiredCheck(string $method): void { $this->request->expects($this->once()) ->method('passesStrictCookieCheck') ->willReturn(false); + $this->request->expects($this->never()) + ->method('getParam'); + $this->request->expects($this->never()) + ->method('getHeader'); + $this->csrfTokenManager->expects($this->never()) + ->method('isTokenValid'); $this->reader->reflect($this->controller, $method); $this->middleware->beforeController($this->controller, $method); @@ -442,6 +480,9 @@ public function testCsrfOcsController(string $controllerClass, bool $hasOcsApiHe $this->request ->method('getHeader') ->willReturnCallback(function ($header) use ($hasOcsApiHeader, $hasBearerAuth) { + if ($header === 'REQUESTTOKEN') { + return ''; + } if ($header === 'OCS-APIREQUEST' && $hasOcsApiHeader) { return 'true'; } @@ -450,9 +491,15 @@ public function testCsrfOcsController(string $controllerClass, bool $hasOcsApiHe } return ''; }); - $this->request->expects($this->once()) + $this->request->expects($this->exactly(2)) ->method('passesStrictCookieCheck') ->willReturn(true); + $this->request->expects($this->once()) + ->method('getParam') + ->with('requesttoken', '') + ->willReturn(''); + $this->csrfTokenManager->expects($this->never()) + ->method('isTokenValid'); $controller = new $controllerClass('test', $this->request); diff --git a/tests/lib/EventSourceFactoryTest.php b/tests/lib/EventSourceFactoryTest.php index 67bc900bd4c03..c0f8b700a5324 100644 --- a/tests/lib/EventSourceFactoryTest.php +++ b/tests/lib/EventSourceFactoryTest.php @@ -23,13 +23,15 @@ namespace Test; use OC\EventSourceFactory; +use OC\Security\CSRF\CsrfValidator; use OCP\IEventSource; use OCP\IRequest; class EventSourceFactoryTest extends TestCase { public function testCreate(): void { $request = $this->createMock(IRequest::class); - $factory = new EventSourceFactory($request); + $csrfValidator = $this->createMock(CsrfValidator::class); + $factory = new EventSourceFactory($request, $csrfValidator); $instance = $factory->create(); $this->assertInstanceOf(IEventSource::class, $instance); diff --git a/tests/lib/Security/CSRF/CsrfValidatorTest.php b/tests/lib/Security/CSRF/CsrfValidatorTest.php new file mode 100644 index 0000000000000..4efa482761bc8 --- /dev/null +++ b/tests/lib/Security/CSRF/CsrfValidatorTest.php @@ -0,0 +1,101 @@ + + * + * @author Daniel Kesselberg + * + * @license GNU AGPL version 3 or any later version + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace lib\Security\CSRF; + +use OC\Security\CSRF\CsrfTokenManager; +use OC\Security\CSRF\CsrfValidator; +use OCP\IRequest; +use Test\TestCase; + +class CsrfValidatorTest extends TestCase { + private CsrfTokenManager $csrfTokenManager; + private CsrfValidator $csrfValidator; + + protected function setUp(): void { + parent::setUp(); + + $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); + $this->csrfValidator = new CsrfValidator($this->csrfTokenManager); + } + + public function testFailStrictCookieCheck(): void { + $request = $this->createMock(IRequest::class); + $request->method('passesStrictCookieCheck') + ->willReturn(false); + + $this->assertFalse($this->csrfValidator->validate($request)); + } + + public function testFailMissingToken(): void { + $request = $this->createMock(IRequest::class); + $request->method('passesStrictCookieCheck') + ->willReturn(true); + $request->method('getParam') + ->with('requesttoken', '') + ->willReturn(''); + $request->method('getHeader') + ->with('REQUESTTOKEN') + ->willReturn(''); + + $this->assertFalse($this->csrfValidator->validate($request)); + } + + public function testFailInvalidToken(): void { + $request = $this->createMock(IRequest::class); + $request->method('passesStrictCookieCheck') + ->willReturn(true); + $request->method('getParam') + ->with('requesttoken', '') + ->willReturn('token123'); + $request->method('getHeader') + ->with('REQUESTTOKEN') + ->willReturn(''); + + $this->csrfTokenManager + ->method('isTokenValid') + ->willReturn(false); + + $this->assertFalse($this->csrfValidator->validate($request)); + } + + public function testPass(): void { + $request = $this->createMock(IRequest::class); + $request->method('passesStrictCookieCheck') + ->willReturn(true); + $request->method('getParam') + ->with('requesttoken', '') + ->willReturn('token123'); + $request->method('getHeader') + ->with('REQUESTTOKEN') + ->willReturn(''); + + $this->csrfTokenManager + ->method('isTokenValid') + ->willReturn(true); + + $this->assertTrue($this->csrfValidator->validate($request)); + } +}