Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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?
Comments
Comment #1
AohRveTPV CreditAttribution: AohRveTPV commentedIt'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.
Comment #2
AohRveTPV CreditAttribution: AohRveTPV commentedActually I thought this problem was only for password history: If a user is updated with
user_save()
(e.g., viadrush 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?
Comment #3
AohRveTPV CreditAttribution: AohRveTPV commentedA 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.
Comment #4
AohRveTPV CreditAttribution: AohRveTPV commentedPossible 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.
Comment #5
dustinmoris CreditAttribution: dustinmoris commentedYou 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:
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.
Comment #6
AohRveTPV CreditAttribution: AohRveTPV commentedI 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.
Comment #7
AohRveTPV CreditAttribution: AohRveTPV commentedYes, 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:
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.
Comment #8
AohRveTPV CreditAttribution: AohRveTPV commentedAnother 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.
Comment #9
AohRveTPV CreditAttribution: AohRveTPV commentedI 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.
Comment #10
AohRveTPV CreditAttribution: AohRveTPV commentedMoving 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
Comment #11
purushotam.rai CreditAttribution: purushotam.rai at QED42 commentedHi @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
Comment #12
purushotam.rai CreditAttribution: purushotam.rai at QED42 commentedComment #13
purushotam.rai CreditAttribution: purushotam.rai at QED42 commentedComment #15
purushotam.rai CreditAttribution: purushotam.rai at QED42 commentedComment #16
AohRveTPV CreditAttribution: AohRveTPV commentedpurushotam.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.
Comment #17
urashima82 CreditAttribution: urashima82 as a volunteer commentedHello,
Here is a patch for 8.x-3.0-beta1.
Regards.
Comment #19
urashima82 CreditAttribution: urashima82 as a volunteer commentedAdding small fixe to show policy.
Comment #20
darvanen#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.
Comment #21
darvanenYes. Constraints work well for forms and API endpoints alike and are easy to add to the password field like this:
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.
Comment #22
darvanenUsing constraints would solve #2933746: Integration with change_pwd_page and likely other issues too. Perhaps a parent issue is in order.
Comment #23
Andriy Khomych CreditAttribution: Andriy Khomych as a volunteer commentedTotally 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.
Comment #24
Andriy Khomych CreditAttribution: Andriy Khomych as a volunteer and at Drupal Ukraine Community commentedSomehow, 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.
Comment #25
Andriy Khomych CreditAttribution: Andriy Khomych as a volunteer commentedFix typo in patch.
Comment #26
darvanen#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.
Comment #27
Andriy Khomych CreditAttribution: Andriy Khomych as a volunteer commented#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.
Comment #28
darvanen#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 :)
Comment #29
Andriy Khomych CreditAttribution: Andriy Khomych as a volunteer commented#28 Totally agree.
Found some issues after applying the patch, probably, will add updates soon.
Comment #30
Andriy Khomych CreditAttribution: Andriy Khomych as a volunteer commentedUpdated patch - fix an issue when it isn't possible to cancel user account.
Comment #31
darvanenIt really helps reviewers if you provide an interdiff :)
Comment #32
Andriy Khomych CreditAttribution: Andriy Khomych as a volunteer commentedAgree, forgot about it, will check it later.
Comment #33
Kristen PolThanks to everyone for your work on this issue.
As the 8.x is no longer supported, I'm postponing this issue for now and need feedback as to whether or not this change is relevant to 4.0.x. If it is, please reopen and change the version and reroll the patch if necessary. If it's not, please closed as "won't fix".
If there is no response to this in a month addressing the above, it can be closed as "won't fix".