`UserAttributeSimilarityValidator` uses unordered string comparison

[Terence Honles]

UserAttributeSimilarityValidator only uses the SequenceMatcher.quick_ratio method (which uses an unordered comparison of two strings and is only intended to be used as an upper bound)

This causes what I would consider "false positives" for strings that are not actually similar (any anagram will produce a ratio of 1 (exactly equal)).

I discovered this because we had a flaky test in our CI (with a user having attributes generated using Faker) and I finally spent some time to investigate the flaky test. The two strings (lowercased) were "andrews" and "new-password", which lead me to wonder how those could be considered 70% similar.

I have added a test with the anagrams: "enumerations" & "mountaineers"

[Terence Honles]

The SequenceMatcher's quick_ratio method has been used since the validators were added to Django in v1.9 (1daae25bdcd735151de394a5578c22257e3e5dc7), and since my PR was automatically closed, here's the same patch

[SofianeB]

Can I work on this issue ?

[Mike Edmunds]

SofianeB the ticket has not yet been accepted, and the original reporter has already claimed the ticket and created a PR for if it is accepted. (Please see Django's ​guide to submitting contributions.)

[Mike Edmunds]

Terence thanks for the report and PR (and for tracking down the history of the implementation).

I'm tentatively accepting this, but there may be some additional discussion needed about whether it constitutes a breaking change (vs. a bug fix), whether the default max_similarity needs to be adjusted (was it tuned for quick_ratio()?), etc.

Checking the actual ratio() (not just quick_ratio()) seems correct to me. False positives in password validators decrease security by frustrating users attempting to set complex passwords. (That's not my opinion, it's ​NIST's position.)

[Terence Honles]

Thanks Mike, yeah I understand that this might be considered a breaking change.

I doubt this was tuned on any significant dataset, otherwise this likely would have been caught, but it's possible the default threshold should be adjusted regardless.

I haven't tested it, but my assumption is the default ratio was attempting to target 70% similarity. If that's the case I think leaving it is fine, but if someone is not using the default then they might have a different expectation and it may need to at least be called out in the release notes. If there's anything else that needs to be added I can adjust my changeset.

[Natalia Bidart]

Thank you for the report and the patch. I'm not sure this is something we want to change in Django core: anyone who needs accurate similarity checking can write a custom password validator and use it in AUTH_PASSWORD_VALIDATORS.

There are two concerns worth raising regardless. First, backward compatibility: this proposal changes which passwords are accepted, which is a behavior change even if it is in the permissive direction. Second, and more importantly, timing: the patch introduces a new code path where ratio() is called only when quick_ratio() passes the threshold. Without careful benchmarking we cannot rule out that this difference in execution time is detectable, which could leak information about how similar a candidate password is to a user attribute. This opens the door to a "classic" timing attack in a security-sensitive code path.

Given those concerns, I'm inclined to revert the acceptance of this ticket and set as wontfix. I'll be happy to reopen if there is proof that the different code paths (as proposed in the patch) are timing equivalent and therefore no timing attack is possible.

If it gets reopened, one thing that needs fixing for the current patch is the docs: it needs a release note, and in the UserAttributeSimilarityValidator section, it says max_similarity "is compared to the result of difflib.SequenceMatcher.quick_ratio()" therefore that sentence would need to be updated to say ratio().

[Terence Honles]

I respectfully disagree, and I also have an issue with how this is handled (maybe this is standard practice, but it seems quite hostile). Why is the ticket immediately closed and rejected as "wontfix" rather than discussing the patch before closing it? From my perspective this means "shut up, don't try", but I hope that's not the signal you're trying to send. If there's not a more appropriate status I think there should be one, as Django should be trying to encourage collaboration rather than shut it down.

Replying to Natalia Bidart:

Thank you for the report and the patch. I'm not sure this is something we want to change in Django core: anyone who needs accurate similarity checking can write a custom password validator and use it in AUTH_PASSWORD_VALIDATORS.

Really? If someone wants something that works they shouldn't use something core provides? Should the provided implementation be just deprecated and removed then? That seems more appropriate than rejecting a fix of broken behavior. In what world does someone actually want to reject anagrams? Especially when this is tested across tokenized user attributes which increases the likelihood of a validation error.

There are two concerns worth raising regardless. First, backward compatibility: this proposal changes which passwords are accepted, which is a behavior change even if it is in the permissive direction. Second, and more importantly, timing: the patch introduces a new code path where ratio() is called only when quick_ratio() passes the threshold. Without careful benchmarking we cannot rule out that this difference in execution time is detectable, which could leak information about how similar a candidate password is to a user attribute. This opens the door to a "classic" timing attack in a security-sensitive code path.

I initially considered just changing the patch to use ratio() instead of quick_ratio(), but I figured there would be an issue with the timing changes. I figured since this is happening when changing a password a slow method would be OK, but in the interest of avoiding complaints about the timing I went with the provided patch.

I understand you are thinking about security, but your timing attack is misguided. You are forgetting *which* password this is checking and where in the code path this lives. This validation (as intended) happens only after a user has received a password reset token and is submitting it back with a new password for a password change. The old password *is not checked* it's not stored in plain text so it can't be. This is not a timing attack. The *new password* you provided is what's being checked and the only information you can glean from a suggested timing attack (assuming you have stolen a reset token) is part of a user attribute. If the attacker has gotten this far then your out of luck.

Given those concerns, I'm inclined to revert the acceptance of this ticket and set as wontfix. I'll be happy to reopen if there is proof that the different code paths (as proposed in the patch) are timing equivalent and therefore no timing attack is possible.

As mentioned above I don't believe this should be considered a timing attack, but I can also change the implementation to always call only a single method and potentially allow a user to select which method to use (this can be something that is deprecated to allow a heads up about the change, something like use_ordered_comparision=False in the constructor).

If it gets reopened, one thing that needs fixing for the current patch is the docs: it needs a release note, and in the UserAttributeSimilarityValidator section, it says max_similarity "is compared to the result of difflib.SequenceMatcher.quick_ratio()" therefore that sentence would need to be updated to say ratio().

Sure, I can make this change. I wasn't going to invest a large amount of effort if this was just going to be rejected immediately (which more or less happened at a couple stages since PRs are auto-closed and now a knee-jerk reaction has closed this ticket).

I do hope that you can reconsider your decision, and I might suggest a nicer workflow might be status: needs info (maybe also with an auto timer) -> status: won't fix would allow a friendlier "we think this should be closed, but you can try to appeal this decision".

[Natalia Bidart]

Hello Terence, first of all, a note on the tone of your message, which is not in line with what we expect in the Django community. Disagreeing with a triage decision is completely valid, but describing this as "hostile", "shut up, don't try", or "a knee-jerk reaction" is not appropriate. Please review ​the CoC before posting again.

Regarding the approach to contributing: the PR was closed because the contributing guide was not followed, that is automatic and not a judgment on the work itself. Similarly, wontfix is a standard triage outcome and should not be taken personally. My invitation to provide benchmarks demonstrating timing equivalence and reopen the ticket still stands. For more context on these processes, the ​contributing guide and ​triage workflow docs are good starting points.

On "if someone wants something that works they shouldn't use Django core": Django is designed to solve the common case reliably and predictably, and that is a feature in itself that millions of projects depend on. The stability and backward compatibility of core components is not something we trade away lightly. For the specific case of password validation: the docs are explicit that writing a custom validator is the intended path when built-in ones don't meet specific needs. That is not a limitation, it is a deliberate design choice, and in cases like this, a custom validator is straightforward to write. See ​validator docs.

For the benchmarks request: framing this as "avoiding complaints" is a dismissal of a legitimate technical requirement. Benchmarks are a standard requirement for most changes, and particularly for security-sensitive components in Django core, and that applies here regardless of how the change is motivated or how small the impact might seem.

Lastly, the remarks about the reset-token and the timing attack are valid for that specific flow. However, validate_password(), which invokes all the validators, is ​explicitly documented for use in custom forms and API endpoints, and Django will not assume the narrowest possible context when evaluating changes to security components. Where there is ambiguity, Django prioritizes security. A change that introduces a variable-cost code path here requires proof that it does not open new attack surfaces in any of those contexts.

If you'd like to revisit this decision, bringing it to the Django Forum is the right next step. That is where the broader community engages and where consensus is built. Benchmarks demonstrating timing equivalence would need to be part of that conversation.