[Django] #29289: Clarify comment regarding what data is hashed to generate PasswordResetTokenGenerator tokens
Django
PasswordResetTokenGenerator tokens
------------------------------------------------+------------------------
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Robert Picard reported this to the security mailing list:
The code for the password generator has
[https://github.com/django/django/blob/be6ca89396c031619947921c81b8795d816e3285/django/contrib/auth/tokens.py#L60
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.
[https://github.com/django/django/blob/be6ca89396c031619947921c81b8795d816e3285/django/contrib/auth/tokens.py#L76
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29289>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
PasswordResetTokenGenerator tokens
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/9856 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29289#comment:1>
Django
PasswordResetTokenGenerator tokens
Reporter: Tim Graham | Owner: nobody
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"85d853b2d3a77e58a5d28fcdd4594b2766e6c202" 85d853b]:
{{{
#!CommitTicketReference repository=""
revision="85d853b2d3a77e58a5d28fcdd4594b2766e6c202"
Fixed #29289 -- Clarified PasswordResetTokenGenerator comment regarding
the data hashed to generate tokens.
Thanks Luke Plant for the draft text.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29289#comment:2>