MDL-79678 tool_mfa: Adapt to OTPHP changes in v11

With the OTPHP upgrade from v10.x to v11.x, the behaviour of the window
feature changed substantially. With version 10, the window of timestamps
goes from `timestamp - window * period` to `timestamp + window *
period`. For example, if the window is 5, the period 30 and the
timestamp 1476822000, the OTP tested are within 1476821850 (`1476822000
- 5 * 30`) and 1476822150 (`1476822000 + 5 * 30`). In other words, this
validated the 5 OTP before and after the current timestamp. With version
11, the TOTP window acts as a time drift. If the window is 15, the
period 30, and the current timestamp is 147682209, the OTP tested are
within 147682194 (`147682209 - 15`), 147682209 and 147682224 (`147682209
+ 15`). The window shall be lower than the period. Therefore, this test
includes the previous OTP but not the next one.

This change required an adaption to align our implementation with OTPHP.
The window of valid TOTP tokens is now much narrower. This change in
functionality is a security improvement, but it also means that the time
on the device generating the TOTP token must be more accurate.  As OTPHP
restricts the window to be strictly lower than the period, our admin
setting now has a maximum allowed value of 29. To ensure we only have
valid window values, we need to update the admin setting to a value
lower than 30; therefore, we include an upgrade step.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>

admin/tool/mfa/factor/totp/classes/factor.php

@@ -29,6 +29,7 @@ require_once(__DIR__.'/../extlib/ParagonIE/ConstantTime/EncoderInterface.php');
 require_once(__DIR__.'/../extlib/ParagonIE/ConstantTime/Binary.php');
 require_once(__DIR__.'/../extlib/ParagonIE/ConstantTime/Base32.php');
 
+use MoodleQuickForm;
 use tool_mfa\local\factor\object_factor_base;
 use OTPHP\TOTP;
 use stdClass;
