Page MenuHomePhorge

Register New Account: unsafe password check not working correctly?
Open, Needs TriagePublic

Description

I think I bumped into a bug which became visible with PHP8.2
When you click on the "Register New Account" button in the login dialog, a crash will occur on the preg_split call below:

PhabricatorAuthPasswordEngine.php @138:
// 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;
  }
}

Apparently, $blocklist is an array of 3 items for which the first 2 items contain NULL and the 3rd one contains the domain name.
Because of this $term is also NULL at some point.

$blocklist is initialized by means of the newPasswordBlocklist method, which shows that the 2 first elements should contain UserName and RealName:

PhabricatorUser.php @1464
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;
}

Username and RealName seems to get their data from $parameters, which is only been set by means of a conduit call, which I think never happens (and $parameters remains null):

ArcanistUserRef.php
<?php

final class ArcanistUserRef
  extends ArcanistRef {

  private $parameters;

  public function getRefDisplayName() {
    return pht('User "%s"', $this->getMonogram());
  }

  public static function newFromConduit(array $parameters) {
    $ref = new self();
    $ref->parameters = $parameters;
    return $ref;
  }

  public static function newFromConduitWhoami(array $parameters) {
    // NOTE: The "user.whoami" call returns a different structure than
    // "user.search". Mangle the data so it looks similar.

    $parameters['fields'] = array(
      'username' => idx($parameters, 'userName'),
      'realName' => idx($parameters, 'realName'),
    );

    return self::newFromConduit($parameters);
  }

  public function getID() {
    return idx($this->parameters, 'id');
  }

  public function getPHID() {
    return idx($this->parameters, 'phid');
  }

  public function getMonogram() {
    return '@'.$this->getUsername();
  }

  public function getUsername() {
    return idxv($this->parameters, array('fields', 'username'));
  }

  public function getRealName() {
    return idxv($this->parameters, array('fields', 'realName'));
  }

  protected function buildRefView(ArcanistRefView $view) {
    $real_name = $this->getRealName();
    if (strlen($real_name)) {
      $real_name = sprintf('(%s)', $real_name);
    }

    $view
      ->setObjectName($this->getMonogram())
      ->setTitle($real_name);
  }


}

I also tried this on a Phabricator instance with PHP 7.3 and I get the same result: the first 2 elements are also null.
I was also able to register a user, whose password is the same as his username.