The password validation and updating the password history only works for a form submit, because it has registered it as form handlers. This means that when a user password gets updated via a different channel (cron, web service, etc.), which triggers the user update hook, the password policy doesn't work.

This is a critical bug in our system, because it allows users to circumvent the password policy in certain scenarios! Could you please fix this and move the validation and update logic into the user update hook?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AohRveTPV’s picture

Assigned: Unassigned » AohRveTPV

It's funny that you posted this, because I have known about this problem for a while, but just yesterday wrote a fix. Need to run tests, then I will post.

AohRveTPV’s picture

Actually I thought this problem was only for password history: If a user is updated with user_save() (e.g., via drush user-password), the new password is not stored to history. The patch I developed will only fix that.

It may not be possible to have all constraints apply to passwords updated by programmatic means. For instance, user_save() hashes the plaintext password before any hooks are called, so there is no opportunity for Password Policy to apply constraints as it can only see the hashed password. The only solution to that would seem to be a core User module change. Do you see another way to solve this?

AohRveTPV’s picture

A possible hack may be to use a custom password .inc to inject code to check Password Policy constraints as part of a call to user_hash_password(). But that seems slightly crazy.

AohRveTPV’s picture

Possible approach to solving this issue:
1. Warn administrators in the documentation that password policies will only apply to password changes via form submissions.
2. Push for a core change that would allow modules to see the plaintext password when a user is updated programmatically.
3. Package a core patch with the module that could be manually applied by administrators who need password policies to be applied for user updates not via form submission. (Some other modules like Drupal 6 SimpleTest and Secure Password Hashes take this approach of bundling a core patch.)

Again, if you can think of a better approach than changing core, please share.

dustinmoris’s picture

You are right. It is poor of the core user module to overwrite the $edit['pass'] with the hashed value. Implementing a custom password.inc would cause a lot of compatibility issues with users, who have already implemented their own password.inc.

I know that some websites have done it to move to a different implementation, because the Drupal hashing algorithm is kind of custom and hasn't been evaluated and stamped as secure as PBKDF2 or Blowfish.

I think ideal would be to try to convince Drupal Core to apply a small patch. Something like this one line:

$edit['pass-submitted'] = $edit['pass']; // <--- Add this one line above the one where the password gets hashed
$edit['pass'] = user_hash_password(trim($edit['pass']));

This change is extremely small and would not break anything else as it is a defensive change by introducing a new array element. I'd be surprised if they would decline this change.

From here it would be easy. You could pick any of the hooks, probably hook_user_presave would be a good start.

What do you think? Have you had good experiences with requesting patches on Drupal Core?

Offtopic:
It is kind of funny to see that before the password gets hashed they decided to trim it. I don't understand why, because it means that they have discriminated the space, which is a normal valid character from all passwords. It doesn't matter since it gets hashed anyway. Really weird some stuff. I would call this a bug, because when I enter " hello-world " as my password I will not know what to do to log in afterwards.

AohRveTPV’s picture

It is kind of funny to see that before the password gets hashed they decided to trim it. I don't understand why, because it means that they have discriminated the space, which is a normal valid character from all passwords. It doesn't matter since it gets hashed anyway. Really weird some stuff. I would call this a bug, because when I enter " hello-world " as my password I will not know what to do to log in afterwards.

I think the password gets trimmed too at login, so login would still work. It may have some other implications though. Check out #1921576: Don't trim whitespace when setting user password.

AohRveTPV’s picture

Implementing a custom password.inc would cause a lot of compatibility issues with users, who have already implemented their own password.inc.

Yes, that is a problem. My thought was that Password Policy could act as a shim (if that's the right term) and pass calls on to any custom password.inc that was set. Maybe like:

$password_inc = variable_get('password_inc', 'includes/password.inc');
if ($password_inc != PASSWORD_POLICY_SHIM_PASSWORD_INC)) {
  variable_set('password_policy_password_inc', $password_inc);
  variable_set('password_inc', PASSWORD_POLICY_SHIM_PASSWORD_INC);
}

Not sure if it would work.

A core change like you suggested is definitely the better solution. Core changes normally have to go into D8 first and then be backported to D7, right. (Not sure whether D8 has this problem.) I am not optimistic on quickly getting a change into D7 core, but we should pursue it.

AohRveTPV’s picture

Another difficulty with this is error handling. If user_save() is called to change the password, but the password fails to meet constraints, FALSE can be returned, but how does the caller know why the call failed?

Any ideas?

I guess the code could try to determine the context of the call. For example, if it is due to a Drush command, set a Drush error. That may not always be possible, though.

AohRveTPV’s picture

Version: 7.x-1.10 » 7.x-1.x-dev
Status: Active » Needs review
FileSize
831 bytes

I would like to suggest that we document this as a limitation of the module, and treat this as a feature request rather than a bug. Passwords normally get set only through the web interface. It is only when doing something custom, like using Drush or web services, that passwords are changed programmatically. (Please correct me if wrong.)

The limitation could be documented on the project page and in README.txt. Contributions to removing this limitation would be invited. Here is a proposed update to README.txt. Similar text would also go on the project page.

AohRveTPV’s picture

Category: Bug report » Feature request
Priority: Critical » Major
Status: Needs review » Active

Moving forward with approach mentioned in #9. Documented limitation on project page and in README.txt of all release branches:

