Track MFA "challenges" so we can bind challenges to sessions and support SMS and other push MFA

Summary:
Ref T13222. See PHI873. Ref T9770.

Currently, we support only TOTP MFA. For some MFA (SMS and "push-to-app"-style MFA) we may need to keep track of MFA details (e.g., the code we SMS'd you). There isn't much support for that yet.

We also currently allow free reuse of TOTP responses across sessions and workflows. This hypothetically enables some "spyglass" attacks where you look at someone's phone and type the code in before they do. T9770 discusses this in more detail, but is focused on an attack window starting when the user submits the form. I claim the attack window opens when the TOTP code is shown on their phone, and the window between the code being shown and being submitted is //much// more interesting than the window after it is submitted.

To address both of these cases, start tracking MFA "Challenges". These are basically a record that we asked you to give us MFA credentials.

For TOTP, the challenge binds a particular timestep to a given session, so an attacker can't look at your phone and type the code into their browser before (or after) you do -- they have a different session. For now, this means that codes are reusable in the same session, but that will be refined in the future.

For SMS / push, the "Challenge" would store the code we sent you so we could validate it.

This is mostly a step on the way toward one-shot MFA, ad-hoc MFA in comment action stacks, and figuring out what's going on with Duo.

Test Plan:
  - Passed MFA normally.
  - Passed MFA normally, simultaneously, as two different users.
  - With two different sessions for the same user:
    - Opened MFA in A, opened MFA in B. B got a "wait".
    - Submitted MFA in A.
    - Clicked "Wait" a bunch in B.
    - Submitted MFA in B when prompted.
  - Passed MFA normally, then passed MFA normally again with the same code in the same session. (This change does not prevent code reuse.)

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222, T9770

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

Author: epriestley

resources/sql/autopatches/20181213.auth.06.challenge.sql

@@ -0,0 +1,12 @@
+CREATE TABLE {$NAMESPACE}_auth.auth_challenge (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  phid VARBINARY(64) NOT NULL,
+  userPHID VARBINARY(64) NOT NULL,
+  factorPHID VARBINARY(64) NOT NULL,
+  sessionPHID VARBINARY(64) NOT NULL,
+  challengeKey VARCHAR(255) NOT NULL COLLATE {$COLLATE_TEXT},
+  challengeTTL INT UNSIGNED NOT NULL,
+  properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT},
+  dateCreated INT UNSIGNED NOT NULL,
+  dateModified INT UNSIGNED NOT NULL
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};

src/__phutil_library_map__.php

@@ -2187,6 +2187,9 @@
     'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php',
     'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php',
     'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php',
+    'PhabricatorAuthChallenge' => 'applications/auth/storage/PhabricatorAuthChallenge.php',
+    'PhabricatorAuthChallengePHIDType' => 'applications/auth/phid/PhabricatorAuthChallengePHIDType.php',
+    'PhabricatorAuthChallengeQuery' => 'applications/auth/query/PhabricatorAuthChallengeQuery.php',
     'PhabricatorAuthChangePasswordAction' => 'applications/auth/action/PhabricatorAuthChangePasswordAction.php',
     'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php',
     'PhabricatorAuthConduitTokenRevoker' => 'applications/auth/revoker/PhabricatorAuthConduitTokenRevoker.php',
@@ -7823,6 +7826,12 @@
     'PhabricatorAuthApplication' => 'PhabricatorApplication',
     'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthAuthProviderPHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorAuthChallenge' => array(
+      'PhabricatorAuthDAO',
+      'PhabricatorPolicyInterface',
+    ),
+    'PhabricatorAuthChallengePHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorAuthChallengeQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorAuthChangePasswordAction' => 'PhabricatorSystemAction',
     'PhabricatorAuthConduitAPIMethod' => 'ConduitAPIMethod',
     'PhabricatorAuthConduitTokenRevoker' => 'PhabricatorAuthRevoker',

src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php

@@ -29,13 +29,28 @@ public function handleRequestThrowable(
     $throwable) {
 
     $viewer = $this->getViewer($request);
+    $results = $throwable->getFactorValidationResults();
 
     $form = id(new PhabricatorAuthSessionEngine())->renderHighSecurityForm(
       $throwable->getFactors(),
-      $throwable->getFactorValidationResults(),
+      $results,
       $viewer,
       $request);
 
+    $is_wait = false;
+    foreach ($results as $result) {
+      if ($result->getIsWait()) {
+        $is_wait = true;
+        break;
+      }
+    }
+
+    if ($is_wait) {
+      $submit = pht('Wait Patiently');
+    } else {
+      $submit = pht('Enter High Security');
+    }
+
     $dialog = id(new AphrontDialogView())
       ->setUser($viewer)
       ->setTitle(pht('Entering High Security'))
@@ -62,7 +77,7 @@ public function handleRequestThrowable(
           'actions, you should leave high security.'))
       ->setSubmitURI($request->getPath())
       ->addCancelButton($throwable->getCancelURI())
