diff --git a/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php b/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php index 6ac616b2a1..794b0b5e22 100644 --- a/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php +++ b/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php @@ -99,6 +99,82 @@ final class PhabricatorAuthPasswordTestCase extends PhabricatorTestCase { $this->assertTrue($account_engine->isUniquePassword($password2)); } + public function testPasswordBlocklisting() { + $user = $this->generateNewTestUser(); + + $user + ->setUsername('iasimov') + ->setRealName('Isaac Asimov') + ->save(); + + $test_type = PhabricatorAuthPassword::PASSWORD_TYPE_TEST; + $content_source = $this->newContentSource(); + + $engine = id(new PhabricatorAuthPasswordEngine()) + ->setViewer($user) + ->setContentSource($content_source) + ->setPasswordType($test_type) + ->setObject($user); + + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('account.minimum-password-length', 4); + + $passwords = array( + 'a23li432m9mdf' => true, + + // Empty. + '' => false, + + // Password length tests. + 'xh3' => false, + 'xh32' => true, + + // In common password blocklist. + 'password1' => false, + + // Tests for the account identifier blocklist. + 'isaac' => false, + 'iasimov' => false, + 'iasimov1' => false, + 'asimov' => false, + 'iSaAc' => false, + '32IASIMOV' => false, + 'i-am-iasimov-this-is-my-long-strong-password' => false, + 'iasimo' => false, + + // These are okay: although they're visually similar, they aren't mutual + // substrings of any identifier. + 'iasimo1' => true, + 'isa1mov' => true, + ); + + foreach ($passwords as $password => $expect) { + $this->assertBlocklistedPassword($engine, $password, $expect); + } + } + + private function assertBlocklistedPassword( + PhabricatorAuthPasswordEngine $engine, + $raw_password, + $expect_valid) { + + $envelope_1 = new PhutilOpaqueEnvelope($raw_password); + $envelope_2 = new PhutilOpaqueEnvelope($raw_password); + + $caught = null; + try { + $engine->checkNewPassword($envelope_1, $envelope_2); + } catch (PhabricatorAuthPasswordException $exception) { + $caught = $exception; + } + + $this->assertEqual( + $expect_valid, + !($caught instanceof PhabricatorAuthPasswordException), + pht('Validity of password "%s".', $raw_password)); + } + + public function testPasswordUpgrade() { $weak_hasher = new PhabricatorIteratedMD5PasswordHasher(); diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php index 067cea30e7..f763b0987f 100644 --- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php @@ -115,7 +115,9 @@ final class PhabricatorAuthPasswordEngine // revoked passwords or colliding passwords either, so we can skip these // checks. - if ($this->getObject()->getPHID()) { + $object = $this->getObject(); + + if ($object->getPHID()) { if ($this->isRevokedPassword($password)) { throw new PhabricatorAuthPasswordException( pht( @@ -132,6 +134,66 @@ final class PhabricatorAuthPasswordEngine pht('Not Unique')); } } + + // Prevent use of passwords which are similar to any object identifier. + // For example, if your username is "alincoln", your password may not be + // "alincoln", "lincoln", or "alincoln1". + $viewer = $this->getViewer(); + $blocklist = $object->newPasswordBlocklist($viewer, $this); + + // Smallest number of overlapping characters that we'll consider to be + // too similar. + $minimum_similarity = 4; + + // Add the domain name to the blocklist. + $base_uri = PhabricatorEnv::getAnyBaseURI(); + $base_uri = new PhutilURI($base_uri); + $blocklist[] = $base_uri->getDomain(); + + // Generate additional subterms by splitting the raw blocklist on + // characters like "@", " " (space), and "." to break up email addresses, + // readable names, and domain names into components. + $terms_map = array(); + foreach ($blocklist as $term) { + $terms_map[$term] = $term; + foreach (preg_split('/[ @.]/', $term) as $subterm) { + $terms_map[$subterm] = $term; + } + } + + // Skip very short terms: it's okay if your password has the substring + // "com" in it somewhere even if the install is on "mycompany.com". + foreach ($terms_map as $term => $source) { + if (strlen($term) < $minimum_similarity) { + unset($terms_map[$term]); + } + } + + // Normalize terms for comparison. + $normal_map = array(); + foreach ($terms_map as $term => $source) { + $term = phutil_utf8_strtolower($term); + $normal_map[$term] = $source; + } + + // Finally, make sure that none of the terms appear in the password, + // and that the password does not appear in any of the terms. + $normal_password = phutil_utf8_strtolower($raw_password); + if (strlen($normal_password) >= $minimum_similarity) { + foreach ($normal_map as $term => $source) { + if (strpos($term, $normal_password) === false && + strpos($normal_password, $term) === false) { + continue; + } + + throw new PhabricatorAuthPasswordException( + pht( + 'The password you entered is very similar to a nonsecret account '. + 'identifier (like a username or email address). Choose a more '. + 'distinct password.'), + pht('Not Distinct')); + } + } } public function isValidPassword(PhutilOpaqueEnvelope $envelope) { diff --git a/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php b/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php index 36a296b209..af20db3ed4 100644 --- a/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php +++ b/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php @@ -6,4 +6,22 @@ interface PhabricatorAuthPasswordHashInterface { PhutilOpaqueEnvelope $envelope, PhabricatorAuthPassword $password); + /** + * Return a list of strings which passwords associated with this object may + * not be similar to. + * + * This method allows you to prevent users from selecting their username + * as their password or picking other passwords which are trivially similar + * to an account or object identifier. + * + * @param PhabricatorUser The user selecting the password. + * @param PhabricatorAuthPasswordEngine The password engine updating a + * password. + * @return list Blocklist of nonsecret identifiers which the password + * should not be similar to. + */ + public function newPasswordBlocklist( + PhabricatorUser $viewer, + PhabricatorAuthPasswordEngine $engine); + } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index cd85800ab1..e2a0f9a2a2 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -1665,5 +1665,23 @@ final class PhabricatorUser return new PhutilOpaqueEnvelope($digest); } + public function newPasswordBlocklist( + PhabricatorUser $viewer, + PhabricatorAuthPasswordEngine $engine) { + + $list = array(); + $list[] = $this->getUsername(); + $list[] = $this->getRealName(); + + $emails = id(new PhabricatorUserEmail())->loadAllWhere( + 'userPHID = %s', + $this->getPHID()); + foreach ($emails as $email) { + $list[] = $email->getAddress(); + } + + return $list; + } + }