Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#20079 closed Bug (fixed)

Improve security of password reset tokens

Reported by: Jacob Owned by: Erik Romijn <erik@…>
Component: contrib.auth Version: dev
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 by Jacob, 11 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Jacob, 11 years ago

Severity: Release blockerNormal

Not a release blocker.

comment:3 by Wiktor, 11 years ago

Owner: changed from nobody to Wiktor
Status: newassigned

comment:5 by Sasha Romijn, 11 years ago

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 11 years ago by Sasha Romijn (previous) (diff)

comment:6 by Wiktor, 11 years ago

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

comment:7 by Sasha Romijn, 11 years ago

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 by Luke Plant, 11 years ago

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 by Sasha Romijn, 11 years ago

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 by Sasha Romijn, 11 years ago

Patch needs improvement: unset

comment:11 by Chris Wilson, 11 years ago

Triage Stage: AcceptedReady for checkin

Looks good to me.

comment:12 by Sasha Romijn, 11 years ago

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 by Sasha Romijn, 11 years ago

Owner: Wiktor removed
Status: assignednew

comment:14 by Erik Romijn <erik@…>, 11 years ago

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

In aeb1389442d0f9669edf6660b747fd10693b63a7:

Fixed #20079 -- Improve security of password reset tokens

comment:15 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In a01b1ee6881cc4ce6c8bee771bb5b463bc16dd77:

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

Fixed #20079 -- Improved security of password reset tokens

comment:16 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In c8756e17fbd5293ee1e0582201c41e2febc58ae1:

Removed obsolete comment. Refs #20079.

Thanks Gavin Wahl.

comment:17 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 6908b6593949a205d05da342060a9d952bd7b77c:

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

Thanks Gavin Wahl.

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