Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#27467 closed Bug (fixed)

UserAttributeSimilarityValidator max_similarity=0/1 doesn't work as documented

Reported by: goblinJoel Owned by: nobody
Component: contrib.auth Version: 1.9
Severity: Normal Keywords: password management validation validator
Cc: Sasha Romijn Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While this has to do with configuring password validation, it doesn't strike me as a security vulnerability per se, so I'm posting in the standard bug tracker:

The documentation for UserAttributeSimilarityValidator states,

"The maximum similarity the password can have, before it is rejected, can be set with the max_similarity parameter, on a scale of 0 to 1. A setting of 0 will cause all passwords to be rejected, whereas a setting of 1 will cause it to only reject passwords that are identical to an attribute’s value."

However, if you set it to 1, it will never reject a password, even when identical to a user attribute (for example, email). The code checks whether the similarity is > the max_similarity, rather than >= as the documentation describes. The documentation should be updated to reflect this.

I found this on Django 1.9.11 while testing that I'd installed password validators correctly: when I tried to change a test user's password (via the admin) to the same as their email, it allowed it. If I changed max_similarity to 1.0 instead of 1, I got the same behavior. If I changed it to 0.9, it rejected the email as password, as it should.

The documentation for the version I'm using: https://docs.djangoproject.com/en/1.9/topics/auth/passwords/#django.contrib.auth.password_validation.UserAttributeSimilarityValidator

The source code for the validator: https://docs.djangoproject.com/en/1.9/_modules/django/contrib/auth/password_validation/#UserAttributeSimilarityValidator

I checked the docs and source code for 1.10 and dev, and they appear to have the same issue. I haven't submitted a bug here before, so I hope I've done everything correctly!

Change History (7)

comment:1 by Tim Graham, 8 years ago

Cc: Sasha Romijn added
Component: Documentationcontrib.auth
Easy pickings: unset
Has patch: set
Summary: Documentation inaccurate for password validator UserAttributeSimilarityValidatorUserAttributeSimilarityValidator max_similarity=0/1 doesn't work as documented
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Is there a reason not to fix the code to match the documentation? PR

comment:2 by Sasha Romijn, 8 years ago

Fixing the code seems like the right approach to me too. This was an oversight when I originally wrote the code.

in reply to:  2 comment:3 by goblinJoel, 8 years ago

Replying to Erik Romijn:

Fixing the code seems like the right approach to me too. This was an oversight when I originally wrote the code.

Makes sense to me. In fact, now that I think about it, that would make a max_similarity of 0 behave as described, as well. As-is, using >, assuming a computed similarity of 0 is possible, a password with no similarity must get accepted, because 0 is not greater than 0. Using >=, it would be rejected, because 0 is greater than or equal to 0. I think the PR noticed the same thing.

However, the documentation may need to be updated that way, too:

The maximum similarity the password can have, before it is rejected, can be set with the max_similarity parameter, on a scale of 0 to 1. A setting of 0 will cause all passwords to be rejected, whereas a setting of 1 will cause it to only reject passwords that are identical to an attribute’s value.

If the code switches to >=, max_similarity no longer represents the maximum similarity before rejection. It will instead be the minimum similarity for rejection. I assume this isn't worth renaming the parameter over, and maybe the 0 and 1 examples make it clear, but it seemed worth noting.

PS: Wow! Did not expect to get a response this quickly!

comment:4 by Tim Graham, 8 years ago

Good point, I updated the docs in the PR.

comment:5 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 0d9ff873:

Fixed #27467 -- Made UserAttributeSimilarityValidator max_similarity=0/1 work as documented.

Thanks goblinJoel for the report and feedback.

comment:6 by goblinJoel, 8 years ago

No problem. Thanks for fixing it!

I'm not familiar with how fixes make their way to releases. Is this something for the next minor version, or will it be in a bugfix for prior versions (e.g., 1.9.x)? No big deal for me either way; workaround was trivial in my case.

comment:7 by Tim Graham, 8 years ago

If we had discovered the issue before the release of Django 1.10, it may have qualified for a backport as a bug in a new feature, but at this point, the fix will go to 1.11. See the supported versions policy for details.

Note: See TracTickets for help on using tickets.
Back to Top