From 5a8a56f414fce1385bdc3b1a46334b1a489f0765 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jan 2018 14:58:46 -0800 Subject: [PATCH] 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 --- .../autopatches/20180121.auth.02.passsalt.sql | 2 + src/__phutil_library_map__.php | 2 + .../engine/PhabricatorAuthPasswordEngine.php | 2 +- .../PhabricatorPasswordHashInterface.php | 9 +++ .../auth/storage/PhabricatorAuthPassword.php | 35 +++++++--- .../people/storage/PhabricatorUser.php | 65 ++++++++++++++++++- src/infrastructure/util/PhabricatorHash.php | 18 ----- 7 files changed, 105 insertions(+), 28 deletions(-) create mode 100644 resources/sql/autopatches/20180121.auth.02.passsalt.sql create mode 100644 src/applications/auth/password/PhabricatorPasswordHashInterface.php diff --git a/resources/sql/autopatches/20180121.auth.02.passsalt.sql b/resources/sql/autopatches/20180121.auth.02.passsalt.sql new file mode 100644 index 0000000000..78ee953ea4 --- /dev/null +++ b/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}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 50f4abc475..efd8c0ed26 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php index 1cbfa0c80b..3ac83c3c59 100644 --- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php +++ b/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; } diff --git a/src/applications/auth/password/PhabricatorPasswordHashInterface.php b/src/applications/auth/password/PhabricatorPasswordHashInterface.php new file mode 100644 index 0000000000..10cad716c1 --- /dev/null +++ b/src/applications/auth/password/PhabricatorPasswordHashInterface.php @@ -0,0 +1,9 @@ + 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; } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 30aa3d81ef..455991ca53 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/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); + } + + } diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php index 5ca4e9b53c..4cd1abcf15 100644 --- a/src/infrastructure/util/PhabricatorHash.php +++ b/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,