From 215d2ec1084dc3855e0cd964ea80fd9689f37276 Mon Sep 17 00:00:00 2001 From: Johannes Rauh Date: Fri, 8 May 2026 10:49:16 +0200 Subject: [PATCH] Avoid storing full remember-me cookie in session during 2FA challenge Previously the `RememberMe` object (containing the AES-encrypted password and the decryption key) was serialized directly into the PHP session while waiting for the user to complete the 2FA challenge. Because PHP sessions are written to disk in plaintext, this exposed the key and ciphertext in the same place, sufficient to recover the user's password. Fix by splitting the secret across the session and the database, mirroring the design of the normal (non-2FA) remember-me flow: - Call `persist()` at login time so the AES key goes to the database immediately, never touching the session. - Store only the cookie value string (ciphertext + IV) in the session. The ciphertext is not exploitable without the key. - After a successful 2FA challenge, reconstruct the `RememberMe` object via a new `RememberMe::fromCookieData()` factory that does a DB lookup by IV and restores the canonical expiry from the database row. - Only then send the browser cookie, so the cookie never reaches the browser unless the second factor was verified. Canceled challenges remove the created DB row, while abandoned challenges leave an orphaned DB row which is cleaned up by the existing `RememberMe::removeExpired()` mechanism. `RememberMe::fromCookieData()` sets `$expiresAt` from the database row so the browser cookie issued after 2FA inherits the expiry stored at login time rather than receiving a fresh 30-day window computed at challenge-completion time. The renewal path in `AuthenticationController::loginAction()` is unaffected, because `renew()` constructs a new object via `fromCredentials()`. --- .../controllers/AuthenticationController.php | 4 ++++ .../forms/Authentication/LoginForm.php | 3 ++- .../Authentication/TwoFactorChallengeForm.php | 5 ++-- .../Icinga/Authentication/TwoFactorState.php | 23 +++++++++---------- library/Icinga/Web/RememberMe.php | 17 ++++++++++++-- 5 files changed, 35 insertions(+), 17 deletions(-) diff --git a/application/controllers/AuthenticationController.php b/application/controllers/AuthenticationController.php index 1ce954877..6ea836331 100644 --- a/application/controllers/AuthenticationController.php +++ b/application/controllers/AuthenticationController.php @@ -203,6 +203,10 @@ class AuthenticationController extends CompatController $form->getPressedSubmitElement()?->getName() === TwoFactorChallengeForm::SUBMIT_CANCEL; if ($csrfValid && $cancelPressed) { + // The login flow may have persisted a remember-me record before issuing the + // challenge. Remove it now since the challenge was canceled and the cookie + // was never delivered to the browser. + RememberMe::removeAllByUsername((new TwoFactorState())->getChallengedUser()->getUsername()); Session::getSession()->purge(); $this->redirectNow($this->withRedirect('authentication/login')); } diff --git a/application/forms/Authentication/LoginForm.php b/application/forms/Authentication/LoginForm.php index ae41a0062..415f376e0 100644 --- a/application/forms/Authentication/LoginForm.php +++ b/application/forms/Authentication/LoginForm.php @@ -138,7 +138,8 @@ class LoginForm extends CompatForm if ($this->getElement('rememberme')->isChecked()) { $rememberMe = RememberMe::fromCredentials($user->getUsername(), $password); - $twoFactorState->setRememberMeCookie($rememberMe); + $twoFactorState->setRememberMeCookieData($rememberMe->getCookie()->getValue()); + $rememberMe->persist(); } $redirectUrl = Url::fromPath('authentication/twofactor'); diff --git a/application/forms/Authentication/TwoFactorChallengeForm.php b/application/forms/Authentication/TwoFactorChallengeForm.php index 06cb2986f..1c45bcb0d 100644 --- a/application/forms/Authentication/TwoFactorChallengeForm.php +++ b/application/forms/Authentication/TwoFactorChallengeForm.php @@ -13,6 +13,7 @@ use Icinga\Application\Logger; use Icinga\Authentication\Auth; use Icinga\Authentication\TwoFactorState; use Icinga\Web\Form\Element\LoginRedirect; +use Icinga\Web\RememberMe; use Icinga\Web\Session; use Icinga\Web\Url; use ipl\Html\Attributes; @@ -126,10 +127,10 @@ class TwoFactorChallengeForm extends CompatForm $response = Icinga::app()->getResponse(); - if ($rememberMe = $twoFactorState->getRememberMeCookie()) { + if ($cookieData = $twoFactorState->getRememberMeCookieData()) { try { + $rememberMe = RememberMe::fromCookieData($cookieData); $response->setCookie($rememberMe->getCookie()); - $rememberMe->persist(); } catch (Exception $e) { Logger::error('Failed to let user "%s" stay logged in: %s', $user->getUsername(), $e); } diff --git a/library/Icinga/Authentication/TwoFactorState.php b/library/Icinga/Authentication/TwoFactorState.php index 0f3bee18e..4d8f9671b 100644 --- a/library/Icinga/Authentication/TwoFactorState.php +++ b/library/Icinga/Authentication/TwoFactorState.php @@ -6,7 +6,6 @@ namespace Icinga\Authentication; use Icinga\User; -use Icinga\Web\RememberMe; use Icinga\Web\Session; use Icinga\Web\Session\SessionNamespace; @@ -21,8 +20,8 @@ class TwoFactorState /** @var string Session key storing the challenged User */ protected const SESSION_CHALLENGED_USER = 'challenged_user'; - /** @var string Session key storing the temporary {@see RememberMe} instance */ - protected const SESSION_REMEMBER_ME_COOKIE = 'remember_me_cookie'; + /** @var string Session key storing the raw remember-me cookie */ + protected const SESSION_REMEMBER_ME_COOKIE_DATA = 'remember_me_cookie_data'; /** @var SessionNamespace Session namespace scoping the challenge state */ protected SessionNamespace $session; @@ -57,7 +56,7 @@ class TwoFactorState public function completeChallenge(): void { $this->session->delete(static::SESSION_CHALLENGED_USER); - $this->session->delete(static::SESSION_REMEMBER_ME_COOKIE); + $this->session->delete(static::SESSION_REMEMBER_ME_COOKIE_DATA); } /** @@ -71,25 +70,25 @@ class TwoFactorState } /** - * Set the remember-me instance to issue after a successful challenge + * Set the remember-me cookie value to issue after a successful challenge * - * @param RememberMe $cookie Temporary remember-me instance + * @param string $cookieData * * @return void */ - public function setRememberMeCookie(RememberMe $cookie): void + public function setRememberMeCookieData(string $cookieData): void { - $this->session->set(static::SESSION_REMEMBER_ME_COOKIE, $cookie); + $this->session->set(static::SESSION_REMEMBER_ME_COOKIE_DATA, $cookieData); } /** - * Get the stored remember-me instance + * Get the stored remember-me cookie value * - * @return ?RememberMe + * @return ?string null if no value was set */ - public function getRememberMeCookie(): ?RememberMe + public function getRememberMeCookieData(): ?string { - return $this->session->get(static::SESSION_REMEMBER_ME_COOKIE); + return $this->session->get(static::SESSION_REMEMBER_ME_COOKIE_DATA); } /** diff --git a/library/Icinga/Web/RememberMe.php b/library/Icinga/Web/RememberMe.php index b425b906f..095bd9745 100644 --- a/library/Icinga/Web/RememberMe.php +++ b/library/Icinga/Web/RememberMe.php @@ -98,9 +98,21 @@ class RememberMe * * @return static */ - public static function fromCookie() + public static function fromCookie(): static { - $data = explode('|', $_COOKIE[static::COOKIE]); + return static::fromCookieData($_COOKIE[static::COOKIE]); + } + + /** + * Create the remember me component from cookie data + * + * @param string $data The cookie data + * + * @return static + */ + public static function fromCookieData(string $data): static + { + $data = explode('|', $data); $iv = base64_decode(array_pop($data)); $select = (new Select()) @@ -135,6 +147,7 @@ class RememberMe $rememberMe->username = $rs->username; $rememberMe->encryptedPassword = $data[0]; + $rememberMe->expiresAt = strtotime($rs->expires_at); return $rememberMe; }