Problem/Motivation

Using multibyte characters in a password results in a miscalculated length, potentially leading to passwords not meeting length constraints.

E.g. for the password abc£, strlen() returns the length 5. If the minimum password length was 5 then this password would pass even though it is only 4 characters long.

Steps to reproduce

  • Set up a password policy with a length constraint.
  • Use one multibyte character in a password that appears to be one character shorter than the constraint.
  • Password is allowed when it shouldn't be.

Proposed resolution

  • Replace strlen() with mb_strlen()

Remaining tasks

  • Create test for this scenario
  • Create patch implementing fix

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork password_policy-3366875

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

justanothermark created an issue. See original summary.

justanothermark opened merge request !59

justanothermark’s picture

justanothermark commented
Status: Active » Needs review

Added patch which:

  • Adds a Unit test case for this issue to \Drupal\Tests\password_policy_length\Unit\PasswordLengthTest::lengthDataProvider()
  • Replaces the use of strlen() with mb_strlen() in \Drupal\password_policy_length\Plugin\PasswordConstraint\PasswordLength::validate()
justanothermark’s picture

justanothermark commented
Issue summary: View changes
alina.basarabeanu’s picture

alina.basarabeanu commented
Status: Needs review » Reviewed & tested by the community

Changes tested on Drupal Version 9.5.10 and Password Policy 4.0.0.
The minimum password length is set to 5 characters and aaa$ or tes£ passwords are failing the validation.
Without the patch those passwords are valid.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: Unassigned » kristen pol

Assigning to myself as I'm reviewing/merging ready RTBC fixes/updates over the next few days.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Reviewed & tested by the community » Needs work

Looks like we are missing a couple of places, but I can update these:

  public function validatePassword(string $password, UserInterface $user, array $edited_user_roles = []): PasswordPolicyValidationReport {
    // Stop before policy-based validation if password exceeds maximum length.
    if (strlen($password) > PasswordInterface::PASSWORD_MAX_LENGTH) {
      return TRUE;
    }

UPDATED: Looking at the code, I don't think this is necessary as it's removing zero character items from the array.

and perhaps: $blacklisted_passwords = array_filter($blacklisted_passwords, 'strlen');

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs work » Needs review

I'll test this.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Actually, I think I'm wrong here. Core is using strlen:

core/lib/Drupal/Core/Password/PhpPassword.php:    if (strlen($password) > static::PASSWORD_MAX_LENGTH

so I'll change it back and add a comment.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Reviewed & tested by the community

Tested both min and max and it works as expected.

Since I didn't add logic, just a comment, back to RTBC. I'll get this merged.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: kristen pol » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks everyone for the help on this issue. The fix has been merged and will be part of the next release.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Version: 4.0.0 » 4.0.x-dev

This is part of the new 4.0.1 release.