Page MenuHomePhabricator
Files F15399480

D18908.id45326.diff
No OneTemporary
Actions

D18908.id45326.diff
View Options

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

File Metadata

Mime Type
text/plain
Expires
Tue, Mar 18, 4:45 AM (4 d, 5 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7709402
Default Alt Text
D18908.id45326.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility