From f1bc5151cd7693d6dcf98a33cb33c463eea9be75 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 19:49:07 +0200 Subject: [PATCH] fix: the login form carries a request token too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Csrf::check()` opened with `isLoggedIn()`, and `initialize()` 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 reachable here with a bare cross-site form:
`analyzeEncrypted()` falls back to the raw request value when what it received is not RSA ciphertext — deliberately, for scripted installs — so the attacker does not even need the installation's public key. SameSite does not stand in the way: it governs whether an existing cookie is *sent*, and this attack does not want the victim's cookie, it wants the response to set a new one. The victim's browser ends up holding a session authenticated as the attacker, with no visible logout, and everything they file afterwards — accounts, passwords — goes into a vault the attacker can open. Two changes, and the second is the one that matters: - The token is minted for any session, not only an authenticated one. It has to exist before the request that needs it, and the request that needs it most is the sign-in. - A state-changing request whose session holds no token is now refused rather than passed. Without that the check still means nothing before sign-in: a browser that has never loaded a page of ours has no token, which is exactly the request to refuse. Being signed in no longer takes part in the decision either way. The token is the check. The unit tests pinned the old contract and are rewritten against the new one, including that a plain read is still let through — the token cannot travel on a bare GET, there is no header to put it in. Three integration tests also had to change, and what they showed is worth recording: they build their own SessionContext stub with isLoggedIn true and no getCSRF, so the old guard skipped them on the `getCSRF() !== null` clause and they were never exercising CSRF at all. They model a real session now, which is what the shared harness has always done. --- src/Infrastructure/Crypt/Csrf.php | 56 ++++++++---- .../UserProfile/UserProfileTest.php | 3 + .../UserSettingsGeneralTest.php | 3 + tests/Unit/Infrastructure/Crypt/CsrfTest.php | 87 +++++++++---------- 4 files changed, 87 insertions(+), 62 deletions(-) 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()); }