From e56dc8f299862dce89ef279bc394183ab15a06dc Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 Aug 2014 12:04:23 -0700
Subject: [PATCH] Invalidate outstanding password reset links when users adjust
 email addresses

Summary:
Fixes T5506. Depends on D10133. When users remove an email address or change their primary email address, invalidate any outstanding password reset links.

This is a very small security risk, but the current behavior is somewhat surprising, and an attacker could sit on a reset link for up to 24 hours and then use it to re-compromise an account.

Test Plan:
  - Changed primary address and removed addreses.
  - Verified these actions invalidated outstanding one-time login temporary tokens.
  - Tried to use revoked reset links.
  - Revoked normally from new UI panel.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5506

Differential Revision: https://secure.phabricator.com/D10134
---
 .../PhabricatorAuthRevokeTokenController.php  |  2 +-
 .../storage/PhabricatorAuthTemporaryToken.php | 24 +++++++++++++++++++
 .../people/editor/PhabricatorUserEditor.php   | 20 ++++++++++++++++
 ...PhabricatorSettingsPanelEmailAddresses.php | 24 +++++++++++++------
 4 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php b/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php
index 1b78a9c829..27981eee27 100644
--- a/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php
+++ b/src/applications/auth/controller/PhabricatorAuthRevokeTokenController.php
@@ -46,7 +46,7 @@ final class PhabricatorAuthRevokeTokenController
 
     if ($request->isDialogFormPost()) {
       foreach ($tokens as $token) {
-        $token->setTokenExpires(PhabricatorTime::getNow() - 1)->save();
+        $token->revokeToken();
       }
       return id(new AphrontRedirectResponse())->setURI($panel_uri);
     }
diff --git a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php
index 970ea7ef2b..167ec39350 100644
--- a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php
+++ b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php
@@ -44,6 +44,30 @@ final class PhabricatorAuthTemporaryToken extends PhabricatorAuthDAO
     return false;
   }
 
+  public function revokeToken() {
+    if ($this->isRevocable()) {
+      $this->setTokenExpires(PhabricatorTime::getNow() - 1)->save();
+    }
+    return $this;
+  }
+
+  public static function revokeTokens(
+    PhabricatorUser $viewer,
+    array $object_phids,
+    array $token_types) {
+
+    $tokens = id(new PhabricatorAuthTemporaryTokenQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs($object_phids)
+      ->withTokenTypes($token_types)
+      ->withExpired(false)
+      ->execute();
+
+    foreach ($tokens as $token) {
+      $token->revokeToken();
+    }
+  }
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 
diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php
index 4f2c5ed8a7..aafb20a7e2 100644
--- a/src/applications/people/editor/PhabricatorUserEditor.php
+++ b/src/applications/people/editor/PhabricatorUserEditor.php
@@ -427,6 +427,8 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
       $user->endWriteLocking();
     $user->saveTransaction();
 
+    $this->revokePasswordResetLinks($user);
+
     return $this;
   }
 
@@ -490,6 +492,9 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
     }
     $email->sendNewPrimaryEmail($user);
 
+
+    $this->revokePasswordResetLinks($user);
+
     return $this;
   }
 
@@ -575,4 +580,19 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
     }
   }
 
+  private function revokePasswordResetLinks(PhabricatorUser $user) {
+    // Revoke any outstanding password reset links. If an attacker compromises
+    // an account, changes the email address, and sends themselves a password
+    // reset link, it could otherwise remain live for a short period of time
+    // and allow them to compromise the account again later.
+
+    PhabricatorAuthTemporaryToken::revokeTokens(
+      $user,
+      array($user->getPHID()),
+      array(
+        PhabricatorAuthSessionEngine::ONETIME_TEMPORARY_TOKEN_TYPE,
+        PhabricatorAuthSessionEngine::PASSWORD_TEMPORARY_TOKEN_TYPE,
+      ));
+  }
+
 }
diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelEmailAddresses.php b/src/applications/settings/panel/PhabricatorSettingsPanelEmailAddresses.php
index 50ff17e3e6..c4a7ecf419 100644
--- a/src/applications/settings/panel/PhabricatorSettingsPanelEmailAddresses.php
+++ b/src/applications/settings/panel/PhabricatorSettingsPanelEmailAddresses.php
@@ -276,9 +276,14 @@ final class PhabricatorSettingsPanelEmailAddresses
       ->setUser($user)
       ->addHiddenInput('delete', $email_id)
       ->setTitle(pht("Really delete address '%s'?", $address))
-      ->appendChild(phutil_tag('p', array(), pht(
-        'Are you sure you want to delete this address? You will no '.
-        'longer be able to use it to login.')))
+      ->appendParagraph(
+        pht(
+          'Are you sure you want to delete this address? You will no '.
+            'longer be able to use it to login.'))
+      ->appendParagraph(
+        pht(
+          'Note: Removing an email address from your account will invalidate '.
+          'any outstanding password reset links.'))
       ->addSubmitButton(pht('Delete'))
       ->addCancelButton($uri);
 
@@ -359,10 +364,15 @@ final class PhabricatorSettingsPanelEmailAddresses
       ->setUser($user)
       ->addHiddenInput('primary', $email_id)
       ->setTitle(pht('Change primary email address?'))
-      ->appendChild(phutil_tag('p', array(), pht(
-        'If you change your primary address, Phabricator will send'.
-          ' all email to %s.',
-        $address)))
+      ->appendParagraph(
+        pht(
+          'If you change your primary address, Phabricator will send all '.
+          'email to %s.',
+          $address))
+      ->appendParagraph(
+        pht(
+          'Note: Changing your primary email address will invalidate any '.
+          'outstanding password reset links.'))
       ->addSubmitButton(pht('Change Primary Address'))
       ->addCancelButton($uri);