Page MenuHomePhabricator
Files F15280303

D18900.id45316.diff
No OneTemporary
Actions

D18900.id45316.diff
View Options

diff --git a/resources/sql/autopatches/20180121.auth.02.passsalt.sql b/resources/sql/autopatches/20180121.auth.02.passsalt.sql
new file mode 100644
--- /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
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3492,6 +3492,7 @@
'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',
@@ -9988,6 +9989,7 @@
'PhabricatorFulltextInterface',
'PhabricatorFerretInterface',
'PhabricatorConduitResultInterface',
+ 'PhabricatorPasswordHashInterface',
),
'PhabricatorUserBadgesCacheType' => 'PhabricatorUserCacheType',
'PhabricatorUserBlurbField' => 'PhabricatorUserCustomField',
diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
--- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
@@ -27,7 +27,7 @@
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
--- /dev/null
+++ b/src/applications/auth/password/PhabricatorPasswordHashInterface.php
@@ -0,0 +1,9 @@
+<?php
+
+interface PhabricatorPasswordHashInterface {
+
+ public function newPasswordDigest(
+ PhutilOpaqueEnvelope $envelope,
+ PhabricatorAuthPassword $password);
+
+}
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
@@ -10,6 +10,7 @@
protected $objectPHID;
protected $passwordType;
protected $passwordHash;
+ protected $passwordSalt;
protected $isRevoked;
private $object = self::ATTACHABLE;
@@ -19,7 +20,7 @@
const PASSWORD_TYPE_TEST = 'test';
public static function initializeNewPassword(
- PhabricatorUser $object,
+ PhabricatorPasswordHashInterface $object,
$type) {
return id(new self())
@@ -35,6 +36,7 @@
self::CONFIG_COLUMN_SCHEMA => array(
'passwordType' => 'text64',
'passwordHash' => 'text128',
+ 'passwordSalt' => 'text64',
'isRevoked' => 'bool',
),
self::CONFIG_KEY_SCHEMA => array(
@@ -70,7 +72,7 @@
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 @@
public function setPassword(
PhutilOpaqueEnvelope $password,
- PhabricatorUser $object) {
+ PhabricatorPasswordHashInterface $object) {
$hasher = PhabricatorPasswordHasher::getBestHasher();
return $this->setPasswordWithHasher($password, $object, $hasher);
@@ -96,9 +98,18 @@
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 @@
public function comparePassword(
PhutilOpaqueEnvelope $password,
- PhabricatorUser $object) {
+ PhabricatorPasswordHashInterface $object) {
$digest = $this->digestPassword($password, $object);
$hash = $this->newPasswordEnvelope();
@@ -122,7 +133,7 @@
private function digestPassword(
PhutilOpaqueEnvelope $password,
- PhabricatorUser $object) {
+ PhabricatorPasswordHashInterface $object) {
$object_phid = $object->getPHID();
@@ -135,9 +146,17 @@
$object->getPHID()));
}
- $raw_input = PhabricatorHash::digestPassword($password, $object_phid);
+ $digest = $object->newPasswordDigest($password, $this);
+
+ 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 new PhutilOpaqueEnvelope($raw_input);
+ return $digest;
}
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
@@ -20,7 +20,8 @@
PhabricatorApplicationTransactionInterface,
PhabricatorFulltextInterface,
PhabricatorFerretInterface,
- PhabricatorConduitResultInterface {
+ PhabricatorConduitResultInterface,
+ PhabricatorPasswordHashInterface {
const SESSION_TABLE = 'phabricator_session';
const NAMETOKEN_TABLE = 'user_nametoken';
@@ -1620,4 +1621,66 @@
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
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -30,24 +30,6 @@
/**
- * 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,
* which is generally safe in most contexts (notably, URLs).

File Metadata

Mime Type
text/plain
Expires
Tue, Mar 4, 9:40 AM (16 h, 12 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7221344
Default Alt Text
D18900.id45316.diff (10 KB)

Event Timeline

Phabricator · Built by Phacility