@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

@@ -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'

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() {

test/two-factor-reauth.php

@@ -0,0 +1,75 @@
+<?php
+
+// Run with: php test/two-factor-reauth.php
+
+// Enable SnappyMail Api and include index file
+$_ENV['SNAPPYMAIL_INCLUDE_AS_API'] = true;
+require __DIR__ . '/../index.php';
+
+require dirname(__DIR__).'/plugins/two-factor-auth/index.php';
+require_once dirname(__DIR__).'/plugins/two-factor-auth/providers/interface.php';
+
+function check(bool $condition, string $message): void
+{
+	if (!$condition) {
+		throw new RuntimeException($message);
+	}
+}
+
+// Storage, provider and logger replaced; the gate itself is the real code.
+class TestTwoFactor extends TwoFactorAuthPlugin
+{
+	public array $info = [];
+	public array $spent = [];
+	protected function getTwoFactorInfo(\RainLoop\Model\MainAccount $oAccount, bool $bRemoveSecret = false) : array { return $this->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";