http://drupalcode.org/project/password_policy.git/commit/14bb463
http://drupalcode.org/project/password_policy.git/commit/f37dadf
http://drupalcode.org/project/password_policy.git/commit/3ad13bc

purushotam.rai’s picture

Hi @AohRveTPV,

Here hoes a patch which resolves this issue. The patch basically gives preference to user account found through route match over current user and hence works fine in both the cases.

Thanks and Regards

purushotam.rai’s picture

Status: Active » Needs review
purushotam.rai’s picture

Version: 7.x-1.x-dev » 8.x-3.x-dev

Status: Needs review » Needs work

The last submitted patch, 11: password-policy-updating-user-2451159.patch, failed testing. View results

purushotam.rai’s picture

Status: Needs work » Needs review
AohRveTPV’s picture

purushotam.rai,

Thanks for your patch. Looking at it, I think it is trying to solve a different problem than this issue covers.

This issue: If you change a user's password in a way that does not use a password form (e.g., via web services), password policies are not applied.

I think your patch may actually be solving the general problem of #2497923: Wrong user used when administrator changing another user's password for 8.x-3.x. Am I correct? Thanks again.

urashima82’s picture

Hello,

Here is a patch for 8.x-3.0-beta1.

Regards.

Status: Needs review » Needs work

The last submitted patch, 17: password-policy-updating-user-2451159-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

urashima82’s picture

Adding small fixe to show policy.

darvanen’s picture

#2: If you implement these plugins as constraints on the password field they will be called during ::validate which precedes ::save and should allow inspection of the raw password.

#9: I disagree. More and more decoupled/headless applications are being built using Drupal and need API endpoints to behave the way the rest of Drupal does.

#16: I agree, the patches here so far are not addressing this issue as written.

I would note that re-writing the plugin system to be used as constraints is a *major* overhaul, and by its very nature (turning on password validation for API endpoints that use validation) would create some non-BC changes that may be best handled by creating a new version (4.x).

As this module doesn't do it, I'm going to write some custom constraints for my current project to implement the rules I want. I will report back here regarding whether constraints work or not.

darvanen’s picture

Yes. Constraints work well for forms and API endpoints alike and are easy to add to the password field like this:

/**
 * Implements hook_entity_base_field_info_alter().
 */
function im_users_entity_base_field_info_alter(&$fields, \Drupal\Core\Entity\EntityTypeInterface $entity_type) {
  if ($entity_type->id() === 'user') {
    /** @var \Drupal\Core\Field\BaseFieldDefinition $passwordField */
    $passwordField = $fields['pass'];
    $passwordField->addConstraint('PasswordPolicyConstraintName');
  }
}

Writing a custom one took all of 5 minutes with drush's generate function, but I have no doubt creating a system that connects constraints with the current plugins would take a fair bit longer.

darvanen’s picture

Using constraints would solve #2933746: Integration with change_pwd_page and likely other issues too. Perhaps a parent issue is in order.

Andriy Khomych’s picture

Totally agree with #21, #22 comments.

However, why not introduce only one constraint, add a new method in PasswordPolicyValidator to validate passwords using internal password policy plugins and return here validation messages. Then use in this constraint new validator method and show messages? IMHO, it would be the easiest fix. I'm in process of building something similar and as far as I see it works.

Andriy Khomych’s picture

Somehow, cannot fork a project in GitLab - 'An error occurred while forking the project. Please try again.'
So, used an old way, attached a patch. It is without tests but could be a start point.

Andriy Khomych’s picture

FileSize
5.49 KB

Fix typo in patch.

darvanen’s picture

#23: Of course /facepalm that's a much better idea!

Obviously this patch is a WIP because it will need to remove the current non-Constraint implementation but it'd definitely a step in the right direction, lovely!

And happily, it shouldn't change any of the behaviour beyond maybe changing how the message is displayed and highlighting the password field(s) so we shouldn't need much test coverage.

I propose a change of title for this issue to "Use constraints to validate passwords to support APIs" but I don't want to just update it without some other input.

Also I still think a change like this calls for a new major version.

Andriy Khomych’s picture

#26, thanks. Only I think it isn't required to remove now form validation.
Generally, it doesn't matter and it runs before constraints.
IMHO, it could be marked as todo/depricated and removed in a few steps within adjusting tests.
As for the new major version, it is up to the module authors to decide about it, still, it isn't a major change in the module API.

darvanen’s picture

#27: Fair enough about the form validation :)

My call to make it a new major version is based on this change specifically altering behaviour for user updates, which is a BC-breaking change that will likely affect a majority of sites in some way.
(It also will start functioning on all API providers (REST, JSON:API, graphQL etc). However, I suspect that like me, most people will have seen that this module didn't support APIs and went looking elsewhere for a solution, so I guess that will mitigate that risk somewhat).

So, with 25k sites using the 8.x version of this module, the safest thing to do is release a new version. If not, at least a minor version with a well written release and warnings wherever they can be placed.

Anyway that's a decision for the maintainers :)

Andriy Khomych’s picture

#28 Totally agree.
Found some issues after applying the patch, probably, will add updates soon.

Andriy Khomych’s picture

Updated patch - fix an issue when it isn't possible to cancel user account.

darvanen’s picture

It really helps reviewers if you provide an interdiff :)

Andriy Khomych’s picture

Agree, forgot about it, will check it later.