Page MenuHomePhabricator
Files F15415308

D18903.id45319.diff
No OneTemporary
Actions

D18903.id45319.diff
View Options

diff --git a/resources/sql/autopatches/20180120.auth.03.vcsdata.sql b/resources/sql/autopatches/20180120.auth.03.vcsdata.sql
--- a/resources/sql/autopatches/20180120.auth.03.vcsdata.sql
+++ b/resources/sql/autopatches/20180120.auth.03.vcsdata.sql
@@ -1,5 +1,6 @@
INSERT INTO {$NAMESPACE}_auth.auth_password
(objectPHID, phid, passwordType, passwordHash, isRevoked,
dateCreated, dateModified)
- SELECT userPHID, '', 'vcs', passwordHash, 0, dateCreated, dateModified
+ SELECT userPHID, CONCAT('XVCS', id), 'vcs', passwordHash, 0,
+ dateCreated, dateModified
FROM {$NAMESPACE}_repository.repository_vcspassword;
diff --git a/resources/sql/autopatches/20180120.auth.04.vcsphid.php b/resources/sql/autopatches/20180120.auth.04.vcsphid.php
--- a/resources/sql/autopatches/20180120.auth.04.vcsphid.php
+++ b/resources/sql/autopatches/20180120.auth.04.vcsphid.php
@@ -6,8 +6,10 @@
$table = new PhabricatorAuthPassword();
$conn = $table->establishConnection('w');
+$password_type = PhabricatorAuthPasswordPHIDType::TYPECONST;
+
foreach (new LiskMigrationIterator($table) as $row) {
- if ($row->getPHID()) {
+ if (phid_get_type($row->getPHID()) == $password_type) {
continue;
}
diff --git a/resources/sql/autopatches/20180121.auth.03.accountdata.sql b/resources/sql/autopatches/20180121.auth.03.accountdata.sql
new file mode 100644
--- /dev/null
+++ b/resources/sql/autopatches/20180121.auth.03.accountdata.sql
@@ -0,0 +1,7 @@
+INSERT INTO {$NAMESPACE}_auth.auth_password
+ (objectPHID, phid, passwordType, passwordHash, passwordSalt, isRevoked,
+ dateCreated, dateModified)
+ SELECT phid, CONCAT('XACCOUNT', id), 'account', passwordHash, passwordSalt, 0,
+ dateCreated, dateModified
+ FROM {$NAMESPACE}_user.user
+ WHERE passwordHash != '';
diff --git a/resources/sql/autopatches/20180120.auth.04.vcsphid.php b/resources/sql/autopatches/20180121.auth.04.accountphid.php
copy from resources/sql/autopatches/20180120.auth.04.vcsphid.php
copy to resources/sql/autopatches/20180121.auth.04.accountphid.php
--- a/resources/sql/autopatches/20180120.auth.04.vcsphid.php
+++ b/resources/sql/autopatches/20180121.auth.04.accountphid.php
@@ -1,13 +1,15 @@
<?php
-// Populate VCS passwords (which we copied from the old "VCS Password" table
-// in the last migration) with new PHIDs.
+// Populate account passwords (which we copied from the user table in the last
+// migration) with new PHIDs.
$table = new PhabricatorAuthPassword();
$conn = $table->establishConnection('w');
+$password_type = PhabricatorAuthPasswordPHIDType::TYPECONST;
+
foreach (new LiskMigrationIterator($table) as $row) {
- if ($row->getPHID()) {
+ if (phid_get_type($row->getPHID()) == $password_type) {
continue;
}
diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
--- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php
+++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
@@ -61,6 +61,9 @@
$default_username = $account->getUsername();
$default_realname = $account->getRealName();
+ $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
+ $content_source = PhabricatorContentSource::newFromRequest($request);
+
$default_email = $account->getEmail();
if ($invite) {
@@ -285,27 +288,22 @@
if ($must_set_password) {
$value_password = $request->getStr('password');
$value_confirm = $request->getStr('confirm');
- if (!strlen($value_password)) {
- $e_password = pht('Required');
- $errors[] = pht('You must choose a password.');
- } else if ($value_password !== $value_confirm) {
- $e_password = pht('No Match');
- $errors[] = pht('Password and confirmation must match.');
- } else if (strlen($value_password) < $min_len) {
- $e_password = pht('Too Short');
- $errors[] = pht(
- 'Password is too short (must be at least %d characters long).',
- $min_len);
- } else if (
- PhabricatorCommonPasswords::isCommonPassword($value_password)) {
-
- $e_password = pht('Very Weak');
- $errors[] = pht(
- 'Password is pathologically weak. This password is one of the '.
- 'most common passwords in use, and is extremely easy for '.
- 'attackers to guess. You must choose a stronger password.');
- } else {
+
+ $password_envelope = new PhutilOpaqueEnvelope($value_password);
+ $confirm_envelope = new PhutilOpaqueEnvelope($value_confirm);
+
+ $engine = id(new PhabricatorAuthPasswordEngine())
+ ->setViewer($user)
+ ->setContentSource($content_source)
+ ->setPasswordType($account_type)
+ ->setObject($user);
+
+ try {
+ $engine->checkNewPassword($password_envelope, $confirm_envelope);
$e_password = null;
+ } catch (PhabricatorAuthPasswordException $ex) {
+ $errors[] = $ex->getMessage();
+ $e_password = $ex->getPasswordError();
}
}
@@ -408,8 +406,13 @@
$editor->createNewUser($user, $email_obj, $allow_reassign_email);
if ($must_set_password) {
- $envelope = new PhutilOpaqueEnvelope($value_password);
- $editor->changePassword($user, $envelope);
+ $password_object = PhabricatorAuthPassword::initializeNewPassword(
+ $user,
+ $account_type);
+
+ $password_object
+ ->setPassword($password_envelope, $user)
+ ->save();
}
if ($is_setup) {
diff --git a/src/applications/auth/controller/PhabricatorAuthSetPasswordController.php b/src/applications/auth/controller/PhabricatorAuthSetPasswordController.php
--- a/src/applications/auth/controller/PhabricatorAuthSetPasswordController.php
+++ b/src/applications/auth/controller/PhabricatorAuthSetPasswordController.php
@@ -40,8 +40,30 @@
return new Aphront404Response();
}
- $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
- $min_len = (int)$min_len;
+ $content_source = PhabricatorContentSource::newFromRequest($request);
+ $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
+
+ $password_objects = id(new PhabricatorAuthPasswordQuery())
+ ->setViewer($viewer)
+ ->withObjectPHIDs(array($viewer->getPHID()))
+ ->withPasswordTypes(array($account_type))
+ ->withIsRevoked(false)
+ ->execute();
+ if ($password_objects) {
+ $password_object = head($password_objects);
+ $has_password = true;
+ } else {
+ $password_object = PhabricatorAuthPassword::initializeNewPassword(
+ $viewer,
+ $account_type);
+ $has_password = false;
+ }
+
+ $engine = id(new PhabricatorAuthPasswordEngine())
+ ->setViewer($viewer)
+ ->setContentSource($content_source)
+ ->setPasswordType($account_type)
+ ->setObject($viewer);
$e_password = true;
$e_confirm = true;
@@ -50,46 +72,23 @@
$password = $request->getStr('password');
$confirm = $request->getStr('confirm');
- $e_password = null;
- $e_confirm = null;
-
- if (!strlen($password)) {
- $errors[] = pht('You must choose a password or skip this step.');
- $e_password = pht('Required');
- } else if (strlen($password) < $min_len) {
- $errors[] = pht(
- 'The selected password is too short. Passwords must be a minimum '.
- 'of %s characters.',
- new PhutilNumber($min_len));
- $e_password = pht('Too Short');
- } else if (!strlen($confirm)) {
- $errors[] = pht('You must confirm the selecetd password.');
- $e_confirm = pht('Required');
- } else if ($password !== $confirm) {
- $errors[] = pht('The password and confirmation do not match.');
- $e_password = pht('Invalid');
- $e_confirm = pht('Invalid');
- } else if (PhabricatorCommonPasswords::isCommonPassword($password)) {
- $e_password = pht('Very Weak');
- $errors[] = pht(
- 'The selected password is very weak: it is one of the most common '.
- 'passwords in use. Choose a stronger password.');
+ $password_envelope = new PhutilOpaqueEnvelope($password);
+ $confirm_envelope = new PhutilOpaqueEnvelope($confirm);
+
+ try {
+ $engine->checkNewPassword($password_envelope, $confirm_envelope, true);
+ $e_password = null;
+ $e_confirm = null;
+ } catch (PhabricatorAuthPasswordException $ex) {
+ $errors[] = $ex->getMessage();
+ $e_password = $ex->getPasswordError();
+ $e_confirm = $ex->getConfirmError();
}
if (!$errors) {
- $envelope = new PhutilOpaqueEnvelope($password);
-
- // This write is unguarded because the CSRF token has already
- // been checked in the call to $request->isFormPost() and
- // the CSRF token depends on the password hash, so when it
- // is changed here the CSRF token check will fail.
- $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-
- id(new PhabricatorUserEditor())
- ->setActor($viewer)
- ->changePassword($viewer, $envelope);
-
- unset($unguarded);
+ $password_object
+ ->setPassword($password_envelope, $viewer)
+ ->save();
// Destroy the token.
$auth_token->delete();
@@ -98,12 +97,15 @@
}
}
+ $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
+ $min_len = (int)$min_len;
+
$len_caption = null;
if ($min_len) {
$len_caption = pht('Minimum password length: %d characters.', $min_len);
}
- if ($viewer->hasPassword()) {
+ if ($has_password) {
$title = pht('Reset Password');
$crumb = pht('Reset Password');
$submit = pht('Reset Password');
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
@@ -110,20 +110,27 @@
pht('Very Weak'));
}
- if ($this->isRevokedPassword($password)) {
- throw new PhabricatorAuthPasswordException(
- pht(
- 'The password you entered has been revoked. You can not reuse '.
- 'a password which has been revoked. Choose a new password.'),
- pht('Revoked'));
- }
+ // If we're creating a brand new object (like registering a new user)
+ // and it does not have a PHID yet, it isn't possible for it to have any
+ // revoked passwords or colliding passwords either, so we can skip these
+ // checks.
- if (!$this->isUniquePassword($password)) {
- throw new PhabricatorAuthPasswordException(
- pht(
- 'The password you entered is the same as another password '.
- 'associated with your account. Each password must be unique.'),
- pht('Not Unique'));
+ if ($this->getObject()->getPHID()) {
+ if ($this->isRevokedPassword($password)) {
+ throw new PhabricatorAuthPasswordException(
+ pht(
+ 'The password you entered has been revoked. You can not reuse '.
+ 'a password which has been revoked. Choose a new password.'),
+ pht('Revoked'));
+ }
+
+ if (!$this->isUniquePassword($password)) {
+ throw new PhabricatorAuthPasswordException(
+ pht(
+ 'The password you entered is the same as another password '.
+ 'associated with your account. Each password must be unique.'),
+ pht('Not Unique'));
+ }
}
}
diff --git a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
--- a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
@@ -253,6 +253,7 @@
$request = $controller->getRequest();
$viewer = $request->getUser();
+ $content_source = PhabricatorContentSource::newFromRequest($request);
$require_captcha = false;
$captcha_valid = false;
@@ -285,22 +286,16 @@
if ($user) {
$envelope = new PhutilOpaqueEnvelope($request->getStr('password'));
- if ($user->comparePassword($envelope)) {
- $account = $this->loadOrCreateAccount($user->getPHID());
- $log_user = $user;
-
- // If the user's password is stored using a less-than-optimal
- // hash, upgrade them to the strongest available hash.
- $hash_envelope = new PhutilOpaqueEnvelope(
- $user->getPasswordHash());
- if (PhabricatorPasswordHasher::canUpgradeHash($hash_envelope)) {
- $user->setPassword($envelope);
+ $engine = id(new PhabricatorAuthPasswordEngine())
+ ->setViewer($user)
+ ->setContentSource($content_source)
+ ->setPasswordType(PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT)
+ ->setObject($user);
- $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
- $user->save();
- unset($unguarded);
- }
+ if ($engine->isValidPassword($envelope)) {
+ $account = $this->loadOrCreateAccount($user->getPHID());
+ $log_user = $user;
}
}
}
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
@@ -127,7 +127,7 @@
return PhabricatorPasswordHasher::comparePassword($digest, $hash);
}
- private function newPasswordEnvelope() {
+ public function newPasswordEnvelope() {
return new PhutilOpaqueEnvelope($this->getPasswordHash());
}
diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php
--- a/src/applications/people/editor/PhabricatorUserEditor.php
+++ b/src/applications/people/editor/PhabricatorUserEditor.php
@@ -132,33 +132,6 @@
/**
* @task edit
*/
- public function changePassword(
- PhabricatorUser $user,
- PhutilOpaqueEnvelope $envelope) {
-
- if (!$user->getID()) {
- throw new Exception(pht('User has not been created yet!'));
- }
-
- $user->openTransaction();
- $user->reload();
-
- $user->setPassword($envelope);
- $user->save();
-
- $log = PhabricatorUserLog::initializeNewLog(
- $this->requireActor(),
- $user->getPHID(),
- PhabricatorUserLog::ACTION_CHANGE_PASSWORD);
- $log->save();
-
- $user->saveTransaction();
- }
-
-
- /**
- * @task edit
- */
public function changeUsername(PhabricatorUser $user, $username) {
$actor = $this->requireActor();
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
@@ -263,28 +263,6 @@
PhabricatorPeopleUserPHIDType::TYPECONST);
}
- public function hasPassword() {
- return (bool)strlen($this->passwordHash);
- }
-
- public function setPassword(PhutilOpaqueEnvelope $envelope) {
- if (!$this->getPHID()) {
- throw new Exception(
- pht(
- 'You can not set a password for an unsaved user because their PHID '.
- 'is a salt component in the password hash.'));
- }
-
- if (!strlen($envelope->openEnvelope())) {
- $this->setPasswordHash('');
- } else {
- $this->setPasswordSalt(md5(Filesystem::readRandomBytes(32)));
- $hash = $this->hashPassword($envelope);
- $this->setPasswordHash($hash->openEnvelope());
- }
- return $this;
- }
-
public function getMonogram() {
return '@'.$this->getUsername();
}
@@ -332,36 +310,6 @@
return Filesystem::readRandomCharacters(255);
}
- public function comparePassword(PhutilOpaqueEnvelope $envelope) {
- if (!strlen($envelope->openEnvelope())) {
- return false;
- }
- if (!strlen($this->getPasswordHash())) {
- return false;
- }
-
- return PhabricatorPasswordHasher::comparePassword(
- $this->getPasswordHashInput($envelope),
- new PhutilOpaqueEnvelope($this->getPasswordHash()));
- }
-
- private function getPasswordHashInput(PhutilOpaqueEnvelope $password) {
- $input =
- $this->getUsername().
- $password->openEnvelope().
- $this->getPHID().
- $this->getPasswordSalt();
-
- return new PhutilOpaqueEnvelope($input);
- }
-
- private function hashPassword(PhutilOpaqueEnvelope $password) {
- $hasher = PhabricatorPasswordHasher::getBestHasher();
-
- $input_envelope = $this->getPasswordHashInput($password);
- return $hasher->getPasswordHashForStorage($input_envelope);
- }
-
const CSRF_CYCLE_FREQUENCY = 3600;
const CSRF_SALT_LENGTH = 8;
const CSRF_TOKEN_LENGTH = 16;
@@ -1669,6 +1617,32 @@
$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));
}
// For passwords which do not have some crazy legacy reason to use some
diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
--- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
@@ -26,6 +26,7 @@
public function processRequest(AphrontRequest $request) {
$user = $request->getUser();
+ $content_source = PhabricatorContentSource::newFromRequest($request);
$token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
$user,
@@ -40,6 +41,22 @@
// registration or password reset. If this flow changes, that flow may
// also need to change.
+ $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
+
+ $password_objects = id(new PhabricatorAuthPasswordQuery())
+ ->setViewer($user)
+ ->withObjectPHIDs(array($user->getPHID()))
+ ->withPasswordTypes(array($account_type))
+ ->withIsRevoked(false)
+ ->execute();
+ if ($password_objects) {
+ $password_object = head($password_objects);
+ } else {
+ $password_object = PhabricatorAuthPassword::initializeNewPassword(
+ $user,
+ $account_type);
+ }
+
$e_old = true;
$e_new = true;
$e_conf = true;
@@ -47,41 +64,42 @@
$errors = array();
if ($request->isFormPost()) {
$envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw'));
- if (!$user->comparePassword($envelope)) {
+
+ $engine = id(new PhabricatorAuthPasswordEngine())
+ ->setViewer($user)
+ ->setContentSource($content_source)
+ ->setPasswordType($account_type)
+ ->setObject($user);
+
+ if (!strlen($envelope->openEnvelope())) {
+ $errors[] = pht('You must enter your current password.');
+ $e_old = pht('Required');
+ } else if (!$engine->isValidPassword($envelope)) {
$errors[] = pht('The old password you entered is incorrect.');
$e_old = pht('Invalid');
+ } else {
+ $e_old = null;
}
$pass = $request->getStr('new_pw');
$conf = $request->getStr('conf_pw');
+ $password_envelope = new PhutilOpaqueEnvelope($pass);
+ $confirm_envelope = new PhutilOpaqueEnvelope($conf);
- if (strlen($pass) < $min_len) {
- $errors[] = pht('Your new password is too short.');
- $e_new = pht('Too Short');
- } else if ($pass !== $conf) {
- $errors[] = pht('New password and confirmation do not match.');
- $e_conf = pht('Invalid');
- } else if (PhabricatorCommonPasswords::isCommonPassword($pass)) {
- $e_new = pht('Very Weak');
- $e_conf = pht('Very Weak');
- $errors[] = pht(
- 'Your new password is very weak: it is one of the most common '.
- 'passwords in use. Choose a stronger password.');
+ try {
+ $engine->checkNewPassword($password_envelope, $confirm_envelope);
+ $e_new = null;
+ $e_conf = null;
+ } catch (PhabricatorAuthPasswordException $ex) {
+ $errors[] = $ex->getMessage();
+ $e_new = $ex->getPasswordError();
+ $e_conf = $ex->getConfirmError();
}
if (!$errors) {
- // This write is unguarded because the CSRF token has already
- // been checked in the call to $request->isFormPost() and
- // the CSRF token depends on the password hash, so when it
- // is changed here the CSRF token check will fail.
- $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-
- $envelope = new PhutilOpaqueEnvelope($pass);
- id(new PhabricatorUserEditor())
- ->setActor($user)
- ->changePassword($user, $envelope);
-
- unset($unguarded);
+ $password_object
+ ->setPassword($password_envelope, $user)
+ ->save();
$next = $this->getPanelURI('?saved=true');
@@ -93,11 +111,9 @@
}
}
- $hash_envelope = new PhutilOpaqueEnvelope($user->getPasswordHash());
- if (strlen($hash_envelope->openEnvelope())) {
+ if ($password_object->getID()) {
try {
- $can_upgrade = PhabricatorPasswordHasher::canUpgradeHash(
- $hash_envelope);
+ $can_upgrade = $password_object->canUpgrade();
} catch (PhabricatorPasswordHasherUnavailableException $ex) {
$can_upgrade = false;
@@ -154,7 +170,7 @@
$properties->addProperty(
pht('Current Algorithm'),
PhabricatorPasswordHasher::getCurrentAlgorithmName(
- new PhutilOpaqueEnvelope($user->getPasswordHash())));
+ $password_object->newPasswordEnvelope()));
$properties->addProperty(
pht('Best Available Algorithm'),

File Metadata

Mime Type
text/plain
Expires
Fri, Mar 21, 5:17 AM (14 h, 22 m ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7707812
Default Alt Text
D18903.id45319.diff (23 KB)

Event Timeline

Phabricator · Built by Phacility