@@ -116,10 +117,10 @@ class factor extends object_factor_base {
     /**
      * TOTP Factor implementation.
      *
-     * @param \MoodleQuickForm $mform
-     * @return \MoodleQuickForm $mform
+     * @param MoodleQuickForm $mform
+     * @return MoodleQuickForm $mform
      */
-    public function setup_factor_form_definition(\MoodleQuickForm $mform): \MoodleQuickForm {
+    public function setup_factor_form_definition(MoodleQuickForm $mform): MoodleQuickForm {
         $secret = $this->generate_secret_code();
         $mform->addElement('hidden', 'secret', $secret);
         $mform->setType('secret', PARAM_ALPHANUM);
@@ -130,10 +131,10 @@ class factor extends object_factor_base {
     /**
      * TOTP Factor implementation.
      *
-     * @param \MoodleQuickForm $mform
-     * @return \MoodleQuickForm $mform
+     * @param MoodleQuickForm $mform
+     * @return MoodleQuickForm $mform
      */
-    public function setup_factor_form_definition_after_data(\MoodleQuickForm $mform): \MoodleQuickForm {
+    public function setup_factor_form_definition_after_data(MoodleQuickForm $mform): MoodleQuickForm {
         global $OUTPUT, $SITE, $USER;
 
         // Array of elements to allow XSS.
@@ -242,10 +243,10 @@ class factor extends object_factor_base {
     /**
      * TOTP Factor implementation.
      *
-     * @param \MoodleQuickForm $mform
-     * @return \MoodleQuickForm $mform
+     * @param MoodleQuickForm $mform
+     * @return MoodleQuickForm $mform
      */
-    public function login_form_definition(\MoodleQuickForm $mform): \MoodleQuickForm {
+    public function login_form_definition(MoodleQuickForm $mform): MoodleQuickForm {
 
         $mform->disable_form_change_checker();
         $mform->addElement(new \tool_mfa\local\form\verification_field());
@@ -264,12 +265,10 @@ class factor extends object_factor_base {
         global $USER;
         $factors = $this->get_active_user_factors($USER);
         $result = ['verificationcode' => get_string('error:wrongverification', 'factor_totp')];
-        $windowconfig = get_config('factor_totp', 'window');
+        $window = get_config('factor_totp', 'window');
 
         foreach ($factors as $factor) {
             $totp = TOTP::create($factor->secret);
-            // Convert seconds to windows.
-            $window = (int) floor($windowconfig / $totp->getPeriod());
             $factorresult = $this->validate_code($data['verificationcode'], $window, $totp, $factor);
             $time = userdate(time(), get_string('systimeformat', 'factor_totp'));
 
@@ -311,23 +310,27 @@ class factor extends object_factor_base {
             return self::TOTP_USED;
         }
 
-        // The window in which to check for clock skew, 5 increments past valid window.
-        $skewwindow = $window + 5;
-        $pasttimestamp = time() - ($skewwindow * $totp->getPeriod());
-        $futuretimestamp = time() + ($skewwindow * $totp->getPeriod());
-
+        // Check if the code is valid, returning early.
         if ($totp->verify($code, time(), $window)) {
             return self::TOTP_VALID;
-        } else if ($totp->verify($code, $pasttimestamp, $skewwindow)) {
-            // Check for clock skew in the past 10 periods.
-            return self::TOTP_OLD;
-        } else if ($totp->verify($code, $futuretimestamp, $skewwindow)) {
-            // Check for clock skew in the future 10 periods.
-            return self::TOTP_FUTURE;
-        } else {
-            // In all other cases, code is invalid.
-            return self::TOTP_INVALID;
         }
+
+        // Check for clock skew in the past and future 10 periods.
+        for ($i = 1; $i <= 10; $i++) {
+            $pasttimestamp = time() - $i * $totp->getPeriod();
+            $futuretimestamp = time() + $i * $totp->getPeriod();
+
+            if ($totp->verify($code, $pasttimestamp, $window)) {
+                return self::TOTP_OLD;
+            }
+
+            if ($totp->verify($code, $futuretimestamp, $window)) {
+                return self::TOTP_FUTURE;
+            }
+        }
+
+        // In all other cases, the code is invalid.
+        return self::TOTP_INVALID;
     }
 
     /**

admin/tool/mfa/factor/totp/db/upgrade.php

@@ -0,0 +1,43 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * factor_totp upgrade library.
+ *
+ * @package    factor_totp
+ * @copyright  2024 Daniel Ziegenberg <daniel@ziegenberg.at>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+/**
+ * Factor totp upgrade helper function
+ *
+ * @param int $oldversion
+ */
+function xmldb_factor_totp_upgrade($oldversion): bool {
+    if ($oldversion < 2024081600) {
+
+        $window = get_config('factor_totp', 'window');
+        if ($window && $window >= 30) {
+            set_config('window', 29, 'factor_totp');
+        }
+
+        // Savepoint reached.
+        upgrade_plugin_savepoint(true, 2024081600, 'factor', 'auth');
+    }
+
+    return true;
+}

admin/tool/mfa/factor/totp/lang/en/factor_totp.php

@@ -54,8 +54,11 @@ $string['revokefactorconfirmation'] = 'Remove \'{$a}\' authenticator app?';
 $string['settings:totplink'] = 'Show mobile app setup link';
 $string['settings:totplink_help'] = 'If enabled the user will see a 3rd setup option with a direct otpauth:// link';
 $string['settings:window'] = 'TOTP verification window';
-$string['settings:window_help'] = 'How long each code is valid for. You can set this to a higher value as a workaround if your users device clocks are often slightly wrong.
-Rounded down to the nearest 30 seconds, which is the time between new generated codes.';
+$string['settings:window_help'] = 'The window of TOTP acts as time drift and specifies how long each code is valid for.
+    The period, which is the time between newly generated codes, is 30 seconds.
+    If the window is 15 (the default) and the current timestamp is 147682209, the OTP tested are within 147682194 (147682209 - 15), 147682209 and 147682224 (147682209 + 15).
+    The window shall be lower than 30. Therefore, this test includes the previous OTP but not the next one.
+    You can set this to a higher value (up to 29) as a workaround if your user\'s device clocks are often slightly wrong.';
 $string['setupfactor'] = 'Set up authenticator app';
 $string['setupfactorbutton'] = 'Set up';
 $string['setupfactor:account'] = 'Account:';

admin/tool/mfa/factor/totp/settings.php

@@ -38,9 +38,11 @@ $settings->add(new admin_setting_configtext('factor_totp/weight',
     new lang_string('settings:weight', 'tool_mfa'),
     new lang_string('settings:weight_help', 'tool_mfa'), 100, PARAM_INT));
 
-$settings->add(new admin_setting_configduration('factor_totp/window',
+$window = new admin_setting_configduration('factor_totp/window',
     new lang_string('settings:window', 'factor_totp'),
-    new lang_string('settings:window_help', 'factor_totp'), 30));
+    new lang_string('settings:window_help', 'factor_totp'), 15);
+$window->set_max_duration(29);
+$settings->add($window);
 
 $settings->add(new admin_setting_configcheckbox('factor_totp/totplink',
     new lang_string('settings:totplink', 'factor_totp'),

admin/tool/mfa/factor/totp/tests/factor_test.php

@@ -50,7 +50,7 @@ class factor_test extends \advanced_testcase {
         $this->setUser($user);
         // Setup test staples.
         $totp = \OTPHP\TOTP::create('fakekey');
-        $window = 10;
+        $window = 29;
 
         set_config('enabled', 1, 'factor_totp');
         $totpfactor = \tool_mfa\plugininfo\factor::get_factor('totp');
@@ -67,21 +67,15 @@ class factor_test extends \advanced_testcase {
         $result = $totpfactor->validate_code($code, $window, $totp, $factorinstance);
         $this->assertEquals($totpfactor::TOTP_VALID, $result);
 
-        // Now update timeverified to 2 mins ago, and check codes within window are blocked.
-        $code = $totp->at(time() - (2 * MINSECS));
-        $DB->set_field('tool_mfa', 'lastverified', time() - (2 * MINSECS), ['id' => $factorinstance->id]);
+        // Now update timeverified to 45 seconds ago, and check codes within window is blocked.
+        $code = $totp->at(time() - (20));
+        $DB->set_field('tool_mfa', 'lastverified', time() - (20), ['id' => $factorinstance->id]);
         $result = $totpfactor->validate_code($code, $window, $totp, $factorinstance);
         $this->assertEquals($totpfactor::TOTP_USED, $result);
 
-        // Now update timeverified to 2 mins ago, and check codes within window are blocked.
+        // Now update timeverified to 45 seconds ago, and check code from current increment within window is blocked.
         $code = $totp->at(time());
-        $DB->set_field('tool_mfa', 'lastverified', time() - (2 * MINSECS), ['id' => $factorinstance->id]);
-        $result = $totpfactor->validate_code($code, $window, $totp, $factorinstance);
-        $this->assertEquals($totpfactor::TOTP_USED, $result);
-
-        // Now update timeverified to 2 mins ago, and check codes within window are blocked.
-        $code = $totp->at(time() - (4 * MINSECS));
-        $DB->set_field('tool_mfa', 'lastverified', time() - (2 * MINSECS), ['id' => $factorinstance->id]);
+        $DB->set_field('tool_mfa', 'lastverified', time() - (20), ['id' => $factorinstance->id]);
         $result = $totpfactor->validate_code($code, $window, $totp, $factorinstance);
         $this->assertEquals($totpfactor::TOTP_USED, $result);
 

admin/tool/mfa/factor/totp/version.php

@@ -26,7 +26,7 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version      = 2024042200;      // The current plugin version (Date: YYYYMMDDXX).
-$plugin->requires     = 2024041600;   // Requires this Moodle version.
+$plugin->version      = 2024081600;      // The current plugin version (Date: YYYYMMDDXX).
+$plugin->requires     = 2024041600;      // Requires this Moodle version.
 $plugin->component    = 'factor_totp';   // Full name of the plugin (used for diagnostics).
 $plugin->maturity     = MATURITY_STABLE;