From 3692820ce3b815dcec0152d9c30721f264fff2d4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 28 Feb 2024 00:22:56 +0100 Subject: [PATCH 1/5] fix(loginflow): report reason for token mismatch Signed-off-by: Arthur Schiwon --- core/Controller/ClientFlowLoginController.php | 21 ++++++++++++++++++- .../ClientFlowLoginV2Controller.php | 18 +++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 76079e710e3ae..ce8780e540b21 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -59,6 +59,7 @@ use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; use OCP\Session\Exceptions\SessionNotAvailableException; +use function OCP\Log\logger; #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)] class ClientFlowLoginController extends Controller { @@ -91,9 +92,24 @@ private function getClientName(): string { private function isValidToken(string $stateToken): bool { $currentToken = $this->session->get(self::STATE_NAME); if (!is_string($currentToken)) { + logger('core')->error('Client login flow state token is not set', [ + 'sessionToken' => $currentToken, + 'requestToken' => $stateToken, + 'loginFlow' => 'v1', + ]); return false; } - return hash_equals($currentToken, $stateToken); + $isValid = hash_equals($currentToken, $stateToken); + if (!$isValid) { + logger('core')->error('Client login flow state token does not match', + [ + 'sessionToken' => $currentToken, + 'requestToken' => $stateToken, + 'loginFlow' => 'v1', + ] + ); + } + return $isValid; } private function stateTokenForbiddenResponse(): StandaloneTemplateResponse { @@ -147,6 +163,9 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user = ISecureRandom::CHAR_LOWER.ISecureRandom::CHAR_UPPER.ISecureRandom::CHAR_DIGITS ); $this->session->set(self::STATE_NAME, $stateToken); + logger('core')->error('Client login flow state token set', [ + 'token' => $stateToken, + ]); $csp = new Http\ContentSecurityPolicy(); if ($client) { diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index d5e9ce492b999..07d53045e53c5 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -49,6 +49,7 @@ use OCP\IUser; use OCP\IUserSession; use OCP\Security\ISecureRandom; +use function OCP\Log\logger; /** * @psalm-import-type CoreLoginFlowV2Credentials from ResponseDefinitions @@ -321,9 +322,24 @@ public function init(): JSONResponse { private function isValidStateToken(string $stateToken): bool { $currentToken = $this->session->get(self::STATE_NAME); if (!is_string($stateToken) || !is_string($currentToken)) { + logger('core')->error('Client login flow state token is not set', [ + 'sessionToken' => $currentToken, + 'requestToken' => $stateToken, + 'loginFlow' => 'v2', + ]); return false; } - return hash_equals($currentToken, $stateToken); + $isValid = hash_equals($currentToken, $stateToken); + if (!$isValid) { + logger('core')->error('Client login flow state token does not match', + [ + 'sessionToken' => $currentToken, + 'requestToken' => $stateToken, + 'loginFlow' => 'v2', + ] + ); + } + return $isValid; } private function stateTokenMissingResponse(): StandaloneTemplateResponse { From 2e6e035e16674fccb68e17b46187dde77a3b17b7 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 28 Feb 2024 00:40:32 +0100 Subject: [PATCH 2/5] fix(loginflow): log setting, removing and clearing state token in session Signed-off-by: Arthur Schiwon --- lib/private/Session/CryptoSessionData.php | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php index 34aab2a51653c..54792aae009b7 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -117,6 +117,12 @@ public function set(string $key, $value) { $reopened = $this->reopen(); $this->sessionValues[$key] = $value; + if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') { + logger('core')->error('Saving state token with session', [ + 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', + 'stateToken' => $value, + ]); + } $this->isModified = true; if ($reopened) { $this->close(); @@ -155,6 +161,14 @@ public function exists(string $key): bool { public function remove(string $key) { $reopened = $this->reopen(); $this->isModified = true; + if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') { + $e = new \Exception(); + logger('core')->error('Removing state token from session', [ + 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', + 'stateToken' => $this->sessionValues[$key], + 'exception' => $e + ]); + } unset($this->sessionValues[$key]); if ($reopened) { $this->close(); @@ -167,6 +181,15 @@ public function remove(string $key) { public function clear() { $reopened = $this->reopen(); $requesttoken = $this->get('requesttoken'); + if ($this->exists('client.flow.v2.state.token') || $this->exists('client.flow.state.token')) { + $key = $this->exists('client.flow.v2.state.token') ? 'client.flow.v2.state.token' : 'client.flow.state.token'; + $e = new \Exception(); + logger('core')->error('Cleared session containing state token', [ + 'loginFlow' => $key === 'client.flow.v2.state.token' ? 'v2' : 'v1', + 'stateToken' => $this->sessionValues[$key], + 'exception' => $e + ]); + } $this->sessionValues = []; if ($requesttoken !== null) { $this->set('requesttoken', $requesttoken); From 3a231c47fa030b8045afb56f86d72a34d166c655 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 2 May 2024 21:07:03 +0200 Subject: [PATCH 3/5] fix(loginflow): log more session related data to setting state token - exisiting values - set values - retry once more if set and generated token differ - log session state Signed-off-by: Arthur Schiwon --- core/Controller/ClientFlowLoginController.php | 21 +++++++++++++++++++ .../ClientFlowLoginV2Controller.php | 21 +++++++++++++++++++ lib/private/Session/CryptoSessionData.php | 17 ++++++++++++++- lib/private/Session/Internal.php | 4 ++++ 4 files changed, 62 insertions(+), 1 deletion(-) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index ce8780e540b21..113957856e233 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -158,6 +158,12 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user = ); } + $oldStateToken = $this->session->get(self::STATE_NAME); + logger('core')->error('Fetching old state token - expected to be null', [ + 'loginFlow' => 'v1', + 'existingStateToken' => $oldStateToken, + ]); + $stateToken = $this->random->generate( 64, ISecureRandom::CHAR_LOWER.ISecureRandom::CHAR_UPPER.ISecureRandom::CHAR_DIGITS @@ -167,6 +173,21 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user = 'token' => $stateToken, ]); + $setStateToken = $this->session->get(self::STATE_NAME); + logger('core')->error('Fetching set state token - expected to match generated one', [ + 'loginFlow' => 'v1', + 'generatedStateToken' => $stateToken, + 'setStateToken' => $setStateToken, + ]); + + if ($stateToken !== $setStateToken) { + logger('core')->error('Generate and set state token mismatch, trying to set it again one more time', [ + 'loginFlow' => 'v1', + 'stateToken' => $stateToken, + ]); + $this->session->set(self::STATE_NAME, $stateToken); + } + $csp = new Http\ContentSecurityPolicy(); if ($client) { $csp->addAllowedFormActionDomain($client->getRedirectUri()); diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index 07d53045e53c5..5d322f22b000f 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -130,12 +130,33 @@ public function showAuthPickerPage(string $user = '', int $direct = 0): Standalo return $this->loginTokenForbiddenResponse(); } + $oldStateToken = $this->session->get(self::STATE_NAME); + logger('core')->error('Fetching old state token - expected to be null', [ + 'loginFlow' => 'v2', + 'existingStateToken' => $oldStateToken, + ]); + $stateToken = $this->random->generate( 64, ISecureRandom::CHAR_LOWER.ISecureRandom::CHAR_UPPER.ISecureRandom::CHAR_DIGITS ); $this->session->set(self::STATE_NAME, $stateToken); + $setStateToken = $this->session->get(self::STATE_NAME); + logger('core')->error('Fetching set state token - expected to match generated one', [ + 'loginFlow' => 'v2', + 'generatedStateToken' => $stateToken, + 'setStateToken' => $setStateToken, + ]); + + if ($stateToken !== $setStateToken) { + logger('core')->error('Generate and set state token mismatch, trying to set it again one more time', [ + 'loginFlow' => 'v2', + 'stateToken' => $stateToken, + ]); + $this->session->set(self::STATE_NAME, $stateToken); + } + return new StandaloneTemplateResponse( $this->appName, 'loginflowv2/authpicker', diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php index 54792aae009b7..404cbee9c2f89 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -110,12 +110,27 @@ protected function initializeSession() { * @param mixed $value */ public function set(string $key, $value) { - if ($this->get($key) === $value) { + $existingValue = $this->get($key); + if ($existingValue === $value) { + if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') { + logger('core')->error('State token value is already present!', [ + 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', + 'stateToken' => $value, + 'existingStateToken' => $existingValue, + ]); + } // Do not write the session if the value hasn't changed to avoid reopening return; } $reopened = $this->reopen(); + if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') { + logger('core')->error('Reporting on whether session was reopened', [ + 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', + 'sessionReopened' => $reopened, + ]); + } + $this->sessionValues[$key] = $value; if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') { logger('core')->error('Saving state token with session', [ diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index ecfab9d8b644b..9c50227c7a197 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -241,6 +241,10 @@ private function invoke(string $functionName, array $parameters = [], bool $sile } return $result; } catch (\Error $e) { + $this->logger?->error('trapping a session error', [ + 'loginFlow' => '?', + 'exception' => $e, + ]); $this->trapError($e->getCode(), $e->getMessage()); } } From b23b52b5431f7195201c8863c6eebbefe1df482f Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 2 Aug 2024 11:58:02 +0200 Subject: [PATCH 4/5] fix(loginflow): log info about regeneration session ids Signed-off-by: Arthur Schiwon --- lib/private/Session/CryptoSessionData.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php index 404cbee9c2f89..d4d3e97dfec75 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -232,6 +232,17 @@ public function reopen(): bool { * @return void */ public function regenerateId(bool $deleteOldSession = true, bool $updateToken = false) { + if ($this->exists('client.flow.v2.state.token') || $this->exists('client.flow.state.token')) { + $key = $this->exists('client.flow.v2.state.token') ? 'client.flow.v2.state.token' : 'client.flow.state.token'; + $e = new \Exception(); + logger('core')->error('Regenerating session ID', [ + 'loginFlow' => $key === 'client.flow.v2.state.token' ? 'v2' : 'v1', + 'stateToken' => $this->sessionValues[$key], + 'deleteOldSessionFile' => $deleteOldSession, + 'updateToken' => $updateToken, + 'exception' => $e, + ]); + } $this->session->regenerateId($deleteOldSession, $updateToken); } From bfa4cd353ba82eb42ad56dd751e860d99cc3ab09 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 30 Jan 2025 18:59:53 +0100 Subject: [PATCH 5/5] fix(loginflow): log backing session class Signed-off-by: Arthur Schiwon --- lib/private/Session/CryptoSessionData.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php index d4d3e97dfec75..b4a7857337186 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -96,6 +96,7 @@ protected function initializeSession() { } catch (\Exception $e) { logger('core')->critical('Could not decrypt or decode encrypted session data', [ 'exception' => $e, + 'backingSessionClass' => get_class($this->session), ]); $this->sessionValues = []; $this->regenerateId(true, false); @@ -117,6 +118,7 @@ public function set(string $key, $value) { 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', 'stateToken' => $value, 'existingStateToken' => $existingValue, + 'backingSessionClass' => get_class($this->session), ]); } // Do not write the session if the value hasn't changed to avoid reopening @@ -128,6 +130,7 @@ public function set(string $key, $value) { logger('core')->error('Reporting on whether session was reopened', [ 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', 'sessionReopened' => $reopened, + 'backingSessionClass' => get_class($this->session), ]); } @@ -136,6 +139,7 @@ public function set(string $key, $value) { logger('core')->error('Saving state token with session', [ 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', 'stateToken' => $value, + 'backingSessionClass' => get_class($this->session), ]); } $this->isModified = true; @@ -181,7 +185,8 @@ public function remove(string $key) { logger('core')->error('Removing state token from session', [ 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', 'stateToken' => $this->sessionValues[$key], - 'exception' => $e + 'exception' => $e, + 'backingSessionClass' => get_class($this->session), ]); } unset($this->sessionValues[$key]); @@ -202,7 +207,8 @@ public function clear() { logger('core')->error('Cleared session containing state token', [ 'loginFlow' => $key === 'client.flow.v2.state.token' ? 'v2' : 'v1', 'stateToken' => $this->sessionValues[$key], - 'exception' => $e + 'exception' => $e, + 'backingSessionClass' => get_class($this->session), ]); } $this->sessionValues = []; @@ -241,6 +247,7 @@ public function regenerateId(bool $deleteOldSession = true, bool $updateToken = 'deleteOldSessionFile' => $deleteOldSession, 'updateToken' => $updateToken, 'exception' => $e, + 'backingSessionClass' => get_class($this->session), ]); } $this->session->regenerateId($deleteOldSession, $updateToken);