-      ->addSubmitButton(pht('Enter High Security'));
+      ->addSubmitButton($submit);
 
     $request_parameters = $request->getPassthroughRequestParameters(
       $respect_quicksand = true);

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -480,7 +480,59 @@ private function newHighSecurityToken(
       new PhabricatorAuthTryFactorAction(),
       0);
 
+    $now = PhabricatorTime::getNow();
+
+    // We need to do challenge validation first, since this happens whether you
+    // submitted responses or not. You can't get a "bad response" error before
+    // you actually submit a response, but you can get a "wait, we can't
+    // issue a challenge yet" response. Load all issued challenges which are
+    // currently valid.
+    $challenges = id(new PhabricatorAuthChallengeQuery())
+      ->setViewer($viewer)
+      ->withFactorPHIDs(mpull($factors, 'getPHID'))
+      ->withUserPHIDs(array($viewer->getPHID()))
+      ->withChallengeTTLBetween($now, null)
+      ->execute();
+    $challenge_map = mgroup($challenges, 'getFactorPHID');
+
     $validation_results = array();
+    $ok = true;
+
+    // Validate each factor against issued challenges. For example, this
+    // prevents you from receiving or responding to a TOTP challenge if another
+    // challenge was recently issued to a different session.
+    foreach ($factors as $factor) {
+      $factor_phid = $factor->getPHID();
+      $issued_challenges = idx($challenge_map, $factor_phid, array());
+      $impl = $factor->requireImplementation();
+
+      $new_challenges = $impl->getNewIssuedChallenges(
+        $factor,
+        $viewer,
+        $issued_challenges);
+
+      foreach ($new_challenges as $new_challenge) {
+        $issued_challenges[] = $new_challenge;
+      }
+      $challenge_map[$factor_phid] = $issued_challenges;
+
+      if (!$issued_challenges) {
+        continue;
+      }
+
+      $result = $impl->getResultFromIssuedChallenges(
+        $factor,
+        $viewer,
+        $issued_challenges);
+
+      if (!$result) {
+        continue;
+      }
+
+      $ok = false;
+      $validation_results[$factor_phid] = $result;
+    }
+
     if ($request->isHTTPPost()) {
       $request->validateCSRF();
       if ($request->getExists(AphrontRequest::TYPE_HISEC)) {
@@ -491,30 +543,28 @@ private function newHighSecurityToken(
           new PhabricatorAuthTryFactorAction(),
           1);
 
-        $ok = true;
         foreach ($factors as $factor) {
-          $id = $factor->getID();
+          $factor_phid = $factor->getPHID();
+
+          // If we already have a validation result from previously issued
+          // challenges, skip validating this factor.
+          if (isset($validation_results[$factor_phid])) {
+            continue;
+          }
+
           $impl = $factor->requireImplementation();
 
-          $validation_result = $impl->processValidateFactorForm(
+          $validation_result = $impl->getResultFromChallengeResponse(
             $factor,
             $viewer,
-            $request);
-
-          if (!($validation_result instanceof PhabricatorAuthFactorResult)) {
-            throw new Exception(
-              pht(
-                'Expected "processValidateFactorForm()" to return an object '.
-                'of class "%s"; got something else (from "%s").',
-                'PhabricatorAuthFactorResult',
-                get_class($impl)));
-          }
+            $request,
+            $issued_challenges);
 
           if (!$validation_result->getIsValid()) {
             $ok = false;
           }
 
-          $validation_results[$id] = $validation_result;
+          $validation_results[$factor_phid] = $validation_result;
         }
 
         if ($ok) {
@@ -566,6 +616,18 @@ private function newHighSecurityToken(
       return $token;
     }
 
+    // If we don't have a validation result for some factors yet, fill them
+    // in with an empty result so form rendering doesn't have to care if the
+    // results exist or not. This happens when you first load the form and have
+    // not submitted any responses yet.
+    foreach ($factors as $factor) {
+      $factor_phid = $factor->getPHID();
+      if (isset($validation_results[$factor_phid])) {
+        continue;
+      }
+      $validation_results[$factor_phid] = new PhabricatorAuthFactorResult();
+    }
+
     throw id(new PhabricatorAuthHighSecurityRequiredException())
       ->setCancelURI($cancel_uri)
       ->setFactors($factors)
@@ -613,7 +675,7 @@ public function renderHighSecurityForm(
       ->appendRemarkupInstructions('');
 
     foreach ($factors as $factor) {
-      $result = idx($validation_results, $factor->getID());
+      $result = $validation_results[$factor->getPHID()];
 
       $factor->requireImplementation()->renderValidateFactorForm(
         $factor,

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -14,12 +14,7 @@ abstract public function renderValidateFactorForm(
     PhabricatorAuthFactorConfig $config,
     AphrontFormView $form,
     PhabricatorUser $viewer,
-    PhabricatorAuthFactorResult $validation_result = null);
-
-  abstract public function processValidateFactorForm(
-    PhabricatorAuthFactorConfig $config,
-    PhabricatorUser $viewer,
-    AphrontRequest $request);
+    PhabricatorAuthFactorResult $validation_result);
 
   public function getParameterName(
     PhabricatorAuthFactorConfig $config,
@@ -40,4 +35,131 @@ protected function newConfigForUser(PhabricatorUser $user) {
       ->setFactorKey($this->getFactorKey());
   }
 
+  protected function newResult() {
+    return new PhabricatorAuthFactorResult();
+  }
+
+  protected function newChallenge(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer) {
+
+    return id(new PhabricatorAuthChallenge())
+      ->setUserPHID($viewer->getPHID())
+      ->setSessionPHID($viewer->getSession()->getPHID())
+      ->setFactorPHID($config->getPHID());
+  }
+
+  final public function getNewIssuedChallenges(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer,
+    array $challenges) {
+    assert_instances_of($challenges, 'PhabricatorAuthChallenge');
+
+    $now = PhabricatorTime::getNow();
+
+    $new_challenges = $this->newIssuedChallenges(
+      $config,
+      $viewer,
+      $challenges);
+
+    assert_instances_of($new_challenges, 'PhabricatorAuthChallenge');
+
+    foreach ($new_challenges as $new_challenge) {
+      $ttl = $new_challenge->getChallengeTTL();
+      if (!$ttl) {
+        throw new Exception(
+          pht('Newly issued MFA challenges must have a valid TTL!'));
+      }
+
+      if ($ttl < $now) {
+        throw new Exception(
+          pht(
+            'Newly issued MFA challenges must have a future TTL. This '.
+            'factor issued a bad TTL ("%s"). (Did you use a relative '.
+            'time instead of an epoch?)',
+            $ttl));
+      }
+    }
+
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      foreach ($new_challenges as $challenge) {
+        $challenge->save();
+      }
+    unset($unguarded);
+
+    return $new_challenges;
+  }
+
+  abstract protected function newIssuedChallenges(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer,
+    array $challenges);
+
+  final public function getResultFromIssuedChallenges(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer,
+    array $challenges) {
+    assert_instances_of($challenges, 'PhabricatorAuthChallenge');
+
+    $result = $this->newResultFromIssuedChallenges(
+      $config,
+      $viewer,
+      $challenges);
+
+    if ($result === null) {
+      return $result;
+    }
+
+    if (!($result instanceof PhabricatorAuthFactorResult)) {
+      throw new Exception(
+        pht(
+          'Expected "newResultFromIssuedChallenges()" to return null or '.
+          'an object of class "%s"; got something else (in "%s").',
+          'PhabricatorAuthFactorResult',
+          get_class($this)));
+    }
+
+    $result->setIssuedChallenges($challenges);
+
+    return $result;
+  }
+
+  abstract protected function newResultFromIssuedChallenges(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer,
+    array $challenges);
+
+  final public function getResultFromChallengeResponse(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer,
+    AphrontRequest $request,
+    array $challenges) {
+    assert_instances_of($challenges, 'PhabricatorAuthChallenge');
+
+    $result = $this->newResultFromChallengeResponse(
+      $config,
+      $viewer,
+      $request,
+      $challenges);
+
+    if (!($result instanceof PhabricatorAuthFactorResult)) {
+      throw new Exception(
+        pht(
+          'Expected "newResultFromChallengeResponse()" to return an object '.
+          'of class "%s"; got something else (in "%s").',
+          'PhabricatorAuthFactorResult',
+          get_class($this)));
+    }
+
+    $result->setIssuedChallenges($challenges);
+
+    return $result;
+  }
+
+  abstract protected function newResultFromChallengeResponse(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer,
+    AphrontRequest $request,
+    array $challenges);
+
 }