Move account passwords to shared infrastructure

Summary:
Ref T13043. This moves user account passwords to the new shared infrastructure.

There's a lot of code changes here, but essentially all of it is the same as the VCS password logic in D18898.

Test Plan:
- Ran migration.
- Spot checked table for general sanity.
- Logged in with an existing password.
- Hit all error conditions on "change password", "set password", "register new account" flows.
- Verified that changing password logs out other sessions.
- Verified that revoked passwords of a different type can't be selected.
- Changed passwords a bunch.
- Verified that salt regenerates properly after password change.
- Tried to login with the wrong password, which didn't work.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

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

resources/sql/autopatches/20180120.auth.03.vcsdata.sql

@@ -1,5 +1,6 @@
 INSERT INTO {$NAMESPACE}_auth.auth_password
   (objectPHID, phid, passwordType, passwordHash, isRevoked,
     dateCreated, dateModified)
-  SELECT userPHID, '', 'vcs', passwordHash, 0, dateCreated, dateModified
+  SELECT userPHID, CONCAT('XVCS', id), 'vcs', passwordHash, 0,
+      dateCreated, dateModified
     FROM {$NAMESPACE}_repository.repository_vcspassword;

resources/sql/autopatches/20180120.auth.04.vcsphid.php

@@ -6,8 +6,10 @@
 $table = new PhabricatorAuthPassword();
 $conn = $table->establishConnection('w');
 
