From a67c85885e101e3e144b784606c8ce33d0fcc187 Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Tue, 27 Oct 2026 22:27:03 +0100 Subject: [PATCH] @ZaphodB fix(security): two-factor-auth could be switched off with only a session With 2-step verification enabled, these plugin actions needed nothing but a session (and the CSRF token that comes with it): - EnableTwoFactor with Enable=0 switched it off, - ClearTwoFactorInfo deleted it, - CreateTwoFactorSecret overwrote the secret with Enable false, i.e. also switched it off, - ShowTwoFactorSecret returned the secret and QR code. A stolen cookie or an unlocked machine was enough to drop or copy the second factor. While 2FA is enabled, all four now require a current TOTP or backup code, checked as at login; a backup code used here is spent. Enabling and the first setup are unchanged. Backup codes come from random_int() instead of rand(). The settings page asks for the code (the existing "Verification Code" label) before those actions and sends it. A refused change reloads the real state; before, any error made the page show 2FA as not configured. A cancelled or refused "off" puts the checkbox back. TwoFactorAuthSettings.min.js, which is what is served, is now the same file as the source rather than a separate hand-minified copy. test/two-factor-reauth.php drives the real gate with storage and provider stubbed: open while not enabled; refused with no, a wrong or an unknown backup code while enabled; allowed with a valid TOTP or backup code, which is spent. --- plugins/two-factor-auth/index.php | 42 ++++++++++- .../js/TwoFactorAuthSettings.js | 59 ++++++++++++--- test/two-factor-reauth.php | 75 +++++++++++++++++++ 3 files changed, 164 insertions(+), 12 deletions(-) create mode 100644 test/two-factor-reauth.php diff --git a/plugins/two-factor-auth/index.php b/plugins/two-factor-auth/index.php index a080ae08b..515fea252 100644 --- a/plugins/two-factor-auth/index.php +++ b/plugins/two-factor-auth/index.php @@ -88,6 +88,34 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin } } + /** + * Turning 2-step verification off, wiping it, replacing its secret or showing + * the secret all used to need only the session. Whoever held a session (a + * stolen cookie, an unlocked machine) could quietly drop the second factor or + * copy it. While it is enabled, these now need a current code or backup code, + * as login does. A used backup code is spent. + */ + private function requireCurrentCode(MainAccount $oAccount) : void + { + $aData = $this->getTwoFactorInfo($oAccount); + if (empty($aData['IsSet']) || empty($aData['Enable']) || empty($aData['Secret'])) { + return; + } + $sCode = \trim($this->jsonParam('Code', '')); + if (\strlen($sCode)) { + if ($this->TwoFactorAuthProvider($oAccount)->VerifyCode($aData['Secret'], $sCode)) { + return; + } + $aBackupCodes = \explode(' ', \trim(\preg_replace('/[^\d]+/', ' ', $aData['BackupCodes'] ?? ''))); + if (6 < \strlen($sCode) && \in_array($sCode, $aBackupCodes, true)) { + $this->removeBackupCodeFromTwoFactorInfo($oAccount, $sCode); + return; + } + } + $this->Logger()->Write("TFA: change refused for {$oAccount->Email()}, no valid current code"); + throw new ClientException(\RainLoop\Notifications::AuthError); + } + public function DoGetTwoFactorInfo() : array { $oAccount = $this->getMainAccountFromToken(); @@ -107,11 +135,15 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + // Overwrites the stored secret with Enable false, i.e. also switches it off + $this->requireCurrentCode($oAccount); + $sEmail = $oAccount->Email(); $sSecret = $this->TwoFactorAuthProvider($oAccount)->CreateSecret(); - $aCodes = \array_map(function(){return \rand(100000000, 900000000);}, \array_fill(0, 8, null)); + // Backup codes are a second factor; rand() is not a CSPRNG + $aCodes = \array_map(fn() => \random_int(100000000, 899999999), \array_fill(0, 8, null)); $this->StorageProvider()->Put($oAccount, \RainLoop\Providers\Storage\Enumerations\StorageType::CONFIG, @@ -148,6 +180,8 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + $this->requireCurrentCode($oAccount); + $aResult = $this->getTwoFactorInfo($oAccount); unset($aResult['BackupCodes']); @@ -164,6 +198,10 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + if ('1' !== \trim($this->jsonParam('Enable', '0'))) { + $this->requireCurrentCode($oAccount); + } + $oActions = $this->Manager()->Actions(); if ($oActions->HasActionParam('EnableTwoFactor')) { $sValue = $oActions->GetActionParam('EnableTwoFactor', ''); @@ -215,6 +253,8 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + $this->requireCurrentCode($oAccount); + $this->StorageProvider()->Clear($oAccount, \RainLoop\Providers\Storage\Enumerations\StorageType::CONFIG, 'two_factor' diff --git a/plugins/two-factor-auth/js/TwoFactorAuthSettings.js b/plugins/two-factor-auth/js/TwoFactorAuthSettings.js index 79d8c6bb5..c85940d37 100644 --- a/plugins/two-factor-auth/js/TwoFactorAuthSettings.js +++ b/plugins/two-factor-auth/js/TwoFactorAuthSettings.js @@ -22,8 +22,9 @@ const * @param {?Function} fCallback * @param {boolean} bEnable */ - enableTwoFactor(fCallback, bEnable) { + enableTwoFactor(fCallback, bEnable, oParams) { rl.pluginRemoteRequest(fCallback, 'EnableTwoFactor', { + ...oParams, Enable: bEnable ? 1 : 0 }); } @@ -61,8 +62,12 @@ class TwoFactorAuthSettings rl.settings.get('RequireTwoFactor') && rl.settings.set('SetupTwoFactor', !!iError); }, value); } else { - value || this.viewEnable_(value); - Remote.enableTwoFactor(fn, false); + // Turning it off needs a current code while it is on. + // Cancelled or refused, the checkbox goes back to on. + this.withCode(params => { + value || this.viewEnable_(value); + Remote.enableTwoFactor(iError => iError && this.viewEnable_(true), false, params); + }, () => this.viewEnable_.valueHasMutated()); } } }, @@ -86,12 +91,28 @@ class TwoFactorAuthSettings }).forEach(([key, fn]) => this[key] = ko.computed(fn)); this.onResult = this.onResult.bind(this); + this.onChangeResult = this.onChangeResult.bind(this); this.onShowSecretResult = this.onShowSecretResult.bind(this); } + /** + * While 2-step verification is on, the server wants a current code (or a + * backup code) to turn it off, clear it, replace it or show its secret. + */ + withCode(fDo, fCancel) { + if (!this.viewEnable_()) { + fDo({}); + } else { + const code = (prompt(rl.i18n('PLUGIN_2FA/LABEL_TWO_FACTOR_CODE')) || '').trim(); + code ? fDo({ Code: code }) : fCancel?.(); + } + } + showSecret() { - this.secreting(true); - rl.pluginRemoteRequest(this.onShowSecretResult, 'ShowTwoFactorSecret'); + this.withCode(params => { + this.secreting(true); + rl.pluginRemoteRequest(this.onShowSecretResult, 'ShowTwoFactorSecret', params); + }); } hideSecret() { @@ -101,8 +122,10 @@ class TwoFactorAuthSettings } createTwoFactor() { - this.processing(true); - rl.pluginRemoteRequest(this.onResult, 'CreateTwoFactorSecret'); + this.withCode(params => { + this.processing(true); + rl.pluginRemoteRequest(this.onChangeResult, 'CreateTwoFactorSecret', params); + }); } testTwoFactor() { @@ -115,12 +138,26 @@ class TwoFactorAuthSettings } clearTwoFactor() { - this.hideSecret(); + this.withCode(params => { + this.hideSecret(); - this.twoFactorTested(false); + this.twoFactorTested(false); - this.clearing(true); - rl.pluginRemoteRequest(this.onResult, 'ClearTwoFactorInfo'); + this.clearing(true); + rl.pluginRemoteRequest(this.onChangeResult, 'ClearTwoFactorInfo', params); + }); + } + + // A refused change must not show the account as unconfigured, which is what + // onResult does with an error; reload the real state instead. + onChangeResult(iError, oData) { + if (iError) { + this.processing(false); + this.clearing(false); + this.onBuild(); + } else { + this.onResult(iError, oData); + } } onShow() { diff --git a/test/two-factor-reauth.php b/test/two-factor-reauth.php new file mode 100644 index 000000000..cd9d5c826 --- /dev/null +++ b/test/two-factor-reauth.php @@ -0,0 +1,75 @@ +info; } + protected function TwoFactorAuthProvider(\RainLoop\Model\MainAccount $oAccount) : ?\TwoFactorAuthInterface + { + return new class implements \TwoFactorAuthInterface { + public function Label() : string { return 'test'; } + public function VerifyCode(string $sSecret, string $sCode) : bool { return 'SECRET' === $sSecret && '123456' === $sCode; } + public function CreateSecret() : string { return 'SECRET'; } + }; + } + protected function Logger() : \MailSo\Log\Logger { return new \MailSo\Log\Logger(false); } + protected function removeBackupCodeFromTwoFactorInfo(\RainLoop\Model\MainAccount $oAccount, string $sCode) : bool { $this->spent[] = $sCode; return true; } +} + +$plugin = (new ReflectionClass(TestTwoFactor::class))->newInstanceWithoutConstructor(); +$actions = (new ReflectionClass(\RainLoop\Actions::class))->newInstanceWithoutConstructor(); +$manager = (new ReflectionClass(\RainLoop\Plugins\Manager::class))->newInstanceWithoutConstructor(); +(new ReflectionProperty(\RainLoop\Plugins\Manager::class, 'oActions'))->setValue($manager, $actions); +(new ReflectionProperty(\RainLoop\Plugins\AbstractPlugin::class, 'oPluginManager'))->setValue($plugin, $manager); +$account = (new ReflectionClass(\RainLoop\Model\MainAccount::class))->newInstanceWithoutConstructor(); +$gate = (new ReflectionMethod(TwoFactorAuthPlugin::class, 'requireCurrentCode')); + +$params = new ReflectionProperty(\RainLoop\Actions::class, 'aCurrentActionParams'); +$allowed = function (array $values) use ($plugin, $actions, $account, $gate, $params) : bool { + $params->setValue($actions, $values); + try { + $gate->invoke($plugin, $account); + return true; + } catch (\RainLoop\Exceptions\ClientException $e) { + return false; + } +}; + +// Not set up, or set up but not enabled: nothing to protect yet. +$plugin->info = ['IsSet' => false, 'Enable' => false, 'Secret' => '']; +check($allowed([]), 'blocked although 2FA is not set up'); +$plugin->info = ['IsSet' => true, 'Enable' => false, 'Secret' => 'SECRET', 'BackupCodes' => '111111111']; +check($allowed([]), 'blocked although 2FA is not enabled'); + +// Enabled: a valid TOTP or backup code is required. +$plugin->info = ['IsSet' => true, 'Enable' => true, 'Secret' => 'SECRET', 'BackupCodes' => '111111111 222222222']; +check(!$allowed([]), 'change allowed with no code while enabled'); +check(!$allowed(['Code' => '000000']), 'change allowed with a wrong code'); +check(!$allowed(['Code' => '33333333']), 'change allowed with an unknown backup code'); +check($allowed(['Code' => '123456']), 'valid TOTP code refused'); +check($allowed(['Code' => '222222222']), 'valid backup code refused'); +check(['222222222'] === $plugin->spent, 'used backup code was not spent'); + +// Backup codes come from random_int now, not rand(). +$source = file_get_contents(dirname(__DIR__).'/plugins/two-factor-auth/index.php'); +check(!preg_match('/\\\\rand\(/', $source) && str_contains($source, '\\random_int('), 'backup codes not from random_int()'); + +echo "two-factor-reauth: ok\n";