diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 76079e710e3ae..113957856e233 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 { @@ -142,11 +158,35 @@ 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 ); $this->session->set(self::STATE_NAME, $stateToken); + logger('core')->error('Client login flow state token set', [ + '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) { diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index d5e9ce492b999..5d322f22b000f 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 @@ -129,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', @@ -321,9 +343,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 { diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php index 34aab2a51653c..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); @@ -110,13 +111,37 @@ 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, + 'backingSessionClass' => get_class($this->session), + ]); + } // 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, + 'backingSessionClass' => get_class($this->session), + ]); + } + $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, + 'backingSessionClass' => get_class($this->session), + ]); + } $this->isModified = true; if ($reopened) { $this->close(); @@ -155,6 +180,15 @@ 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, + 'backingSessionClass' => get_class($this->session), + ]); + } unset($this->sessionValues[$key]); if ($reopened) { $this->close(); @@ -167,6 +201,16 @@ 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, + 'backingSessionClass' => get_class($this->session), + ]); + } $this->sessionValues = []; if ($requesttoken !== null) { $this->set('requesttoken', $requesttoken); @@ -194,6 +238,18 @@ 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, + 'backingSessionClass' => get_class($this->session), + ]); + } $this->session->regenerateId($deleteOldSession, $updateToken); } 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()); } }