Opened 2 years ago

Closed 2 years ago

Last modified 2 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

Description

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.

Risk

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.

Difficulty

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.

Solution

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 2 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 2 years ago by jacob

  • Severity changed from Release blocker to Normal

Not a release blocker.

comment:3 Changed 2 years ago by viciu

  • Owner changed from nobody to viciu
  • Status changed from new to assigned

comment:5 Changed 2 years ago by erikr

  • Cc eromijn@… added
  • Has patch set
  • Keywords dceu13 added
  • Patch needs improvement set
  • Version changed from 1.5 to master

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 2 years ago by erikr (previous) (diff)

comment:6 Changed 2 years ago by viciu

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

comment:7 Changed 2 years ago by erikr

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready 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 2 years ago by lukeplant

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 2 years ago by erikr

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.
PR: https://github.com/django/django/pull/1218

comment:10 Changed 2 years ago by erikr

  • Patch needs improvement unset

comment:11 Changed 2 years ago by gcc

  • Triage Stage changed from Accepted to Ready for checkin

Looks good to me.

comment:12 Changed 2 years ago by erikr

The fix for #20593 breaks PR 1218.

I have made a new PR in https://github.com/django/django/pull/1280, 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 2 years ago by erikr

  • Owner viciu deleted
  • Status changed from assigned to new

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

  • Owner set to Erik Romijn <erik@…>
  • Resolution set to fixed
  • Status changed from new to closed

In aeb1389442d0f9669edf6660b747fd10693b63a7:

Fixed #20079 -- Improve security of password reset tokens

comment:15 Changed 2 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 2 years ago by Aymeric Augustin <aymeric.augustin@…>

In c8756e17fbd5293ee1e0582201c41e2febc58ae1:

Removed obsolete comment. Refs #20079.

Thanks Gavin Wahl.

comment:17 Changed 2 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