Prepare the new AuthPassword infrastructure for storing account passwords

Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.

Before account passwords can move, we need to make two changes:

  - For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
  - Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.

Then add a nice story about password digestion.

Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

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

resources/sql/autopatches/20180121.auth.02.passsalt.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_password
+  ADD passwordSalt VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT};

src/__phutil_library_map__.php

@@ -3494,6 +3494,7 @@ phutil_register_library_map(array(
     'PhabricatorPassphraseApplication' => 'applications/passphrase/application/PhabricatorPassphraseApplication.php',
     'PhabricatorPasswordAuthProvider' => 'applications/auth/provider/PhabricatorPasswordAuthProvider.php',
     'PhabricatorPasswordDestructionEngineExtension' => 'applications/auth/extension/PhabricatorPasswordDestructionEngineExtension.php',
+    'PhabricatorPasswordHashInterface' => 'applications/auth/password/PhabricatorPasswordHashInterface.php',
     'PhabricatorPasswordHasher' => 'infrastructure/util/password/PhabricatorPasswordHasher.php',
     'PhabricatorPasswordHasherTestCase' => 'infrastructure/util/password/__tests__/PhabricatorPasswordHasherTestCase.php',
     'PhabricatorPasswordHasherUnavailableException' => 'infrastructure/util/password/PhabricatorPasswordHasherUnavailableException.php',
@@ -9992,6 +9993,7 @@ phutil_register_library_map(array(
       'PhabricatorFulltextInterface',
       'PhabricatorFerretInterface',
       'PhabricatorConduitResultInterface',
+      'PhabricatorPasswordHashInterface',
     ),
     'PhabricatorUserBadgesCacheType' => 'PhabricatorUserCacheType',
     'PhabricatorUserBlurbField' => 'PhabricatorUserCustomField',

src/applications/auth/engine/PhabricatorAuthPasswordEngine.php

@@ -27,7 +27,7 @@ final class PhabricatorAuthPasswordEngine
     return $this->contentSource;
   }
 
-  public function setObject($object) {
+  public function setObject(PhabricatorPasswordHashInterface $object) {
     $this->object = $object;
     return $this;
   }

src/applications/auth/password/PhabricatorPasswordHashInterface.php

@@ -0,0 +1,9 @@
+<?php
+
+interface PhabricatorPasswordHashInterface {
+
+  public function newPasswordDigest(
+    PhutilOpaqueEnvelope $envelope,
+    PhabricatorAuthPassword $password);
+
+}

src/applications/auth/storage/PhabricatorAuthPassword.php

@@ -10,6 +10,7 @@ final class PhabricatorAuthPassword
   protected $objectPHID;
   protected $passwordType;
   protected $passwordHash;
+  protected $passwordSalt;
   protected $isRevoked;
 
   private $object = self::ATTACHABLE;
@@ -19,7 +20,7 @@ final class PhabricatorAuthPassword
   const PASSWORD_TYPE_TEST = 'test';
 
   public static function initializeNewPassword(
-    PhabricatorUser $object,
+    PhabricatorPasswordHashInterface $object,
     $type) {
 
     return id(new self())
@@ -35,6 +36,7 @@ final class PhabricatorAuthPassword
       self::CONFIG_COLUMN_SCHEMA => array(
         'passwordType' => 'text64',
         'passwordHash' => 'text128',
+        'passwordSalt' => 'text64',
         'isRevoked' => 'bool',
       ),
       self::CONFIG_KEY_SCHEMA => array(
@@ -70,7 +72,7 @@ final class PhabricatorAuthPassword
 
   public function upgradePasswordHasher(
     PhutilOpaqueEnvelope $envelope,
-    PhabricatorUser $object) {
+    PhabricatorPasswordHashInterface $object) {
 
     // Before we make changes, double check that this is really the correct
     // password. It could be really bad if we "upgraded" a password and changed
@@ -88,7 +90,7 @@ final class PhabricatorAuthPassword
 
   public function setPassword(
     PhutilOpaqueEnvelope $password,
-    PhabricatorUser $object) {
+    PhabricatorPasswordHashInterface $object) {
 
     $hasher = PhabricatorPasswordHasher::getBestHasher();
     return $this->setPasswordWithHasher($password, $object, $hasher);
@@ -96,9 +98,18 @@ final class PhabricatorAuthPassword
 
   public function setPasswordWithHasher(
     PhutilOpaqueEnvelope $password,
-    PhabricatorUser $object,
+    PhabricatorPasswordHashInterface $object,
     PhabricatorPasswordHasher $hasher) {
 
+    if (!strlen($password->openEnvelope())) {
+      throw new Exception(
+        pht('Attempting to set an empty password!'));
+    }
+
+    // Generate (or regenerate) the salt first.
+    $new_salt = Filesystem::readRandomCharacters(64);
+    $this->setPasswordSalt($new_salt);
+
     $digest = $this->digestPassword($password, $object);
     $hash = $hasher->getPasswordHashForStorage($digest);
     $raw_hash = $hash->openEnvelope();
@@ -108,7 +119,7 @@ final class PhabricatorAuthPassword
 
   public function comparePassword(
     PhutilOpaqueEnvelope $password,
-    PhabricatorUser $object) {
+    PhabricatorPasswordHashInterface $object) {
 
     $digest = $this->digestPassword($password, $object);
     $hash = $this->newPasswordEnvelope();
@@ -122,7 +133,7 @@ final class PhabricatorAuthPassword
 
   private function digestPassword(
     PhutilOpaqueEnvelope $password,
-    PhabricatorUser $object) {
+    PhabricatorPasswordHashInterface $object) {
 
     $object_phid = $object->getPHID();
 
@@ -135,9 +146,17 @@ final class PhabricatorAuthPassword
           $object->getPHID()));
     }
 
-    $raw_input = PhabricatorHash::digestPassword($password, $object_phid);
+    $digest = $object->newPasswordDigest($password, $this);
 
-    return new PhutilOpaqueEnvelope($raw_input);
+    if (!($digest instanceof PhutilOpaqueEnvelope)) {
+      throw new Exception(
+        pht(
+          'Failed to digest password: object ("%s") did not return an '.
+          'opaque envelope with a password digest.',
+          $object->getPHID()));
+    }
+
+    return $digest;
   }
 
 

src/applications/people/storage/PhabricatorUser.php

@@ -20,7 +20,8 @@ final class PhabricatorUser
     PhabricatorApplicationTransactionInterface,
     PhabricatorFulltextInterface,
     PhabricatorFerretInterface,
-    PhabricatorConduitResultInterface {
+    PhabricatorConduitResultInterface,
+    PhabricatorPasswordHashInterface {
 
   const SESSION_TABLE = 'phabricator_session';
   const NAMETOKEN_TABLE = 'user_nametoken';
@@ -1620,4 +1621,66 @@ final class PhabricatorUser
     return $variables[$variable_key];
   }
 
+/* -(  PhabricatorPasswordHashInterface  )----------------------------------- */
+
+
+  public function newPasswordDigest(
+    PhutilOpaqueEnvelope $envelope,
+    PhabricatorAuthPassword $password) {
+
+    // Before passwords are hashed, they are digested. The goal of digestion
+    // is twofold: to reduce the length of very long passwords to something
+    // reasonable; and to salt the password in case the best available hasher
+    // does not include salt automatically.
+
+    // Users may choose arbitrarily long passwords, and attackers may try to
+    // attack the system by probing it with very long passwords. When large
+    // inputs are passed to hashers -- which are intentionally slow -- it
+    // can result in unacceptably long runtimes. The classic attack here is
+    // to try to log in with a 64MB password and see if that locks up the
+    // machine for the next century. By digesting passwords to a standard
+    // length first, the length of the raw input does not impact the runtime
+    // of the hashing algorithm.
+
+    // Some hashers like bcrypt are self-salting, while other hashers are not.
+    // Applying salt while digesting passwords ensures that hashes are salted
+    // whether we ultimately select a self-salting hasher or not.
+
+    // For legacy compatibility reasons, the VCS and Account password digest
+    // algorithms are significantly more complicated than necessary to achieve
+    // these goals. This is because they once used a different hashing and
+    // salting process. When we upgraded to the modern modular hasher
+    // infrastructure, we just bolted it onto the end of the existing pipelines
+    // so that upgrading didn't break all users' credentials.
+
+    // New implementations can (and, generally, should) safely select the
+    // simple HMAC SHA256 digest at the bottom of the function, which does
+    // everything that a digest callback should without any needless legacy
+    // baggage on top.
+
+    switch ($password->getPasswordType()) {
+      case PhabricatorAuthPassword::PASSWORD_TYPE_VCS:
+        // VCS passwords use an iterated HMAC SHA1 as a digest algorithm. They
+        // originally used this as a hasher, but it became a digest alorithm
+        // once hashing was upgraded to include bcrypt.
+        $digest = $envelope->openEnvelope();
+        $salt = $this->getPHID();
+        for ($ii = 0; $ii < 1000; $ii++) {
+          $digest = PhabricatorHash::weakDigest($digest, $salt);
+        }
+        return new PhutilOpaqueEnvelope($digest);
+    }
+
+    // For passwords which do not have some crazy legacy reason to use some
+    // other digest algorithm, HMAC SHA256 is an excellent choice. It satisfies
+    // the digest requirements and is simple.
+
+    $digest = PhabricatorHash::digestHMACSHA256(
+      $envelope->openEnvelope(),
+      $password->getPasswordSalt());
+
+    return new PhutilOpaqueEnvelope($digest);
+  }
+
+
 }

src/infrastructure/util/PhabricatorHash.php

@@ -29,24 +29,6 @@ final class PhabricatorHash extends Phobject {
   }
 
 
-  /**
-   * Digest a string into a password hash. This is similar to @{method:digest},
-   * but requires a salt and iterates the hash to increase cost.
-   */
-  public static function digestPassword(PhutilOpaqueEnvelope $envelope, $salt) {
-    $result = $envelope->openEnvelope();
-    if (!$result) {
-      throw new Exception(pht('Trying to digest empty password!'));
-    }
-
-    for ($ii = 0; $ii < 1000; $ii++) {
-      $result = self::weakDigest($result, $salt);
-    }
-
-    return $result;
-  }
-
-
   /**
    * Digest a string for use in, e.g., a MySQL index. This produces a short
    * (12-byte), case-sensitive alphanumeric string with 72 bits of entropy,