diff --git a/src/Infrastructure/Crypt/Csrf.php b/src/Infrastructure/Crypt/Csrf.php index b366703fd..627f017a2 100644 --- a/src/Infrastructure/Crypt/Csrf.php +++ b/src/Infrastructure/Crypt/Csrf.php @@ -55,40 +55,66 @@ public function __construct( } /** - * Check for CSRF token on POST requests + * Check for CSRF token on state-changing requests + * + * This used to begin with `isLoggedIn()`, and `initialize()` below minted a token only for a + * session that was already authenticated — so every request made before signing in was + * unprotected, the sign-in itself above all. + * + * That is login CSRF, and it is not a theoretical one here. A cross-site form posting + * `user` and `pass` to `login/login` signs the victim's browser into the *attacker's* account: + * `analyzeEncrypted()` falls back to the raw value when what it got is not RSA ciphertext, so + * the attacker does not even need the installation's public key. SameSite does not help — it + * governs whether an existing cookie is sent, and this attack does not need the victim's + * cookie, it needs the response to set a new one. The victim then goes on filing accounts and + * passwords into a vault the attacker can open. + * + * A session with no token now fails a state-changing request rather than passing it. That is + * what makes the check mean anything before sign-in: a browser that has never loaded a page of + * ours has no token, which is exactly the request that should be refused. */ public function check(): bool { $method = $this->request->getMethod(); $with = $this->request->getHeader('X-Requested-With'); - if ($this->context->isLoggedIn() - && ($sessionToken = $this->context->getCSRF()) !== null - && ($method === Method::POST - || ($method === Method::GET && $with === 'XMLHttpRequest')) - ) { - $token = $this->request->getHeader('X-CSRF'); + $changesState = $method === Method::POST + || ($method === Method::GET && $with === 'XMLHttpRequest'); - if (empty($token) || !hash_equals($sessionToken, $token)) { - logger('Invalid CSRF token', 'ERROR'); + if (!$changesState) { + return true; + } + + $sessionToken = $this->context->getCSRF(); - return false; - } + if ($sessionToken === null) { + logger('No CSRF token for this session', 'ERROR'); - logger('CSRF token OK'); + return false; } + $token = $this->request->getHeader('X-CSRF'); + + if (empty($token) || !hash_equals($sessionToken, $token)) { + logger('Invalid CSRF token', 'ERROR'); + + return false; + } + + logger('CSRF token OK'); + return true; } /** * Initialize the CSRF token + * + * For any session, not only an authenticated one. The token has to exist before the request + * that needs it, and the request that needs it most is the sign-in. */ public function initialize(): void { - if ($this->context->isLoggedIn() - && $this->context->getCSRF() === null - ) { + if ($this->context->getCSRF() === null) { $this->context->setCSRF(bin2hex(random_bytes(self::TOKEN_BYTES))); logger('CSRF token set'); diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserProfile/UserProfileTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserProfile/UserProfileTest.php index ee3f48333..509cc97ae 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserProfile/UserProfileTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserProfile/UserProfileTest.php @@ -549,6 +549,9 @@ private function contextWithActorProfile(ProfileData $actorProfile, bool $isAdmi { $context = self::createStub(SessionContext::class); $context->method('isLoggedIn')->willReturn(true); + // A real session holds a request token; without one the guard has nothing to check + // against, which is the state it now refuses rather than waves through. + $context->method('getCSRF')->willReturn(self::CSRF_TOKEN); $context->method('getAuthCompleted')->willReturn(true); $context->method('getUserData')->willReturn($this->getUserDataDto()->mutate(['isAdminApp' => $isAdminApp])); $context->method('getUserProfile')->willReturn($actorProfile); diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserSettingsGeneral/UserSettingsGeneralTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserSettingsGeneral/UserSettingsGeneralTest.php index 1c76b18d3..288aec592 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserSettingsGeneral/UserSettingsGeneralTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserSettingsGeneral/UserSettingsGeneralTest.php @@ -88,6 +88,9 @@ protected function getContext(): SessionContext|Stub { $context = self::createStub(SessionContext::class); $context->method('isLoggedIn')->willReturn(true); + // A real session holds a request token; without one the guard has nothing to check + // against, which is the state it now refuses rather than waves through. + $context->method('getCSRF')->willReturn(self::CSRF_TOKEN); $context->method('getAuthCompleted')->willReturn(true); $context->method('getUserData')->willReturn($this->getUserDataDto()); $context->method('getUserProfile')->willReturn($this->getUserProfile()); diff --git a/tests/Unit/Infrastructure/Crypt/CsrfTest.php b/tests/Unit/Infrastructure/Crypt/CsrfTest.php index 59d265f8b..f4a99b24c 100644 --- a/tests/Unit/Infrastructure/Crypt/CsrfTest.php +++ b/tests/Unit/Infrastructure/Crypt/CsrfTest.php @@ -59,9 +59,8 @@ public static function httpMethodDataProvider(): array public function testInitialize() { $this->sessionContext - ->expects(self::once()) - ->method('isLoggedIn') - ->willReturn(true); + ->expects(self::never()) + ->method('isLoggedIn'); $this->sessionContext ->expects(self::once()) @@ -94,7 +93,6 @@ public function testInitializeIssuesADifferentTokenEachTime() { $tokens = []; - $this->sessionContext->method('isLoggedIn')->willReturn(true); $this->sessionContext->method('getCSRF')->willReturn(null); $this->sessionContext ->expects(self::exactly(2)) @@ -133,11 +131,6 @@ public function testCheckWithValidToken(Method $method, string $header) ->with(...self::withConsecutive(['X-Requested-With'], ['X-CSRF'])) ->willReturn($header, $sessionToken); - $this->sessionContext - ->expects(self::once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->sessionContext ->expects(self::once()) ->method('getCSRF') @@ -172,11 +165,6 @@ public function testCheckIsBoundToTheSessionTokenOnly(Method $method, string $he ->expects(self::never()) ->method('getClientAddress'); - $this->sessionContext - ->expects(self::once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->sessionContext ->expects(self::once()) ->method('getCSRF') @@ -202,11 +190,6 @@ public function testCheckWithNoToken(Method $method, string $header) ->with(...self::withConsecutive(['X-Requested-With'], ['X-CSRF'])) ->willReturn($header, ''); - $this->sessionContext - ->expects(self::once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->sessionContext ->expects(self::once()) ->method('getCSRF') @@ -216,54 +199,64 @@ public function testCheckWithNoToken(Method $method, string $header) } /** - * @return void + * A request that changes nothing is not checked. + * + * A plain GET without the ajax header is a page being read, and the token cannot travel on one + * — there is no header to put it in. */ - public function testCheckWithNoLogin() + public function testCheckLetsAPlainReadThrough() { - $this->requestInterface - ->expects(self::once()) - ->method('getMethod') - ->willReturn(Method::GET); - + $this->requestInterface->expects(self::once())->method('getMethod')->willReturn(Method::GET); $this->requestInterface ->expects(self::once()) ->method('getHeader') ->with('X-Requested-With') - ->willReturn('test'); + ->willReturn(''); - $this->sessionContext - ->expects(self::once()) - ->method('isLoggedIn') - ->willReturn(false); + $this->sessionContext->expects(self::never())->method('getCSRF'); self::assertTrue($this->csrf->check()); } /** - * @return void + * A session with no token fails a state-changing request rather than passing it. + * + * This is the case that used to be waved through, and with it every request made before + * signing in — the sign-in itself above all, which is login CSRF: a cross-site form posting a + * username and password signs the victim's browser into the attacker's account, and they go on + * filing passwords into a vault the attacker can open. A browser that has never loaded a page + * of ours holds no token, which is exactly the request to refuse. */ - public function testCheckWithNoCsrf() + #[DataProvider('httpMethodDataProvider')] + public function testCheckRefusesAStateChangingRequestWithNoSessionToken(Method $method, string $header) { - $this->requestInterface - ->expects(self::once()) - ->method('getMethod') - ->willReturn(Method::GET); - + $this->requestInterface->expects(self::once())->method('getMethod')->willReturn($method); $this->requestInterface ->expects(self::once()) ->method('getHeader') ->with('X-Requested-With') - ->willReturn('test'); + ->willReturn($header); - $this->sessionContext - ->expects(self::once()) - ->method('isLoggedIn') - ->willReturn(true); + $this->sessionContext->expects(self::once())->method('getCSRF')->willReturn(null); - $this->sessionContext - ->expects(self::once()) - ->method('getCSRF') - ->willReturn(null); + self::assertFalse($this->csrf->check()); + } + + /** + * Being signed in is not what decides it, either way. The token is the whole check. + */ + #[DataProvider('httpMethodDataProvider')] + public function testCheckDoesNotAskWhetherTheSessionIsSignedIn(Method $method, string $header) + { + $token = str_repeat('a', 64); + + $this->requestInterface->method('getMethod')->willReturn($method); + $this->requestInterface + ->method('getHeader') + ->willReturnMap([['X-Requested-With', $header], ['X-CSRF', $token]]); + + $this->sessionContext->expects(self::never())->method('isLoggedIn'); + $this->sessionContext->method('getCSRF')->willReturn($token); self::assertTrue($this->csrf->check()); }