From 026ec11b9d6bd7f6d7311011b02d0d412c7be320 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 21 Jan 2018 17:44:00 -0800
Subject: [PATCH] Add a rate limit for guessing old passwords when changing
 passwords

Summary:
Depends on D18904. Ref T13043. If an attacker compromises a victim's session and bypasses their MFA, they can try to guess the user's current account password by making repeated requests to change it: if they guess the right "Old Password", they get a different error than if they don't.

I don't think this is really a very serious concern (the attacker already got a session and MFA, if configured, somehow; many installs don't use passwords anyway) but we get occasional reports about it from HackerOne. Technically, it's better policy to rate limit it, and this should reduce the reports we receive.

Test Plan: Tried to change password over and over again, eventually got rated limited. Used `bin/auth unlimit` to clear the limit, changed password normally without issues.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18906
---
 src/__phutil_library_map__.php                |  2 ++
 .../PhabricatorAuthChangePasswordAction.php   | 22 ++++++++++++++++
 .../PhabricatorPasswordSettingsPanel.php      | 26 +++++++++++++++----
 3 files changed, 45 insertions(+), 5 deletions(-)
 create mode 100644 src/applications/auth/action/PhabricatorAuthChangePasswordAction.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 55d7aa3b5f..c83e336a70 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2033,6 +2033,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php',
     'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php',
     'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php',
+    'PhabricatorAuthChangePasswordAction' => 'applications/auth/action/PhabricatorAuthChangePasswordAction.php',
     'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php',
     'PhabricatorAuthConduitTokenRevoker' => 'applications/auth/revoker/PhabricatorAuthConduitTokenRevoker.php',
     'PhabricatorAuthConfirmLinkController' => 'applications/auth/controller/PhabricatorAuthConfirmLinkController.php',
@@ -7321,6 +7322,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthApplication' => 'PhabricatorApplication',
     'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthAuthProviderPHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorAuthChangePasswordAction' => 'PhabricatorSystemAction',
     'PhabricatorAuthConduitAPIMethod' => 'ConduitAPIMethod',
     'PhabricatorAuthConduitTokenRevoker' => 'PhabricatorAuthRevoker',
     'PhabricatorAuthConfirmLinkController' => 'PhabricatorAuthController',
diff --git a/src/applications/auth/action/PhabricatorAuthChangePasswordAction.php b/src/applications/auth/action/PhabricatorAuthChangePasswordAction.php
new file mode 100644
index 0000000000..323c3e65b6
--- /dev/null
+++ b/src/applications/auth/action/PhabricatorAuthChangePasswordAction.php
@@ -0,0 +1,22 @@
+<?php
+
+final class PhabricatorAuthChangePasswordAction
+  extends PhabricatorSystemAction {
+
+  const TYPECONST = 'auth.password';
+
+  public function getActionConstant() {
+    return self::TYPECONST;
+  }
+
+  public function getScoreThreshold() {
+    return 20 / phutil_units('1 hour in seconds');
+  }
+
+  public function getLimitExplanation() {
+    return pht(
+      'You have failed to enter the correct account password too often in '.
+      'a short period of time.');
+  }
+
+}
diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
index 793928d5e2..9fb8838cf7 100644
--- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
@@ -25,11 +25,13 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
   }
 
   public function processRequest(AphrontRequest $request) {
-    $user = $request->getUser();
+    $viewer = $request->getUser();
+    $user = $this->getUser();
+
     $content_source = PhabricatorContentSource::newFromRequest($request);
 
     $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
-      $user,
+      $viewer,
       $request,
       '/settings/');
 
@@ -44,7 +46,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
     $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
 
     $password_objects = id(new PhabricatorAuthPasswordQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->withObjectPHIDs(array($user->getPHID()))
       ->withPasswordTypes(array($account_type))
       ->withIsRevoked(false)
@@ -63,10 +65,18 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
 
     $errors = array();
     if ($request->isFormPost()) {
+      // Rate limit guesses about the old password. This page requires MFA and
+      // session compromise already, so this is mostly just to stop researchers
+      // from reporting this as a vulnerability.
+      PhabricatorSystemActionEngine::willTakeAction(
+        array($viewer->getPHID()),
+        new PhabricatorAuthChangePasswordAction(),
+        1);
+
       $envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw'));
 
       $engine = id(new PhabricatorAuthPasswordEngine())
-        ->setViewer($user)
+        ->setViewer($viewer)
         ->setContentSource($content_source)
         ->setPasswordType($account_type)
         ->setObject($user);
@@ -79,6 +89,12 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
         $e_old = pht('Invalid');
       } else {
         $e_old = null;
+
+        // Refund the user an action credit for getting the password right.
+        PhabricatorSystemActionEngine::willTakeAction(
+          array($viewer->getPHID()),
+          new PhabricatorAuthChangePasswordAction(),
+          -1);
       }
 
       $pass = $request->getStr('new_pw');
@@ -142,7 +158,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
     }
 
     $form = id(new AphrontFormView())
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->appendChild(
         id(new AphrontFormPasswordControl())
           ->setLabel(pht('Old Password'))