Rate limit multi-factor actions

Summary: Ref T4398. Prevent users from brute forcing multi-factor auth by rate limiting attempts. This slightly refines the rate limiting to allow callers to check for a rate limit without adding points, and gives users credit for successfully completing an auth workflow.

Test Plan: Tried to enter hisec with bad credentials 11 times in a row, got rate limited.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

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

resources/celerity/map.php

@@ -356,6 +356,7 @@ return array(
     'rsrc/js/application/conpherence/behavior-pontificate.js' => '53f6f2dd',
     'rsrc/js/application/conpherence/behavior-widget-pane.js' => 'd8ef8659',
     'rsrc/js/application/countdown/timer.js' => '889c96f3',
+    'rsrc/js/application/dashboard/behavior-dashboard-async-panel.js' => '4398eabb',
     'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'f2441746',
     'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => '533a187b',
     'rsrc/js/application/differential/behavior-comment-jump.js' => '71755c79',
@@ -546,6 +547,7 @@ return array(
     'javelin-behavior-conpherence-widget-pane' => 'd8ef8659',
     'javelin-behavior-countdown-timer' => '889c96f3',
     'javelin-behavior-dark-console' => 'e9fdb5e5',
+    'javelin-behavior-dashboard-async-panel' => '4398eabb',
     'javelin-behavior-device' => '03d6ed07',
     'javelin-behavior-differential-add-reviewers-and-ccs' => '533a187b',
     'javelin-behavior-differential-comment-jump' => '71755c79',
@@ -1073,6 +1075,12 @@ return array(
       1 => 'javelin-dom',
       2 => 'phortune-credit-card-form',
     ),
+    '4398eabb' =>
+    array(
+      0 => 'javelin-behavior',
+      1 => 'javelin-dom',
+      2 => 'javelin-workflow',
+    ),
     '441f2137' =>
     array(
       0 => 'javelin-behavior',

src/__phutil_library_map__.php

@@ -1257,6 +1257,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php',
     'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php',
     'PhabricatorAuthTerminateSessionController' => 'applications/auth/controller/PhabricatorAuthTerminateSessionController.php',
+    'PhabricatorAuthTryFactorAction' => 'applications/auth/action/PhabricatorAuthTryFactorAction.php',
     'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php',
     'PhabricatorAuthValidateController' => 'applications/auth/controller/PhabricatorAuthValidateController.php',
     'PhabricatorAuthenticationConfigOptions' => 'applications/config/option/PhabricatorAuthenticationConfigOptions.php',
@@ -4029,6 +4030,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorAuthStartController' => 'PhabricatorAuthController',
     'PhabricatorAuthTerminateSessionController' => 'PhabricatorAuthController',
+    'PhabricatorAuthTryFactorAction' => 'PhabricatorSystemAction',
     'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController',
     'PhabricatorAuthValidateController' => 'PhabricatorAuthController',
     'PhabricatorAuthenticationConfigOptions' => 'PhabricatorApplicationConfigOptions',

src/applications/auth/action/PhabricatorAuthTryFactorAction.php

@@ -0,0 +1,21 @@
+<?php
+
+final class PhabricatorAuthTryFactorAction extends PhabricatorSystemAction {
+
+  const TYPECONST = 'auth.factor';
+
+  public function getActionConstant() {
+    return self::TYPECONST;
+  }
+
+  public function getScoreThreshold() {
+    return 10 / phutil_units('1 hour in seconds');
+  }
+
+  public function getLimitExplanation() {
+    return pht(
+      'You have failed to verify multi-factor authentication too often in '.
+      'a short period of time.');
+  }
+
+}

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -247,11 +247,24 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       return $this->issueHighSecurityToken($session, true);
     }
 
+    // Check for a rate limit without awarding points, so the user doesn't
+    // get partway through the workflow only to get blocked.
+    PhabricatorSystemActionEngine::willTakeAction(
+      array($viewer->getPHID()),
+      new PhabricatorAuthTryFactorAction(),
+      0);
+
     $validation_results = array();
     if ($request->isHTTPPost()) {
       $request->validateCSRF();
       if ($request->getExists(AphrontRequest::TYPE_HISEC)) {
 
+        // Limit factor verification rates to prevent brute force attacks.
+        PhabricatorSystemActionEngine::willTakeAction(
+          array($viewer->getPHID()),
+          new PhabricatorAuthTryFactorAction(),
+          1);
+
         $ok = true;
         foreach ($factors as $factor) {
           $id = $factor->getID();
@@ -268,6 +281,12 @@ final class PhabricatorAuthSessionEngine extends Phobject {
         }
 
         if ($ok) {
+          // Give the user a credit back for a successful factor verification.
+          PhabricatorSystemActionEngine::willTakeAction(
+            array($viewer->getPHID()),
+            new PhabricatorAuthTryFactorAction(),
+            -1);
+
           $until = time() + phutil_units('15 minutes in seconds');
           $session->setHighSecurityUntil($until);
 
@@ -284,6 +303,9 @@ final class PhabricatorAuthSessionEngine extends Phobject {
             PhabricatorUserLog::ACTION_ENTER_HISEC);
           $log->save();
         } else {
+
+
+
           $log = PhabricatorUserLog::initializeNewLog(
             $viewer,
             $viewer->getPHID(),

src/applications/system/engine/PhabricatorSystemActionEngine.php

@@ -2,6 +2,42 @@
 
 final class PhabricatorSystemActionEngine extends Phobject {
 
+  /**
+   * Prepare to take an action, throwing an exception if the user has exceeded
+   * the rate limit.
+   *
+   * The `$actors` are a list of strings. Normally this will be a list of
+   * user PHIDs, but some systems use other identifiers (like email
+   * addresses). Each actor's score threshold is tracked independently. If
+   * any actor exceeds the rate limit for the action, this method throws.
+   *
+   * The `$action` defines the actual thing being rate limited, and sets the
+   * limit.
+   *
+   * You can pass either a positive, zero, or negative `$score` to this method:
+   *
+   *   - If the score is positive, the user is given that many points toward
+   *     the rate limit after the limit is checked. Over time, this will cause
+   *     them to hit the rate limit and be prevented from taking further
+   *     actions.
+   *   - If the score is zero, the rate limit is checked but no score changes
+   *     are made. This allows you to check for a rate limit before beginning
+   *     a workflow, so the user doesn't fill in a form only to get rate limited
+   *     at the end.
+   *   - If the score is negative, the user is credited points, allowing them
+   *     to take more actions than the limit normally permits. By awarding
+   *     points for failed actions and credits for successful actions, a
+   *     system can be sensitive to failure without overly restricting
+   *     legitimate uses.
+   *
+   * If any actor is exceeding their rate limit, this method throws a
+   * @{class:PhabricatorSystemActionRateLimitException}.
+   *
+   * @param list<string> List of actors.
+   * @param PhabricatorSystemAction Action being taken.
+   * @param float Score or credit, see above.
+   * @return void
+   */
   public static function willTakeAction(
     array $actors,
     PhabricatorSystemAction $action,
@@ -20,9 +56,11 @@ final class PhabricatorSystemActionEngine extends Phobject {
       }
     }
 
-    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-      self::recordAction($actors, $action, $score);
-    unset($unguarded);
+    if ($score != 0) {
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        self::recordAction($actors, $action, $score);
+      unset($unguarded);
+    }
   }
 
   public static function loadBlockedActors(
@@ -35,9 +73,17 @@ final class PhabricatorSystemActionEngine extends Phobject {
 
     $blocked = array();
     foreach ($scores as $actor => $actor_score) {
-      $actor_score = $actor_score + ($score / $window);
+      // For the purposes of checking for a block, we just use the raw
+      // persistent score and do not include the score for this action. This
+      // allows callers to test for a block without adding any points and get
+      // the same result they would if they were adding points: we only
+      // trigger a rate limit when the persistent score exceeds the threshold.
       if ($action->shouldBlockActor($actor, $actor_score)) {
-        $blocked[$actor] = $actor_score;
+        // When reporting the results, we do include the points for this
+        // action. This makes the error messages more clear, since they
+        // more accurately report the number of actions the user has really
+        // tried to take.
+        $blocked[$actor] = $actor_score + ($score / $window);
       }
     }