Code

Opened 16 months ago

Closed 13 months ago

Last modified 13 months 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).

Attachments (0)

Change History (17)

comment:1 Changed 16 months ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 16 months ago by jacob

  • Severity changed from Release blocker to Normal

Not a release blocker.

comment:3 Changed 14 months ago by viciu

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

comment:5 Changed 14 months 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 14 months ago by erikr (previous) (diff)

comment:6 Changed 14 months ago by viciu

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

comment:7 Changed 14 months 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 14 months 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 14 months 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 14 months ago by erikr

  • Patch needs improvement unset

comment:11 Changed 14 months ago by gcc

  • Triage Stage changed from Accepted to Ready for checkin

Looks good to me.

comment:12 Changed 13 months 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 13 months ago by erikr

  • Owner viciu deleted
  • Status changed from assigned to new

comment:14 Changed 13 months 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 13 months 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 13 months ago by Aymeric Augustin <aymeric.augustin@…>

In c8756e17fbd5293ee1e0582201c41e2febc58ae1:

Removed obsolete comment. Refs #20079.

Thanks Gavin Wahl.

comment:17 Changed 13 months ago by Aymeric Augustin <aymeric.augustin@…>

In 6908b6593949a205d05da342060a9d952bd7b77c:

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

Thanks Gavin Wahl.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.