Posted by jgalletta on July 22, 2011 at 9:10am
4 followers
| Project: | Password policy |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
The username constraint compare the submited password with the login in database and not with the login in form, which is not suitable:
- The constraint test always return OK on modify user form if the user change login and password although they are identical
- The constraint test always return OK on create user form as there is no login in database to compare with
So basically this username constraint functionality works only in the specific case of user changing only his pasword.
Comments
#1
yeah, the api didn't have a way to support this. I've modified the constraint API to use
<?phpfunction ...($constraint, $account) {
?>
Instead of:
<?phpfunction ...($pass, $constraint, $uid) {
?>
uid and pass are still available because we add them to the account object before we pass it into validation, but now we also have the username as well, so we can check and see if the new password matches the new (or current) username. We also avoid a couple user_loads() as a result. I've tested this locally and it seems to work well.
#2
Ah, a patch file would help :)
#3
I agree this seems like a fundamental problem with this constraint. Unfortunately I don't think an API change for the 6.x version is appropriate at this point.
#4
This seems like a security issue. If you have the username constraint enabled, you expect that passwords will never be the username. Since the constraint does not actually work in cases, you are misled into thinking your accounts are secure against unauthorized access by attackers guessing the username as the password.
I think this constraint should be removed from Password Policy 6.x-1.x since the API does not correctly support it and it gives a false impression of security.
#5
I agree, this feature simply doesn't work, it should be removed or corrected, the patch to make this works is not very complicated.
The purpose of this module is security, and this issue is clearly a security issue.
#6
I agree that this is a functionality issue, that leads to lessened security. I think this should likely be fixed in place, however, as this module is still widely used in D6 and should not be dropped after release.