Improve security of password reset tokens

[Jacob]

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).

[Jacob]

Not a release blocker.

[Wiktor]

Pull request here: ​https://github.com/django/django/pull/1170

[Sasha Romijn]

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.

[Wiktor]

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

[Sasha Romijn]

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.

[Luke Plant]

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.

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

[Chris Wilson]

Looks good to me.

[Sasha Romijn]

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.

[Erik Romijn <erik@…>]

In aeb1389442d0f9669edf6660b747fd10693b63a7:

Fixed #20079 -- Improve security of password reset tokens

[Aymeric Augustin <aymeric.augustin@…>]

In a01b1ee6881cc4ce6c8bee771bb5b463bc16dd77:

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

Fixed #20079 -- Improved security of password reset tokens

[Aymeric Augustin <aymeric.augustin@…>]

In c8756e17fbd5293ee1e0582201c41e2febc58ae1:

Removed obsolete comment. Refs #20079.

Thanks Gavin Wahl.

[Aymeric Augustin <aymeric.augustin@…>]

In 6908b6593949a205d05da342060a9d952bd7b77c:

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

Thanks Gavin Wahl.