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 @@ 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 @@ -129,33 +129,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 */ 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'),