Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#20079 closed Bug (fixed)

Improve security of password reset tokens

Reported by: Jacob Owned by: Erik Romijn <erik@…>
Component: contrib.auth Version: master
Severity: Normal Keywords: dceu13
Cc: eromijn@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


If SECRET_KEY remains secret, the admin/auth password reset functionality should be very secure. However, it is less secure if the SECRET_KEY is exposed, but could be improved.


Attacker could gain access to a staff or superuser account, which often gets you a very high level of access to information and ability to change/delete information.


Using the default reset token generator, the attacker would need to know:

pk of admin - this is very easy to guess, since a superuser will often have pk=1, and other staff users have increasing IDs
hashed password of user

if this is set to "!", an unusable password, this is easy to guess
otherwise almost impossible

last login timestamp, truncated to second precision.

An attacker who knows SECRET_KEY has a practical chance of success if there are admin users with no password set. This can happen if the 'createsuperuser' command is used in a script, or other situations. For such users, the last login timestamp is never updated, and will be the time the user was created on the system, and it's possible an attacker could have a good idea of this. If they know it to within 2 weeks, that's 1.2 million values to try, which is feasible over a network if they don't mind waiting.


The probability of attack here is pretty low, and requires knowledge of SECRET_KEY in the first place, but there is an easy way to improve it: add a load of alphanumeric entropy to the 'unusable password', so it is different in every case. An unusable password simply needs to start with "!", which makes it an impossible value for any of the hashers (old MD5 only has alphanumeric chars and not !, and new hashers all have $ in the value).

Change History (17)

comment:1 Changed 4 years ago by Jacob

Triage Stage: UnreviewedAccepted

comment:2 Changed 4 years ago by Jacob

Severity: Release blockerNormal

Not a release blocker.

comment:3 Changed 3 years ago by Wiktor

Owner: changed from nobody to Wiktor
Status: newassigned

comment:5 Changed 3 years ago by Erik Romijn

Cc: eromijn@… added
Has patch: set
Keywords: dceu13 added
Patch needs improvement: set
Version: 1.5master

Two comments:

  • The test does not check the new feature ("passwords always include entropy") - if I apply your test, but not the implementation, the test succeeds.
  • The admin_views tests contain several checks that a set password is not equal to UNUSABLE_PASSWORD; but with this patch, those tests would succeed even if the user's password had become set to an unusable one. In other words, those test will also need to be updated to use startswith.
Last edited 3 years ago by Erik Romijn (previous) (diff)

comment:6 Changed 3 years ago by Wiktor

Thanks for pointing this out. I've updated the pull request.

comment:7 Changed 3 years ago by Erik Romijn

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

The patch looks good to me. The test has a minor chance of a hash collision one day, but as the two random strings are 40 characters, this will probably never happen in our lifetime.

comment:8 Changed 3 years ago by Luke Plant

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

It's important to change the name of the constant UNUSABLE_PASSWORD to something else (UNUSABLE_PASSWORD_PREFIX, perhaps), because:

  1. UNUSABLE_PASSWORD is now a misleading name
  2. As far as possible, code should be left as if it had been written that way from the beginning.
  3. If any code is depending on the original meaning of UNUSABLE_PASSWORD, it will be broken. This could include 3rd party code. It is relying on an undocumented internal, so it is OK to break their code, but by changing the name of the constant, their code will break loudly and obviously, rather than in difficult to spot ways. The same thing applies to other instances of UNUSABLE_PASSWORD in Django's own code base - if the patch had been written this way from the start, it would have been impossible to miss those other instances that erikr pointed out.

In fact, in most cases that UNUSABLE_PASSWORD is used in the tests, it should actually be removed and replaced with user.has_usable_password() (except if has_usable_password is itself being tested), because that is the whole point of User.has_usable_password() - to hide the implementation detail of UNUSABLE_PASSWORD. That isn't the fault of the current patch, but it is a good opportunity to clean it up.

comment:9 Changed 3 years ago by Erik Romijn

Thanks for your input, Luke.

I have made an improved version of viciu's patch, taking all Luke's comments into account. UNUSABLE_PASSWORD_PREFIX sounds like a good name to me.

comment:10 Changed 3 years ago by Erik Romijn

Patch needs improvement: unset

comment:11 Changed 3 years ago by Chris Wilson

Triage Stage: AcceptedReady for checkin

Looks good to me.

comment:12 Changed 3 years ago by Erik Romijn

The fix for #20593 breaks PR 1218.

I have made a new PR in, which cleanly applies. The only other change I made was replace the magic number for the number of random characters to add, with a defined UNUSABLE_PASSWORD_SUFFIX_LENGTH.

comment:13 Changed 3 years ago by Erik Romijn

Owner: Wiktor deleted
Status: assignednew

comment:14 Changed 3 years ago by Erik Romijn <erik@…>

Owner: set to Erik Romijn <erik@…>
Resolution: fixed
Status: newclosed

In aeb1389442d0f9669edf6660b747fd10693b63a7:

Fixed #20079 -- Improve security of password reset tokens

comment:15 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

In a01b1ee6881cc4ce6c8bee771bb5b463bc16dd77:

Merge pull request #1280 from erikr/improve-password-reset2

Fixed #20079 -- Improved security of password reset tokens

comment:16 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

In c8756e17fbd5293ee1e0582201c41e2febc58ae1:

Removed obsolete comment. Refs #20079.

Thanks Gavin Wahl.

comment:17 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

In 6908b6593949a205d05da342060a9d952bd7b77c:

[1.6.x] Removed obsolete comment. Refs #20079.

Thanks Gavin Wahl.

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