+$password_type = PhabricatorAuthPasswordPHIDType::TYPECONST;
+
 foreach (new LiskMigrationIterator($table) as $row) {
-  if ($row->getPHID()) {
+  if (phid_get_type($row->getPHID()) == $password_type) {
     continue;
   }
 

resources/sql/autopatches/20180121.auth.03.accountdata.sql

@@ -0,0 +1,7 @@
+INSERT INTO {$NAMESPACE}_auth.auth_password
+  (objectPHID, phid, passwordType, passwordHash, passwordSalt, isRevoked,
+    dateCreated, dateModified)
+  SELECT phid, CONCAT('XACCOUNT', id), 'account', passwordHash, passwordSalt, 0,
+      dateCreated, dateModified
+    FROM {$NAMESPACE}_user.user
+    WHERE passwordHash != '';

resources/sql/autopatches/20180121.auth.04.accountphid.php

@@ -0,0 +1,24 @@
+<?php
+
+// Populate account passwords (which we copied from the user table in the last
+// migration) with new PHIDs.
+
+$table = new PhabricatorAuthPassword();
+$conn = $table->establishConnection('w');
+
+$password_type = PhabricatorAuthPasswordPHIDType::TYPECONST;
+
+foreach (new LiskMigrationIterator($table) as $row) {
+  if (phid_get_type($row->getPHID()) == $password_type) {
+    continue;
+  }
+
+  $new_phid = $row->generatePHID();
+
+  queryfx(
+    $conn,
+    'UPDATE %T SET phid = %s WHERE id = %d',
+    $table->getTableName(),
+    $new_phid,
+    $row->getID());
+}

src/applications/auth/controller/PhabricatorAuthRegisterController.php

@@ -61,6 +61,9 @@ final class PhabricatorAuthRegisterController
     $default_username = $account->getUsername();
     $default_realname = $account->getRealName();
 
+    $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
+    $content_source = PhabricatorContentSource::newFromRequest($request);
+
     $default_email = $account->getEmail();
 
     if ($invite) {
@@ -285,27 +288,22 @@ final class PhabricatorAuthRegisterController
       if ($must_set_password) {
         $value_password = $request->getStr('password');
         $value_confirm = $request->getStr('confirm');
-        if (!strlen($value_password)) {
-          $e_password = pht('Required');
-          $errors[] = pht('You must choose a password.');
-        } else if ($value_password !== $value_confirm) {
-          $e_password = pht('No Match');
-          $errors[] = pht('Password and confirmation must match.');
-        } else if (strlen($value_password) < $min_len) {
-          $e_password = pht('Too Short');
-          $errors[] = pht(
-            'Password is too short (must be at least %d characters long).',
-            $min_len);
-        } else if (
-          PhabricatorCommonPasswords::isCommonPassword($value_password)) {
 
-          $e_password = pht('Very Weak');
+        $password_envelope = new PhutilOpaqueEnvelope($value_password);
-          $errors[] = pht(
+        $confirm_envelope = new PhutilOpaqueEnvelope($value_confirm);
-            'Password is pathologically weak. This password is one of the '.
+
-            'most common passwords in use, and is extremely easy for '.
+        $engine = id(new PhabricatorAuthPasswordEngine())
-            'attackers to guess. You must choose a stronger password.');
+          ->setViewer($user)
-        } else {
+          ->setContentSource($content_source)
+          ->setPasswordType($account_type)
+          ->setObject($user);
+
+        try {
+          $engine->checkNewPassword($password_envelope, $confirm_envelope);
           $e_password = null;
+        } catch (PhabricatorAuthPasswordException $ex) {
+          $errors[] = $ex->getMessage();
+          $e_password = $ex->getPasswordError();
         }
       }
 
@@ -408,8 +406,13 @@ final class PhabricatorAuthRegisterController
 
             $editor->createNewUser($user, $email_obj, $allow_reassign_email);
             if ($must_set_password) {
-              $envelope = new PhutilOpaqueEnvelope($value_password);
+              $password_object = PhabricatorAuthPassword::initializeNewPassword(
-              $editor->changePassword($user, $envelope);
+                $user,
+                $account_type);
+
+              $password_object
+                ->setPassword($password_envelope, $user)
+                ->save();
             }
 
             if ($is_setup) {

src/applications/auth/controller/PhabricatorAuthSetPasswordController.php

@@ -40,8 +40,30 @@ final class PhabricatorAuthSetPasswordController
       return new Aphront404Response();
     }
 
-    $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
+    $content_source = PhabricatorContentSource::newFromRequest($request);
-    $min_len = (int)$min_len;
+    $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
+
+    $password_objects = id(new PhabricatorAuthPasswordQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($viewer->getPHID()))
+      ->withPasswordTypes(array($account_type))
+      ->withIsRevoked(false)
+      ->execute();
+    if ($password_objects) {
+      $password_object = head($password_objects);
+      $has_password = true;
+    } else {
+      $password_object = PhabricatorAuthPassword::initializeNewPassword(
+        $viewer,
+        $account_type);
+      $has_password = false;
+    }
+
+    $engine = id(new PhabricatorAuthPasswordEngine())
+      ->setViewer($viewer)
+      ->setContentSource($content_source)
+      ->setPasswordType($account_type)
+      ->setObject($viewer);
 
     $e_password = true;
     $e_confirm = true;
@@ -50,46 +72,23 @@ final class PhabricatorAuthSetPasswordController
       $password = $request->getStr('password');
       $confirm = $request->getStr('confirm');
 
+      $password_envelope = new PhutilOpaqueEnvelope($password);
+      $confirm_envelope = new PhutilOpaqueEnvelope($confirm);
+
+      try {
+        $engine->checkNewPassword($password_envelope, $confirm_envelope, true);
         $e_password = null;
         $e_confirm = null;
-
+      } catch (PhabricatorAuthPasswordException $ex) {
-      if (!strlen($password)) {
+        $errors[] = $ex->getMessage();
-        $errors[] = pht('You must choose a password or skip this step.');
+        $e_password = $ex->getPasswordError();
-        $e_password = pht('Required');
+        $e_confirm = $ex->getConfirmError();
-      } else if (strlen($password) < $min_len) {
-        $errors[] = pht(
-          'The selected password is too short. Passwords must be a minimum '.
-          'of %s characters.',
-          new PhutilNumber($min_len));
-        $e_password = pht('Too Short');
-      } else if (!strlen($confirm)) {
-        $errors[] = pht('You must confirm the selecetd password.');
-        $e_confirm = pht('Required');
-      } else if ($password !== $confirm) {
-        $errors[] = pht('The password and confirmation do not match.');
-        $e_password = pht('Invalid');
-        $e_confirm = pht('Invalid');
-      } else if (PhabricatorCommonPasswords::isCommonPassword($password)) {
-        $e_password = pht('Very Weak');
-        $errors[] = pht(
-          'The selected password is very weak: it is one of the most common '.
-          'passwords in use. Choose a stronger password.');
       }
 
       if (!$errors) {
-        $envelope = new PhutilOpaqueEnvelope($password);
+        $password_object
-
+          ->setPassword($password_envelope, $viewer)
-        // This write is unguarded because the CSRF token has already
+          ->save();
-        // been checked in the call to $request->isFormPost() and
-        // the CSRF token depends on the password hash, so when it
-        // is changed here the CSRF token check will fail.
-        $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-
-          id(new PhabricatorUserEditor())
-            ->setActor($viewer)
-            ->changePassword($viewer, $envelope);
-
-        unset($unguarded);
 
         // Destroy the token.
         $auth_token->delete();
@@ -98,12 +97,15 @@ final class PhabricatorAuthSetPasswordController
       }
     }
 
+    $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
+    $min_len = (int)$min_len;
+
     $len_caption = null;
     if ($min_len) {
       $len_caption = pht('Minimum password length: %d characters.', $min_len);
     }
 
-    if ($viewer->hasPassword()) {
+    if ($has_password) {
       $title = pht('Reset Password');
       $crumb = pht('Reset Password');
       $submit = pht('Reset Password');

src/applications/auth/engine/PhabricatorAuthPasswordEngine.php

@@ -110,6 +110,12 @@ final class PhabricatorAuthPasswordEngine
         pht('Very Weak'));
     }
 
+    // If we're creating a brand new object (like registering a new user)
+    // and it does not have a PHID yet, it isn't possible for it to have any
+    // revoked passwords or colliding passwords either, so we can skip these
+    // checks.
+
+    if ($this->getObject()->getPHID()) {
       if ($this->isRevokedPassword($password)) {
         throw new PhabricatorAuthPasswordException(
           pht(
@@ -126,6 +132,7 @@ final class PhabricatorAuthPasswordEngine
           pht('Not Unique'));
       }
     }
+  }
 
   public function isValidPassword(PhutilOpaqueEnvelope $envelope) {
     $this->requireSetup();

src/applications/auth/provider/PhabricatorPasswordAuthProvider.php

@@ -253,6 +253,7 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider {
 
     $request = $controller->getRequest();
     $viewer = $request->getUser();
+    $content_source = PhabricatorContentSource::newFromRequest($request);
 
     $require_captcha = false;
     $captcha_valid = false;
@@ -285,22 +286,16 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider {
 
           if ($user) {
             $envelope = new PhutilOpaqueEnvelope($request->getStr('password'));
-            if ($user->comparePassword($envelope)) {
+
+            $engine = id(new PhabricatorAuthPasswordEngine())
+              ->setViewer($user)
+              ->setContentSource($content_source)
+              ->setPasswordType(PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT)
+              ->setObject($user);
+
+            if ($engine->isValidPassword($envelope)) {
               $account = $this->loadOrCreateAccount($user->getPHID());
               $log_user = $user;
-
-              // If the user's password is stored using a less-than-optimal
-              // hash, upgrade them to the strongest available hash.
-
-              $hash_envelope = new PhutilOpaqueEnvelope(
-                $user->getPasswordHash());
-              if (PhabricatorPasswordHasher::canUpgradeHash($hash_envelope)) {
-                $user->setPassword($envelope);
-
-                $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-                  $user->save();
-                unset($unguarded);
-              }
             }
           }
         }

src/applications/auth/storage/PhabricatorAuthPassword.php

@@ -127,7 +127,7 @@ final class PhabricatorAuthPassword
     return PhabricatorPasswordHasher::comparePassword($digest, $hash);
   }
 
-  private function newPasswordEnvelope() {
+  public function newPasswordEnvelope() {
     return new PhutilOpaqueEnvelope($this->getPasswordHash());
   }
 

src/applications/people/editor/PhabricatorUserEditor.php

@@ -129,33 +129,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
   }
 
 
-  /**
-   * @task edit
-   */
-  public function changePassword(
-    PhabricatorUser $user,
-    PhutilOpaqueEnvelope $envelope) {
-
-    if (!$user->getID()) {
-      throw new Exception(pht('User has not been created yet!'));
-    }
-
-    $user->openTransaction();
-      $user->reload();
-
-      $user->setPassword($envelope);
-      $user->save();
-
-      $log = PhabricatorUserLog::initializeNewLog(
-        $this->requireActor(),
-        $user->getPHID(),
-        PhabricatorUserLog::ACTION_CHANGE_PASSWORD);
-      $log->save();
-
-    $user->saveTransaction();
-  }
-
-
   /**
    * @task edit
    */

src/applications/people/storage/PhabricatorUser.php

@@ -263,28 +263,6 @@ final class PhabricatorUser
       PhabricatorPeopleUserPHIDType::TYPECONST);
   }
 
-  public function hasPassword() {
-    return (bool)strlen($this->passwordHash);
-  }
-
-  public function setPassword(PhutilOpaqueEnvelope $envelope) {
-    if (!$this->getPHID()) {
-      throw new Exception(
-        pht(
-          'You can not set a password for an unsaved user because their PHID '.
-          'is a salt component in the password hash.'));
-    }
-
-    if (!strlen($envelope->openEnvelope())) {
-      $this->setPasswordHash('');
-    } else {
-      $this->setPasswordSalt(md5(Filesystem::readRandomBytes(32)));
-      $hash = $this->hashPassword($envelope);
-      $this->setPasswordHash($hash->openEnvelope());
-    }
-    return $this;
-  }
-
   public function getMonogram() {
     return '@'.$this->getUsername();
   }
@@ -332,36 +310,6 @@ final class PhabricatorUser
     return Filesystem::readRandomCharacters(255);
   }
 
-  public function comparePassword(PhutilOpaqueEnvelope $envelope) {
-    if (!strlen($envelope->openEnvelope())) {
-      return false;
-    }
-    if (!strlen($this->getPasswordHash())) {
-      return false;
-    }
-
-    return PhabricatorPasswordHasher::comparePassword(
-      $this->getPasswordHashInput($envelope),
-      new PhutilOpaqueEnvelope($this->getPasswordHash()));
-  }
-
-  private function getPasswordHashInput(PhutilOpaqueEnvelope $password) {
-    $input =
-      $this->getUsername().
-      $password->openEnvelope().
-      $this->getPHID().
-      $this->getPasswordSalt();
-
-    return new PhutilOpaqueEnvelope($input);
-  }
-
-  private function hashPassword(PhutilOpaqueEnvelope $password) {
-    $hasher = PhabricatorPasswordHasher::getBestHasher();
-
-    $input_envelope = $this->getPasswordHashInput($password);
-    return $hasher->getPasswordHashForStorage($input_envelope);
-  }
-
   const CSRF_CYCLE_FREQUENCY  = 3600;
   const CSRF_SALT_LENGTH      = 8;
   const CSRF_TOKEN_LENGTH     = 16;
@@ -1669,6 +1617,32 @@ final class PhabricatorUser
           $digest = PhabricatorHash::weakDigest($digest, $salt);
         }
         return new PhutilOpaqueEnvelope($digest);
+      case PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT:
+        // Account passwords use this weird mess of salt and do not digest
+        // the input to a standard length.
+
+        // TODO: We should build a migration pathway forward from this which
+        // uses a better (HMAC SHA256) digest algorithm. Beyond this being
+        // a weird special case, there are two actual problems with this,
+        // although neither are particularly severe:
+
+        // First, because we do not normalize the length of passwords, this
+        // algorithm may make us vulnerable to DOS attacks where attacker
+        // attempt to use very long inputs to slow down hashers.
+
+        // Second, because the username is part of the hash algorithm, renaming
+        // a user breaks their password. This isn't a huge deal but it's pretty
+        // silly. There's no security justification for this behavior, I just
+        // didn't think about the implication when I wrote it originally.
+
+        $parts = array(
+          $this->getUsername(),
+          $envelope->openEnvelope(),
+          $this->getPHID(),
+          $password->getPasswordSalt(),
+        );
+
+        return new PhutilOpaqueEnvelope(implode('', $parts));
     }
 
     // For passwords which do not have some crazy legacy reason to use some

src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php

@@ -26,6 +26,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
 
   public function processRequest(AphrontRequest $request) {
     $user = $request->getUser();
+    $content_source = PhabricatorContentSource::newFromRequest($request);
 
     $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
       $user,
@@ -40,6 +41,22 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
     // registration or password reset. If this flow changes, that flow may
     // also need to change.
 
+    $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
+
+    $password_objects = id(new PhabricatorAuthPasswordQuery())
+      ->setViewer($user)
+      ->withObjectPHIDs(array($user->getPHID()))
+      ->withPasswordTypes(array($account_type))
+      ->withIsRevoked(false)
+      ->execute();
+    if ($password_objects) {
+      $password_object = head($password_objects);
+    } else {
+      $password_object = PhabricatorAuthPassword::initializeNewPassword(
+        $user,
+        $account_type);
+    }
+
     $e_old = true;
     $e_new = true;
     $e_conf = true;
@@ -47,41 +64,42 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
     $errors = array();
     if ($request->isFormPost()) {
       $envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw'));
-      if (!$user->comparePassword($envelope)) {
+
+      $engine = id(new PhabricatorAuthPasswordEngine())
+        ->setViewer($user)
+        ->setContentSource($content_source)
+        ->setPasswordType($account_type)
+        ->setObject($user);
+
+      if (!strlen($envelope->openEnvelope())) {
+        $errors[] = pht('You must enter your current password.');
+        $e_old = pht('Required');
+      } else if (!$engine->isValidPassword($envelope)) {
         $errors[] = pht('The old password you entered is incorrect.');
         $e_old = pht('Invalid');
+      } else {
+        $e_old = null;
       }
 
       $pass = $request->getStr('new_pw');
       $conf = $request->getStr('conf_pw');
+      $password_envelope = new PhutilOpaqueEnvelope($pass);
+      $confirm_envelope = new PhutilOpaqueEnvelope($conf);
 
-      if (strlen($pass) < $min_len) {
+      try {
-        $errors[] = pht('Your new password is too short.');
+        $engine->checkNewPassword($password_envelope, $confirm_envelope);
-        $e_new = pht('Too Short');
+        $e_new = null;
-      } else if ($pass !== $conf) {
+        $e_conf = null;
-        $errors[] = pht('New password and confirmation do not match.');
+      } catch (PhabricatorAuthPasswordException $ex) {
-        $e_conf = pht('Invalid');
+        $errors[] = $ex->getMessage();
-      } else if (PhabricatorCommonPasswords::isCommonPassword($pass)) {
+        $e_new = $ex->getPasswordError();
-        $e_new = pht('Very Weak');
+        $e_conf = $ex->getConfirmError();
-        $e_conf = pht('Very Weak');
-        $errors[] = pht(
-          'Your new password is very weak: it is one of the most common '.
-          'passwords in use. Choose a stronger password.');
       }
 
       if (!$errors) {
-        // This write is unguarded because the CSRF token has already
+        $password_object
-        // been checked in the call to $request->isFormPost() and
+          ->setPassword($password_envelope, $user)
-        // the CSRF token depends on the password hash, so when it
+          ->save();
-        // is changed here the CSRF token check will fail.
-        $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-
-        $envelope = new PhutilOpaqueEnvelope($pass);
-          id(new PhabricatorUserEditor())
-            ->setActor($user)
-            ->changePassword($user, $envelope);
-
-        unset($unguarded);
 
         $next = $this->getPanelURI('?saved=true');
 
@@ -93,11 +111,9 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
       }
     }
 
-    $hash_envelope = new PhutilOpaqueEnvelope($user->getPasswordHash());
+    if ($password_object->getID()) {
-    if (strlen($hash_envelope->openEnvelope())) {
       try {
-        $can_upgrade = PhabricatorPasswordHasher::canUpgradeHash(
+        $can_upgrade = $password_object->canUpgrade();
-          $hash_envelope);
       } catch (PhabricatorPasswordHasherUnavailableException $ex) {
         $can_upgrade = false;
 
@@ -154,7 +170,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
     $properties->addProperty(
       pht('Current Algorithm'),
       PhabricatorPasswordHasher::getCurrentAlgorithmName(
-        new PhutilOpaqueEnvelope($user->getPasswordHash())));
+        $password_object->newPasswordEnvelope()));
 
     $properties->addProperty(
       pht('Best Available Algorithm'),