Page MenuHomePhabricator

Prevent users from selecting excessively bad passwords based on their username or email address
ClosedPublic

Authored by epriestley on Nov 6 2018, 4:52 PM.
Tags
None
Referenced Files
F13034725: D19776.diff
Sat, Apr 13, 6:35 PM
F13034685: D19776.diff
Sat, Apr 13, 6:08 PM
Unknown Object (File)
Thu, Apr 11, 4:10 AM
Unknown Object (File)
Thu, Mar 28, 3:40 AM
Unknown Object (File)
Tue, Mar 19, 3:44 PM
Unknown Object (File)
Tue, Mar 19, 3:44 PM
Unknown Object (File)
Tue, Mar 19, 3:43 PM
Unknown Object (File)
Tue, Mar 19, 3:43 PM
Subscribers
None

Details

Summary

Ref T13216. We occasionally receive HackerOne reports concerned that you can select your username as a password. I suspect very few users actually do this and that this is mostly a compliance/checklist sort of issue, not a real security issue.

On this install, we have about 41,000 user accounts. Of these, 100 have their username as a password (account or VCS). A substantial subset of these are either explicitly intentional ("demo", "bugmenot") or obvious test accounts ("test" in name, or name is a nonsensical string of gibberish, or looks like "tryphab" or similar) or just a bunch of numbers (?), or clearly a "researcher" doing this on purpose (e.g., name includes "pentest" or "xss" or similar).

So I'm not sure real users are actually very inclined to do this, and we can't really ever stop them from picking awful passwords anyway. But we can stop researchers from reporting that this is an issue.

Don't allow users to select passwords which contain words in a blocklist: their username, real name, email addresses, or the install's domain name. These words also aren't allowed to contain the password (that is, neither your password nor your username may be a substring of the other one). We also do a little normalization to try to split apart email addresses, domains, and real names, so I can't have "evan1234" as my password.

Test Plan
  • Added unit tests and made them pass.
  • Tried to set my password to a bunch of variations of my username / email / domain name / real name / etc, got rejected.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I used this script to identify accounts with the account name as a password:

<?php

require_once 'scripts/init/init-script.php';

foreach (new LiskMigrationIterator(new PhabricatorUser()) as $user) {
  $passwords = id(new PhabricatorAuthPassword())->loadAllWhere(
    'objectPHID = %s',
    $user->getPHID());

  $envelope = new PhutilOpaqueEnvelope($user->getUsername());
  foreach ($passwords as $password) {
    if ($password->comparePassword($envelope, $user)) {
      echo $user->getUsername()."\n";
      break;
    }
  }
}
amckinley added inline comments.
src/applications/auth/password/PhabricatorAuthPasswordHashInterface.php
10–11

A++++ sentence construction; would buy again.

This revision is now accepted and ready to land.Nov 6 2018, 5:52 PM
This revision was automatically updated to reflect the committed changes.