Tighten some MFA/TOTP parameters to improve resistance to brute force attacks

Summary:
Depends on D19897. Ref T13222. See some discussion in D19890.

  - Only rate limit users if they're actually answering a challenge, not if they're just clicking "Wait Patiently".
  - Reduce the number of allowed attempts per hour from 100 back to 10.
  - Reduce the TOTP window from +/- 2 timesteps (allowing ~60 seconds of skew) to +/- 1 timestep (allowing ~30 seconds of skew).
  - Change the window where a TOTP response remains valid to a flat 60 seconds instead of a calculation based on windows and timesteps.

Test Plan:
  - Hit an MFA prompt.
  - Without typing in any codes, mashed "submit" as much as I wanted (>>10 times / hour).
  - Answered prompt correctly.
  - Mashed "Wait Patiently" as much as I wanted (>>10 times / hour).
  - Guessed random numbers, was rate limited after 10 attempts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

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

src/applications/auth/action/PhabricatorAuthTryFactorAction.php

@@ -9,7 +9,7 @@ final class PhabricatorAuthTryFactorAction extends PhabricatorSystemAction {
   }
 
   public function getScoreThreshold() {
-    return 100 / phutil_units('1 hour in seconds');
+    return 10 / phutil_units('1 hour in seconds');
   }
 
   public function getLimitExplanation() {

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -567,10 +567,21 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       if ($request->getExists(AphrontRequest::TYPE_HISEC)) {
 
         // Limit factor verification rates to prevent brute force attacks.
-        PhabricatorSystemActionEngine::willTakeAction(
-          array($viewer->getPHID()),
-          new PhabricatorAuthTryFactorAction(),
-          1);
+        $any_attempt = false;
+        foreach ($factors as $factor) {
+          $impl = $factor->requireImplementation();
+          if ($impl->getRequestHasChallengeResponse($factor, $request)) {
+            $any_attempt = true;
+            break;
+          }
+        }
+
+        if ($any_attempt) {
+          PhabricatorSystemActionEngine::willTakeAction(
+            array($viewer->getPHID()),
+            new PhabricatorAuthTryFactorAction(),
+            1);
+        }
 
         foreach ($factors as $factor) {
           $factor_phid = $factor->getPHID();
@@ -610,10 +621,12 @@ final class PhabricatorAuthSessionEngine extends Phobject {
           }
 
           // Give the user a credit back for a successful factor verification.
-          PhabricatorSystemActionEngine::willTakeAction(
-            array($viewer->getPHID()),
-            new PhabricatorAuthTryFactorAction(),
-            -1);
+          if ($any_attempt) {
+            PhabricatorSystemActionEngine::willTakeAction(
+              array($viewer->getPHID()),
+              new PhabricatorAuthTryFactorAction(),
+              -1);
+          }
 
           if ($session->getIsPartial() && !$jump_into_hisec) {
             // If we have a partial session and are not jumping directly into

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -52,6 +52,10 @@ abstract class PhabricatorAuthFactor extends Phobject {
       ->setWorkflowKey($engine->getWorkflowKey());
   }
 
+  abstract public function getRequestHasChallengeResponse(
+    PhabricatorAuthFactorConfig $config,
+    AphrontRequest $response);
+
   final public function getNewIssuedChallenges(
     PhabricatorAuthFactorConfig $config,
     PhabricatorUser $viewer,

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -80,7 +80,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       $okay = (bool)$this->getTimestepAtWhichResponseIsValid(
         $this->getAllowedTimesteps($this->getCurrentTimestep()),
         new PhutilOpaqueEnvelope($key),
-        (string)$code);
+        $code);
 
       if ($okay) {
         $config = $this->newConfigForUser($user)
@@ -206,9 +206,10 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     if (!$control) {
       $value = $result->getValue();
       $error = $result->getErrorMessage();
+      $name = $this->getChallengeResponseParameterName($config);
 
       $control = id(new PHUIFormNumberControl())
-        ->setName($this->getParameterName($config, 'totpcode'))
+        ->setName($name)
         ->setDisableAutocomplete(true)
         ->setValue($value)
         ->setError($error);
@@ -221,6 +222,15 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     $form->appendChild($control);
   }
 
+  public function getRequestHasChallengeResponse(
+    PhabricatorAuthFactorConfig $config,
+    AphrontRequest $request) {
+
+    $value = $this->getChallengeResponseFromRequest($config, $request);
+    return (bool)strlen($value);
+  }
+
+
   protected function newResultFromIssuedChallenges(
     PhabricatorAuthFactorConfig $config,
     PhabricatorUser $viewer,
@@ -301,7 +311,9 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     AphrontRequest $request,
     array $challenges) {
 
-    $code = $request->getStr($this->getParameterName($config, 'totpcode'));
+    $code = $this->getChallengeResponseFromRequest(
+      $config,
+      $request);
 
     $result = $this->newResult()
       ->setValue($code);
@@ -335,13 +347,10 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     $valid_timestep = $this->getTimestepAtWhichResponseIsValid(
       array_intersect_key($challenge_timesteps, $current_timesteps),
       new PhutilOpaqueEnvelope($config->getFactorSecret()),
-      (string)$code);
+      $code);
 
     if ($valid_timestep) {
-      $now = PhabricatorTime::getNow();
-      $step_duration = $this->getTimestepDuration();
-      $step_window = $this->getTimestepWindowSize();
-      $ttl = $now + ($step_duration * $step_window);
+      $ttl = PhabricatorTime::getNow() + 60;
 
       $challenge
         ->setProperty('totp.timestep', $valid_timestep)
@@ -475,7 +484,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     // 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;
+    return 1;
   }
 
   private function getTimestepAtWhichResponseIsValid(
@@ -493,6 +502,21 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     return null;
   }
 
+  private function getChallengeResponseParameterName(
+    PhabricatorAuthFactorConfig $config) {
+    return $this->getParameterName($config, 'totpcode');
+  }
 
+  private function getChallengeResponseFromRequest(
+    PhabricatorAuthFactorConfig $config,
+    AphrontRequest $request) {
 
+    $name = $this->getChallengeResponseParameterName($config);
+
+    $value = $request->getStr($name);
+    $value = (string)$value;
+    $value = trim($value);
+
+    return $value;
+  }
 }