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.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Nov 6 2018, 4:52 PM
epriestley edited the summary of this revision. (Show Details)Nov 6 2018, 4:53 PM
epriestley requested review of this revision.Nov 6 2018, 4:53 PM

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 accepted this revision.Nov 6 2018, 5:52 PM
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.