Differential D19776

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

Authored by epriestley on Nov 6 2018, 4:52 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13216: 2018 Week 45-47 Bonus Content

Commits

rP1f6a4cfffe58: Prevent users from selecting excessively bad passwords based on their username…

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

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

Harbormaster completed remote builds in B21086: Diff 47237.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;
    }
  }
}