Page MenuHomePhabricator
Files F15433497

D19776.id.diff
No OneTemporary
Actions

D19776.id.diff
View Options

diff --git a/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php b/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php
--- a/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php
+++ b/src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php
@@ -99,6 +99,82 @@
$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
--- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
@@ -115,7 +115,9 @@
// 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 @@
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
--- a/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php
+++ b/src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php
@@ -6,4 +6,22 @@
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<string> 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
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -1665,5 +1665,23 @@
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;
+ }
+
}

File Metadata

Mime Type
text/plain
Expires
Tue, Mar 25, 11:30 PM (1 d, 23 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7712348
Default Alt Text
D19776.id.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility