When accepting a TOTP response, require it respond explicitly to a specific challenge

Summary:
Depends on D19890. Ref T13222. See PHI873. Currently, we only validate TOTP responses against the current (realtime) timestep. Instead, also validate them against a specific challenge.

This mostly just moves us toward more specifically preventing responses from being reused, and supporting flows which must look more like this (SMS/push).

One rough edge here is that during the T+3 and T+4 windows (you request a prompt, then wait 60-120 seconds to respond) only past responses actually work (the current code on your device won't). For example:

  - At T+0, you request MFA. We issue a T+0 challenge that accepts codes T-2, T-1, T+0, T+1, and T+2. The challenge locks out T+3 and T+4 to prevent the window from overlapping with the next challenge we may issue (see D19890).
  - If you wait 60 seconds until T+3 to actually submit a code, the realtime valid responses are T+1, T+2, T+3, T+4, T+5. The challenge valid responses are T-2, T-1, T+0, T+1, and T+2. Only T+1 and T+2 are in the intersection. Your device is showing T+3 if the clock is right, so if you type in what's shown on your device it won't be accepted.
  - This //may// get refined in future changes, but, in the worst case, it's probably fine if it doesn't. Beyond 120s you'll get a new challenge and a full [-2, ..., +2] window to respond, so this lockout is temporary even if you manage to hit it.
  - If this //doesn't// get refined, I'll change the UI to say "This factor recently issued a challenge which has expired, wait N seconds." to smooth this over a bit.

Test Plan:
  - Went through MFA.
  - Added a new TOTP factor.
  - Hit some error cases on purpose.
  - Tried to use an old code a moment after it expired, got rejected.
  - Waited 60+ seconds, tried to use the current displayed factor, got rejected (this isn't great, but currently expected).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19893

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -77,10 +77,10 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
 
     $e_code = true;
     if ($request->getExists('totp')) {
-      $okay = $this->verifyTOTPCode(
+      $okay = (bool)$this->getTimestepAtWhichResponseIsValid(
-        $user,
+        $this->getAllowedTimesteps($this->getCurrentTimestep()),
         new PhutilOpaqueEnvelope($key),
-        $code);
+        (string)$code);
 
       if ($okay) {
         $config = $this->newConfigForUser($user)
@@ -240,6 +240,8 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     $engine = $config->getSessionEngine();
     $workflow_key = $engine->getWorkflowKey();
 
+    $current_timestep = $this->getCurrentTimestep();
+
     foreach ($challenges as $challenge) {
       $challenge_timestep = (int)$challenge->getChallengeKey();
       $wait_duration = ($challenge->getChallengeTTL() - $now) + 1;
@@ -265,6 +267,22 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
               'again.',
               new PhutilNumber($wait_duration)));
       }
+
+      // If the current realtime timestep isn't a valid response to the current
+      // challenge but the challenge hasn't expired yet, we're locking out
+      // the factor to prevent challenge windows from overlapping. Let the user
+      // know that they should wait for a new challenge.
+      $challenge_timesteps = $this->getAllowedTimesteps($challenge_timestep);
+      if (!isset($challenge_timesteps[$current_timestep])) {
+        return $this->newResult()
+          ->setIsWait(true)
+          ->setErrorMessage(
+            pht(
+              'This factor recently issued a challenge which has expired. '.
+              'A new challenge can not be issued yet. Wait %s second(s) for '.
+              'the code to cycle, then try again.',
+              new PhutilNumber($wait_duration)));
+      }
     }
 
     return null;
@@ -277,12 +295,36 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     array $challenges) {
 
     $code = $request->getStr($this->getParameterName($config, 'totpcode'));
-    $key = new PhutilOpaqueEnvelope($config->getFactorSecret());
 
     $result = $this->newResult()
       ->setValue($code);
 
-    if ($this->verifyTOTPCode($viewer, $key, (string)$code)) {
+    // We expect to reach TOTP validation with exactly one valid challenge.
+    if (count($challenges) !== 1) {
+      throw new Exception(
+        pht(
+          'Reached TOTP challenge validation with an unexpected number of '.
+          'unexpired challenges (%d), expected exactly one.',
+          phutil_count($challenges)));
+    }
+
+    $challenge = head($challenges);
+
+    $challenge_timestep = (int)$challenge->getChallengeKey();
+    $current_timestep = $this->getCurrentTimestep();
+
+    $challenge_timesteps = $this->getAllowedTimesteps($challenge_timestep);
+    $current_timesteps = $this->getAllowedTimesteps($current_timestep);
+
+    // We require responses be both valid for the challenge and valid for the
+    // current timestep. A longer challenge TTL doesn't let you use older
+    // codes for a longer period of time.
+    $valid_timestep = $this->getTimestepAtWhichResponseIsValid(
+      array_intersect_key($challenge_timesteps, $current_timesteps),
+      new PhutilOpaqueEnvelope($config->getFactorSecret()),
+      (string)$code);
+
+    if ($valid_timestep) {
       $result->setIsValid(true);
     } else {
       if (strlen($code)) {
@@ -300,29 +342,6 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     return strtoupper(Filesystem::readRandomCharacters(32));
   }
 
-  private function verifyTOTPCode(
-    PhabricatorUser $user,
-    PhutilOpaqueEnvelope $key,
-    $code) {
-
-    $now = (int)(time() / 30);
-
-    // Allow the user to enter a code a few minutes away on either side, in
-    // case the server or client has some clock skew.
-    for ($offset = -2; $offset <= 2; $offset++) {
-      $real = self::getTOTPCode($key, $now + $offset);
-      if (phutil_hashes_are_identical($real, $code)) {
-        return true;
-      }
-    }
-
-    // TODO: After validating a code, this should mark it as used and prevent
-    // it from being reused.
-
-    return false;
-  }
-
-
   public static function base32Decode($buf) {
     $buf = strtoupper($buf);
 
@@ -424,15 +443,34 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     return (int)(PhabricatorTime::getNow() / $duration);
   }
 
-  private function getAllowedTimesteps() {
+  private function getAllowedTimesteps($at_timestep) {
-    $current_step = $this->getCurrentTimestep();
     $window = $this->getTimestepWindowSize();
-    return range($current_step - $window, $current_step + $window);
+    $range = range($at_timestep - $window, $at_timestep + $window);
+    return array_fuse($range);
   }
 
   private function getTimestepWindowSize() {
+    // The user is allowed to provide a code from the recent past or the
+    // near future to account for minor clock skew between the client
+    // and server, and the time it takes to actually enter a code.
     return 2;
   }
 
+  private function getTimestepAtWhichResponseIsValid(
+    array $timesteps,
+    PhutilOpaqueEnvelope $key,
+    $code) {
+
+    foreach ($timesteps as $timestep) {
+      $expect_code = self::getTOTPCode($key, $timestep);
+      if (phutil_hashes_are_identical($code, $expect_code)) {
+        return $timestep;
+      }
+    }
+
+    return null;
+  }
+
+
 
 }