diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 6a5115c94cda4..2d27a771bd299 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -56,6 +56,7 @@ use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; use OCP\Session\Exceptions\SessionNotAvailableException; +use function OCP\Log\logger; class ClientFlowLoginController extends Controller { private IUserSession $userSession; @@ -107,9 +108,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 { @@ -157,11 +173,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 ef16cfbd04bf5..cd5c9c39b6c74 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -46,6 +46,7 @@ use OCP\IUser; use OCP\IUserSession; use OCP\Security\ISecureRandom; +use function OCP\Log\logger; class ClientFlowLoginV2Controller extends Controller { public const TOKEN_NAME = 'client.flow.v2.login.token'; @@ -124,12 +125,33 @@ public function showAuthPickerPage($user = ''): StandaloneTemplateResponse { 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', @@ -303,9 +325,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 1eb6987fc18f7..97d3fd78fd877 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -32,6 +32,7 @@ use OCP\ISession; use OCP\Security\ICrypto; use OCP\Session\Exceptions\SessionNotAvailableException; +use function OCP\Log\logger; /** * Class CryptoSessionData @@ -97,13 +98,34 @@ 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', [ + 'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1', + 'stateToken' => $value, + ]); + } $this->isModified = true; if ($reopened) { $this->close(); @@ -142,6 +164,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(); @@ -154,6 +184,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); diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index 87dd5ed60145c..6ed1d02f82188 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -36,6 +36,7 @@ use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Token\IProvider; use OCP\Session\Exceptions\SessionNotAvailableException; +use function OCP\Log\logger; /** * Class Internal @@ -221,6 +222,10 @@ private function invoke(string $functionName, array $parameters = [], bool $sile return call_user_func_array($functionName, $parameters); } } catch (\Error $e) { + logger('core')->error('trapping a session error', [ + 'loginFlow' => '?', + 'exception' => $e, + ]); $this->trapError($e->getCode(), $e->getMessage()); } }