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

[goblinJoel]

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!

[Tim Graham]

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

[Sasha Romijn]

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

[goblinJoel]

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!

[Tim Graham]

Good point, I updated the docs in the PR.

[Tim Graham <timograham@…>]

In 0d9ff873:

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

Thanks goblinJoel for the report and feedback.

[goblinJoel]

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.

[Tim Graham]

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.