diff --git a/resources/sql/autopatches/20180121.auth.06.legacydigest.sql b/resources/sql/autopatches/20180121.auth.06.legacydigest.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20180121.auth.06.legacydigest.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_password + ADD legacyDigestFormat VARCHAR(32) COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180121.auth.07.marklegacy.sql b/resources/sql/autopatches/20180121.auth.07.marklegacy.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20180121.auth.07.marklegacy.sql @@ -0,0 +1,4 @@ +UPDATE {$NAMESPACE}_auth.auth_password + SET legacyDigestFormat = 'v1' + WHERE passwordType IN ('vcs', 'account') + AND legacyDigestFormat IS NULL; diff --git a/src/applications/auth/storage/PhabricatorAuthPassword.php b/src/applications/auth/storage/PhabricatorAuthPassword.php --- a/src/applications/auth/storage/PhabricatorAuthPassword.php +++ b/src/applications/auth/storage/PhabricatorAuthPassword.php @@ -12,6 +12,7 @@ protected $passwordHash; protected $passwordSalt; protected $isRevoked; + protected $legacyDigestFormat; private $object = self::ATTACHABLE; @@ -38,6 +39,7 @@ 'passwordHash' => 'text128', 'passwordSalt' => 'text64', 'isRevoked' => 'bool', + 'legacyDigestFormat' => 'text32?', ), self::CONFIG_KEY_SCHEMA => array( 'key_role' => array( @@ -66,6 +68,12 @@ } public function canUpgrade() { + // If this password uses a legacy digest format, we can upgrade it to the + // new digest format even if a better hasher isn't available. + if ($this->getLegacyDigestFormat() !== null) { + return true; + } + $hash = $this->newPasswordEnvelope(); return PhabricatorPasswordHasher::canUpgradeHash($hash); } @@ -110,6 +118,9 @@ $new_salt = Filesystem::readRandomCharacters(64); $this->setPasswordSalt($new_salt); + // Clear any legacy digest format to force a modern digest. + $this->setLegacyDigestFormat(null); + $digest = $this->digestPassword($password, $object); $hash = $hasher->getPasswordHashForStorage($digest); $raw_hash = $hash->openEnvelope(); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -1590,7 +1590,7 @@ // 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 + // For legacy compatibility reasons, old 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 @@ -1602,43 +1602,44 @@ // 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); - 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)); + if ($password->getLegacyDigestFormat() == 'v1') { + switch ($password->getPasswordType()) { + case PhabricatorAuthPassword::PASSWORD_TYPE_VCS: + // Old VCS passwords use an iterated HMAC SHA1 as a digest algorithm. + // They originally used this as a hasher, but it became a digest + // algorithm 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); + case PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT: + // Account passwords previously used this weird mess of salt and did + // not digest the input to a standard length. + + // 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 an attacker + // attempts to use a very long input 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