Opened 6 years ago
Closed 6 years ago
#29289 closed Cleanup/optimization (fixed)
Clarify comment regarding what data is hashed to generate PasswordResetTokenGenerator tokens
| Reported by: | Tim Graham | Owned by: | nobody |
|---|---|---|---|
| Component: | contrib.auth | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
Robert Picard reported this to the security mailing list:
The code for the password generator has a comment (there since it was written 10 years ago in fcd837cd0f9b2c706bc49af509628778d442bb3f) that mentions that the salt is included in the hashed values so that the token can only be used once. The code actually uses the password hash (
user.password) instead of the salt.
While this is a hash, it seems like making a value from the password hash was not the original intention and is an unnecessary security risk (though it may be a small risk).
The security team couldn't identify a problem with the implementation. The suggestion to change the implementation to use only the salt was raised, but Luke Plant said:
Using just the password salt for the reset token hash, rather than the whole field, could reduce the entropy available to the reset token. possibly quite significantly if the salt is short (some old/custom password hash algorithms?), so I think that would be a bad idea.
From what I'm aware, there is no known danger from using the full password field (i.e. including hash) to create the token, providing that we are hashing it (and in our case we are adding various other bits that the attacker does not know to it as well). Of course, *theoretically* there is a danger - we are inevitably leaking *some* information about the password when we do this, in the information theoretic sense - if you have a hash of a password, you have more information about the password than if you had nothing, and similarly for a hash of a hash of a password. The issue for an attack is turning this theory into practice, which is very hard, which is why we use password hashing etc.. And in this case, we are sending that 'leaked' information only to the user themselves.
AFAICS, the hash function would have to be *very* broken for this to be a problem in practice. The benefit of using it (the extra entropy it provides to the reset token) seems to far outweigh any danger, IMO.
PR