diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index faa277285f6fb..5c08e8680e2c2 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -16,6 +16,7 @@ use OCA\DAV\Connector\Sabre\MaintenancePlugin; use OCA\DAV\Connector\Sabre\Principal; use OCP\Accounts\IAccountManager; +use OCP\Security\CSRF\ICsrfValidator; use Psr\Log\LoggerInterface; $authBackend = new Auth( @@ -24,6 +25,7 @@ \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), \OC::$server->getBruteForceThrottler(), + \OC::$server->get(ICsrfValidator::class), 'principals/' ); $principalBackend = new Principal( diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index aa721c9553117..73a28c5a3f7b8 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -18,6 +18,7 @@ use OCA\DAV\Connector\Sabre\Principal; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; +use OCP\Security\CSRF\ICsrfValidator; use Psr\Log\LoggerInterface; use Sabre\CardDAV\Plugin; @@ -27,6 +28,7 @@ \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), \OC::$server->getBruteForceThrottler(), + \OC::$server->get(ICsrfValidator::class), 'principals/' ); $principalBackend = new Principal( diff --git a/apps/dav/appinfo/v1/webdav.php b/apps/dav/appinfo/v1/webdav.php index 1683c29ca80f5..8f03fee94993c 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -5,6 +5,8 @@ * SPDX-FileCopyrightText: 2016 ownCloud, Inc. * SPDX-License-Identifier: AGPL-3.0-only */ + +use OCP\Security\CSRF\ICsrfValidator; use Psr\Log\LoggerInterface; // no php execution timeout for webdav @@ -38,6 +40,7 @@ \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), \OC::$server->getBruteForceThrottler(), + \OC::$server->get(ICsrfValidator::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 09495d27976ef..c8f639d3c085b 100644 --- a/apps/dav/lib/Connector/Sabre/Auth.php +++ b/apps/dav/lib/Connector/Sabre/Auth.php @@ -17,6 +17,7 @@ use OCP\ISession; use OCP\Security\Bruteforce\IThrottler; use OCP\Security\Bruteforce\MaxDelayReached; +use OCP\Security\CSRF\ICsrfValidator; use Psr\Log\LoggerInterface; use Sabre\DAV\Auth\Backend\AbstractBasic; use Sabre\DAV\Exception\NotAuthenticated; @@ -39,6 +40,7 @@ public function __construct(ISession $session, IRequest $request, Manager $twoFactorManager, IThrottler $throttler, + private ICsrfValidator $csrfValidator, string $principalPrefix = 'principals/users/') { $this->session = $session; $this->userSession = $userSession; @@ -164,7 +166,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 133ac55f303ed..73fb87012be3f 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -55,6 +55,8 @@ use OCP\IRequest; use OCP\Profiler\IProfiler; use OCP\SabrePluginEvent; +use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\CSRF\ICsrfValidator; use Psr\Log\LoggerInterface; use Sabre\CardDAV\VCFExportPlugin; use Sabre\DAV\Auth\Plugin; @@ -95,7 +97,8 @@ public function __construct(IRequest $request, string $baseUri) { \OC::$server->getUserSession(), \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), - \OC::$server->getBruteForceThrottler() + \OC::$server->get(IThrottler::class), + \OC::$server->get(ICsrfValidator::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 c9868debb43a9..54882bd30222a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php @@ -8,11 +8,13 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; use OC\Authentication\TwoFactorAuth\Manager; +use OC\Security\CSRF\CsrfValidator; use OC\User\Session; use OCP\IRequest; use OCP\ISession; use OCP\IUser; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\CSRF\ICsrfValidator; use Sabre\DAV\Server; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; @@ -37,6 +39,7 @@ class AuthTest extends TestCase { private $twoFactorManager; /** @var IThrottler */ private $throttler; + private ICsrfValidator $csrfValidator; protected function setUp(): void { parent::setUp(); @@ -52,12 +55,14 @@ protected function setUp(): void { $this->throttler = $this->getMockBuilder(IThrottler::class) ->disableOriginalConstructor() ->getMock(); + $this->csrfValidator = $this->createMock(ICsrfValidator::class); $this->auth = new \OCA\DAV\Connector\Sabre\Auth( $this->session, $this->userSession, $this->request, $this->twoFactorManager, - $this->throttler + $this->throttler, + $this->csrfValidator, ); } @@ -248,9 +253,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 = [ @@ -295,9 +300,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); } @@ -340,9 +345,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') @@ -389,9 +394,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); } @@ -430,9 +435,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); @@ -499,9 +504,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 f116ffab2afa4..01815385c4227 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -38,6 +38,7 @@ use OCP\IUserManager; use OCP\Notification\IManager; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\CSRF\ICsrfValidator; use OCP\Util; class LoginController extends Controller { @@ -60,6 +61,7 @@ public function __construct( private IManager $manager, private IL10N $l10n, private IAppManager $appManager, + private ICsrfValidator $csrfValidator, ) { parent::__construct($appName, $request); } @@ -207,7 +209,7 @@ private function setPasswordResetInitialState(?string $username): void { $this->canResetPassword($passwordLink, $user) ); } - + /** * Sets the initial state of whether or not a user is allowed to login with their email * initial state is passed in the array of 1 for email allowed and 0 for not allowed @@ -284,7 +286,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 8e1408e121ef6..5d7e73f1af8c3 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -627,6 +627,7 @@ 'OCP\\Security\\Bruteforce\\IThrottler' => $baseDir . '/lib/public/Security/Bruteforce/IThrottler.php', 'OCP\\Security\\Bruteforce\\MaxDelayReached' => $baseDir . '/lib/public/Security/Bruteforce/MaxDelayReached.php', 'OCP\\Security\\CSP\\AddContentSecurityPolicyEvent' => $baseDir . '/lib/public/Security/CSP/AddContentSecurityPolicyEvent.php', + 'OCP\\Security\\CSRF\\ICsrfValidator' => $baseDir . '/lib/public/Security/CSRF/ICsrfValidator.php', 'OCP\\Security\\Events\\GenerateSecurePasswordEvent' => $baseDir . '/lib/public/Security/Events/GenerateSecurePasswordEvent.php', 'OCP\\Security\\Events\\ValidatePasswordPolicyEvent' => $baseDir . '/lib/public/Security/Events/ValidatePasswordPolicyEvent.php', 'OCP\\Security\\FeaturePolicy\\AddFeaturePolicyEvent' => $baseDir . '/lib/public/Security/FeaturePolicy/AddFeaturePolicyEvent.php', @@ -1775,6 +1776,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 d6939ae36ce8b..fc0f96ba5cc5c 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -668,6 +668,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Security\\Bruteforce\\IThrottler' => __DIR__ . '/../../..' . '/lib/public/Security/Bruteforce/IThrottler.php', 'OCP\\Security\\Bruteforce\\MaxDelayReached' => __DIR__ . '/../../..' . '/lib/public/Security/Bruteforce/MaxDelayReached.php', 'OCP\\Security\\CSP\\AddContentSecurityPolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/CSP/AddContentSecurityPolicyEvent.php', + 'OCP\\Security\\CSRF\\ICsrfValidator' => __DIR__ . '/../../..' . '/lib/public/Security/CSRF/ICsrfValidator.php', 'OCP\\Security\\Events\\GenerateSecurePasswordEvent' => __DIR__ . '/../../..' . '/lib/public/Security/Events/GenerateSecurePasswordEvent.php', 'OCP\\Security\\Events\\ValidatePasswordPolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/Events/ValidatePasswordPolicyEvent.php', 'OCP\\Security\\FeaturePolicy\\AddFeaturePolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/FeaturePolicy/AddFeaturePolicyEvent.php', @@ -1816,6 +1817,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 9e3119d262ae7..18555f497b624 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -46,6 +46,7 @@ use OCP\IURLGenerator; use OCP\IUserSession; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\CSRF\ICsrfValidator; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -207,7 +208,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $c->get(IRequest::class), $c->get(IControllerMethodReflector::class), $c->get(IUserSession::class), - $c->get(IThrottler::class) + $c->get(IThrottler::class), + $c->get(ICsrfValidator::class), ) ); $dispatcher->registerMiddleware( @@ -231,7 +233,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $server->getAppManager(), $server->getL10N('lib'), $c->get(AuthorizedGroupMapper::class), - $server->get(IUserSession::class) + $server->get(IUserSession::class), + $c->get(ICsrfValidator::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 7b617b22e3c25..ee58790a6085b 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -21,6 +21,7 @@ use OCP\IRequest; use OCP\ISession; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\CSRF\ICsrfValidator; use ReflectionMethod; /** @@ -42,7 +43,8 @@ class CORSMiddleware extends Middleware { public function __construct(IRequest $request, ControllerMethodReflector $reflector, Session $session, - IThrottler $throttler) { + IThrottler $throttler, + private ICsrfValidator $csrfValidator) { $this->request = $request; $this->reflector = $reflector; $this->session = $session; @@ -70,7 +72,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; } // Skip CORS check for requests with AppAPI auth. diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index a38ad610fc609..4db1917fcac16 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -36,6 +36,7 @@ use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUserSession; +use OCP\Security\CSRF\ICsrfValidator; use OCP\Util; use Psr\Log\LoggerInterface; use ReflectionMethod; @@ -86,7 +87,8 @@ public function __construct(IRequest $request, IAppManager $appManager, IL10N $l10n, AuthorizedGroupMapper $mapper, - IUserSession $userSession + IUserSession $userSession, + private ICsrfValidator $csrfValidator ) { $this->navigationManager = $navigationManager; $this->request = $request; @@ -213,7 +215,7 @@ private function isInvalidCSRFRequired(ReflectionMethod $reflectionMethod): bool return false; } - return !$this->request->passesCSRFCheck(); + return !$this->csrfValidator->validate($this->request); } private function isValidOCSRequest(): bool { diff --git a/lib/private/EventSource.php b/lib/private/EventSource.php index dbeda25049e0c..bf56003365ccf 100644 --- a/lib/private/EventSource.php +++ b/lib/private/EventSource.php @@ -10,6 +10,7 @@ use OCP\IEventSource; use OCP\IRequest; +use OCP\Security\CSRF\ICsrfValidator; class EventSource implements IEventSource { private bool $fallback = false; @@ -18,6 +19,7 @@ class EventSource implements IEventSource { public function __construct( private IRequest $request, + private ICsrfValidator $csrfValidator, ) { } @@ -54,7 +56,7 @@ protected function init(): void { 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/EventSourceFactory.php b/lib/private/EventSourceFactory.php index 57888b1be4c8f..ad6abee3cb829 100644 --- a/lib/private/EventSourceFactory.php +++ b/lib/private/EventSourceFactory.php @@ -12,14 +12,19 @@ use OCP\IEventSource; use OCP\IEventSourceFactory; use OCP\IRequest; +use OCP\Security\CSRF\ICsrfValidator; class EventSourceFactory implements IEventSourceFactory { public function __construct( private IRequest $request, + private ICsrfValidator $csrfValidator, ) { } public function create(): IEventSource { - return new EventSource($this->request); + return new 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..59a69142e20fe --- /dev/null +++ b/lib/private/Security/CSRF/CsrfValidator.php @@ -0,0 +1,37 @@ +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/Server.php b/lib/private/Server.php index bcdf482f02d0e..5cd631308a25d 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -98,6 +98,7 @@ use OC\Security\CSP\ContentSecurityPolicyManager; use OC\Security\CSP\ContentSecurityPolicyNonceManager; use OC\Security\CSRF\CsrfTokenManager; +use OC\Security\CSRF\CsrfValidator; use OC\Security\CSRF\TokenStorage\SessionStorage; use OC\Security\Hasher; use OC\Security\RateLimiting\Limiter; @@ -209,6 +210,7 @@ use OCP\RichObjectStrings\IValidator; use OCP\Route\IRouter; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\CSRF\ICsrfValidator; use OCP\Security\IContentSecurityPolicyManager; use OCP\Security\ICredentialsManager; use OCP\Security\ICrypto; @@ -1401,6 +1403,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerAlias(\OCP\TaskProcessing\IManager::class, \OC\TaskProcessing\Manager::class); + $this->registerAlias(ICsrfValidator::class, CsrfValidator::class); + $this->connectDispatcher(); } diff --git a/lib/private/legacy/OC_JSON.php b/lib/private/legacy/OC_JSON.php index a9682e2cce7df..567a4b6b3d61c 100644 --- a/lib/private/legacy/OC_JSON.php +++ b/lib/private/legacy/OC_JSON.php @@ -7,6 +7,8 @@ */ use OC\Authentication\TwoFactorAuth\Manager as TwoFactorAuthManager; +use OCP\IRequest; +use OCP\Security\CSRF\ICsrfValidator; class OC_JSON { /** @@ -45,12 +47,16 @@ public static function checkLoggedIn() { * @suppress PhanDeprecatedFunction */ public static function callCheck() { - if (!\OC::$server->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(ICsrfValidator::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 18efd7a6d1601..db82d72891ebc 100644 --- a/lib/public/IRequest.php +++ b/lib/public/IRequest.php @@ -177,6 +177,7 @@ public function getCookie(string $key); * * @return bool true if CSRF check passed * @since 6.0.0 + * @deprecated 30.0.0 use \OCP\Security\CSRF\ICsrfValidator::validate instead */ public function passesCSRFCheck(): bool; diff --git a/lib/public/Security/CSRF/ICsrfValidator.php b/lib/public/Security/CSRF/ICsrfValidator.php new file mode 100644 index 0000000000000..7e6453509e52d --- /dev/null +++ b/lib/public/Security/CSRF/ICsrfValidator.php @@ -0,0 +1,24 @@ +request = $this->createMock(IRequest::class); @@ -101,6 +107,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') @@ -126,6 +134,7 @@ protected function setUp(): void { $this->notificationManager, $this->l, $this->appManager, + $this->csrfValidator, ); } @@ -437,9 +446,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, @@ -472,9 +488,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, @@ -504,9 +527,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') @@ -534,9 +564,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') @@ -566,9 +603,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, @@ -597,9 +641,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 ab06b020c9bdf..49afba1ee7805 100644 --- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php @@ -10,6 +10,8 @@ use OC\AppFramework\Middleware\Security\CORSMiddleware; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Security\CSRF\CsrfTokenManager; +use OC\Security\CSRF\CsrfValidator; use OC\User\Session; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\Response; @@ -27,6 +29,8 @@ class CORSMiddlewareTest extends \Test\TestCase { private $session; /** @var IThrottler|MockObject */ private $throttler; + private CsrfTokenManager $csrfTokenManager; + private CsrfValidator $csrfValidator; /** @var CORSMiddlewareController */ private $controller; @@ -35,6 +39,8 @@ protected function setUp(): void { $this->reflector = new ControllerMethodReflector(); $this->session = $this->createMock(Session::class); $this->throttler = $this->createMock(IThrottler::class); + $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); + $this->csrfValidator = new CsrfValidator($this->csrfTokenManager); $this->controller = new CORSMiddlewareController( 'test', $this->createMock(IRequest::class) @@ -62,7 +68,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(); @@ -79,7 +85,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(); @@ -103,7 +109,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(); @@ -133,7 +139,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'); @@ -159,7 +165,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); @@ -193,7 +199,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); @@ -234,7 +240,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); } @@ -267,7 +273,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); } @@ -300,7 +306,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); } @@ -314,7 +320,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); @@ -330,7 +336,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); @@ -349,7 +355,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 1d7753a347777..0f1d0b99c1094 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -17,6 +17,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; @@ -61,6 +64,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(); @@ -77,6 +82,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); @@ -101,7 +108,8 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA $this->appManager, $this->l10n, $this->authorizedGroupMapper, - $this->userSession + $this->userSession, + $this->csrfValidator, ); } @@ -307,12 +315,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); } @@ -333,11 +347,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); @@ -350,12 +373,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); @@ -372,6 +404,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); @@ -427,6 +465,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'; } @@ -435,9 +476,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 c5e22a8fe34a3..abd23e53bb57d 100644 --- a/tests/lib/EventSourceFactoryTest.php +++ b/tests/lib/EventSourceFactoryTest.php @@ -9,11 +9,13 @@ use OC\EventSourceFactory; use OCP\IEventSource; use OCP\IRequest; +use OCP\Security\CSRF\ICsrfValidator; class EventSourceFactoryTest extends TestCase { public function testCreate(): void { $request = $this->createMock(IRequest::class); - $factory = new EventSourceFactory($request); + $csrfValidator = $this->createMock(ICsrfValidator::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..e6eb92952b906 --- /dev/null +++ b/tests/lib/Security/CSRF/CsrfValidatorTest.php @@ -0,0 +1,85 @@ +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)); + } +}