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,