From 7c1d1c13f4a308244f30ef7658d447f9347ee133 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 23 Jan 2019 07:22:10 -0800
Subject: [PATCH] Add a rate limit for enroll attempts when adding new MFA
 configurations

Summary:
Depends on D20018. Ref T13222. When you add a new MFA configuration, you can technically (?) guess your way through it with brute force. It's not clear why this would ever really be useful (if an attacker can get here and wants to add TOTP, they can just add TOTP!) but it's probably bad, so don't let users do it.

This limit is fairly generous because I don't think this actually part of any real attack, at least today with factors we're considering.

Test Plan:
  - Added TOTP, guessed wrong a ton of times, got rate limited.
  - Added TOTP, guessed right, got a TOTP factor configuration added to my account.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20019
---
 src/__phutil_library_map__.php                |  2 ++
 .../action/PhabricatorAuthNewFactorAction.php | 21 ++++++++++++++++
 .../PhabricatorMultiFactorSettingsPanel.php   | 24 +++++++++++++++++++
 3 files changed, 47 insertions(+)
 create mode 100644 src/applications/auth/action/PhabricatorAuthNewFactorAction.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9e7769f093..7b4055d1b0 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2296,6 +2296,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthNeedsApprovalController' => 'applications/auth/controller/PhabricatorAuthNeedsApprovalController.php',
     'PhabricatorAuthNeedsMultiFactorController' => 'applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php',
     'PhabricatorAuthNewController' => 'applications/auth/controller/config/PhabricatorAuthNewController.php',
+    'PhabricatorAuthNewFactorAction' => 'applications/auth/action/PhabricatorAuthNewFactorAction.php',
     'PhabricatorAuthOldOAuthRedirectController' => 'applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php',
     'PhabricatorAuthOneTimeLoginController' => 'applications/auth/controller/PhabricatorAuthOneTimeLoginController.php',
     'PhabricatorAuthOneTimeLoginTemporaryTokenType' => 'applications/auth/tokentype/PhabricatorAuthOneTimeLoginTemporaryTokenType.php',
@@ -8021,6 +8022,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthNeedsApprovalController' => 'PhabricatorAuthController',
     'PhabricatorAuthNeedsMultiFactorController' => 'PhabricatorAuthController',
     'PhabricatorAuthNewController' => 'PhabricatorAuthProviderConfigController',
+    'PhabricatorAuthNewFactorAction' => 'PhabricatorSystemAction',
     'PhabricatorAuthOldOAuthRedirectController' => 'PhabricatorAuthController',
     'PhabricatorAuthOneTimeLoginController' => 'PhabricatorAuthController',
     'PhabricatorAuthOneTimeLoginTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
diff --git a/src/applications/auth/action/PhabricatorAuthNewFactorAction.php b/src/applications/auth/action/PhabricatorAuthNewFactorAction.php
new file mode 100644
index 0000000000..c1244587f1
--- /dev/null
+++ b/src/applications/auth/action/PhabricatorAuthNewFactorAction.php
@@ -0,0 +1,21 @@
+<?php
+
+final class PhabricatorAuthNewFactorAction extends PhabricatorSystemAction {
+
+  const TYPECONST = 'auth.factor.new';
+
+  public function getActionConstant() {
+    return self::TYPECONST;
+  }
+
+  public function getScoreThreshold() {
+    return 60 / phutil_units('1 hour in seconds');
+  }
+
+  public function getLimitExplanation() {
+    return pht(
+      'You have failed too many attempts to synchronize new multi-factor '.
+      'authentication methods in a short period of time.');
+  }
+
+}
diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
index 60fca07ff9..e5d1ba17c2 100644
--- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
@@ -234,12 +234,36 @@ final class PhabricatorMultiFactorSettingsPanel
     $form = id(new AphrontFormView())
       ->setViewer($viewer);
 
+    if ($request->isFormPost()) {
+      // Subject users to rate limiting so that it's difficult to add factors
+      // by pure brute force. This is normally not much of an attack, but push
+      // factor types may have side effects.
+      PhabricatorSystemActionEngine::willTakeAction(
+        array($viewer->getPHID()),
+        new PhabricatorAuthNewFactorAction(),
+        1);
+    } else {
+      // Test the limit before showing the user a form, so we don't give them
+      // a form which can never possibly work because it will always hit rate
+      // limiting.
+      PhabricatorSystemActionEngine::willTakeAction(
+        array($viewer->getPHID()),
+        new PhabricatorAuthNewFactorAction(),
+        0);
+    }
+
     $config = $selected_provider->processAddFactorForm(
       $form,
       $request,
       $user);
 
     if ($config) {
+      // If the user added a factor, give them a rate limiting point back.
+      PhabricatorSystemActionEngine::willTakeAction(
+        array($viewer->getPHID()),
+        new PhabricatorAuthNewFactorAction(),
+        -1);
+
       $config->save();
 
       $log = PhabricatorUserLog::initializeNewLog(