Skip to content

Commit

Permalink
feat: move csrf validation out of request
Browse files Browse the repository at this point in the history
CsrfTokenManager is a heavy dependency that needs a user session and working memory cache.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
  • Loading branch information
kesselb committed May 25, 2023
1 parent b6bb032 commit ac0652e
Show file tree
Hide file tree
Showing 18 changed files with 242 additions and 39 deletions.
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/caldav.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/carddav.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/webdav.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 6 additions & 1 deletion apps/dav/lib/Connector/Sabre/Auth.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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') {
Expand Down
5 changes: 4 additions & 1 deletion apps/dav/lib/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
30 changes: 17 additions & 13 deletions apps/dav/tests/unit/Connector/Sabre/AuthTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -59,6 +60,7 @@ class AuthTest extends TestCase {
private $twoFactorManager;
/** @var Throttler */
private $throttler;
private CsrfValidator $csrfValidator;

protected function setUp(): void {
parent::setUp();
Expand All @@ -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,
);
}

Expand Down Expand Up @@ -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 = [
Expand Down Expand Up @@ -322,9 +326,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);
}
Expand Down Expand Up @@ -372,9 +376,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')
Expand Down Expand Up @@ -426,9 +430,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);
}
Expand Down Expand Up @@ -472,9 +476,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);
Expand Down Expand Up @@ -541,9 +545,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);
Expand Down
8 changes: 6 additions & 2 deletions core/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,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;
Expand Down Expand Up @@ -75,6 +76,7 @@ class LoginController extends Controller {
private WebAuthnManager $webAuthnManager;
private IManager $manager;
private IL10N $l10n;
private CsrfValidator $csrfValidator;

public function __construct(?string $appName,
IRequest $request,
Expand All @@ -88,7 +90,8 @@ public function __construct(?string $appName,
IInitialStateService $initialStateService,
WebAuthnManager $webAuthnManager,
IManager $manager,
IL10N $l10n) {
IL10N $l10n,
CsrfValidator $csrfValidator) {
parent::__construct($appName, $request);
$this->userManager = $userManager;
$this->config = $config;
Expand All @@ -101,6 +104,7 @@ public function __construct(?string $appName,
$this->webAuthnManager = $webAuthnManager;
$this->manager = $manager;
$this->l10n = $l10n;
$this->csrfValidator = $csrfValidator;
}

/**
Expand Down Expand Up @@ -296,7 +300,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
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -1550,6 +1550,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',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -1583,6 +1583,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',
Expand Down
6 changes: 4 additions & 2 deletions lib/private/AppFramework/DependencyInjection/DIContainer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -56,6 +57,7 @@ class CORSMiddleware extends Middleware {
private $session;
/** @var Throttler */
private $throttler;
private CsrfValidator $csrfValidator;

/**
* @param IRequest $request
Expand All @@ -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;
}

/**
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -102,6 +103,7 @@ class SecurityMiddleware extends Middleware {
private $groupAuthorizationMapper;
/** @var IUserSession */
private $userSession;
private $csrfValidator;

public function __construct(IRequest $request,
ControllerMethodReflector $reflector,
Expand All @@ -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;
Expand All @@ -130,6 +133,7 @@ public function __construct(IRequest $request,
$this->l10n = $l10n;
$this->groupAuthorizationMapper = $mapper;
$this->userSession = $userSession;
$this->csrfValidator = $csrfValidator;
}

/**
Expand Down Expand Up @@ -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' ||
strpos($this->request->getHeader('Authorization'), 'Bearer ') === 0
Expand Down
Loading

0 comments on commit ac0652e

Please sign in to comment.