[Django] #20079: Improve security of password reset tokens
Django
-------------------------------------------+------------------------
Reporter: jacob | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/20079>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jacob | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jacob):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:1>
Django
Reporter: jacob | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jacob):
* severity: Release blocker => Normal
Comment:
Not a release blocker.
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:2>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by viciu):
* owner: nobody => viciu
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:3>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by viciu):
Pull request here: https://github.com/django/django/pull/1170
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:4>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* keywords: => dceu13
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* version: 1.5 => master
* cc: eromijn@… (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:5>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by viciu):
Thanks for pointing this out. I've updated the pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:6>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by erikr):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:7>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by lukeplant):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:8>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:9>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:10>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
Looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:11>
Django
Reporter: jacob | Owner: viciu
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:12>
Django
Reporter: jacob | Owner:
Type: Bug | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* owner: viciu =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:13>
Django
Reporter: jacob | Owner: Erik
Type: Bug | Romijn <erik@…>
Component: contrib.auth | Status: closed
Severity: Normal | Version: master
Keywords: dceu13 | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Erik Romijn <erik@…>):
* status: new => closed
* owner: => Erik Romijn <erik@…>
* resolution: => fixed
Comment:
In [changeset:"aeb1389442d0f9669edf6660b747fd10693b63a7"]:
{{{
#!CommitTicketReference repository=""
revision="aeb1389442d0f9669edf6660b747fd10693b63a7"
Fixed #20079 -- Improve security of password reset tokens
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:14>
Django
Reporter: jacob | Owner: Erik
Type: Bug | Romijn <erik@…>
Component: contrib.auth | Status: closed
Severity: Normal | Version: master
Keywords: dceu13 | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"a01b1ee6881cc4ce6c8bee771bb5b463bc16dd77"]:
{{{
#!CommitTicketReference repository=""
revision="a01b1ee6881cc4ce6c8bee771bb5b463bc16dd77"
Merge pull request #1280 from erikr/improve-password-reset2
Fixed #20079 -- Improved security of password reset tokens
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:15>
Django
Reporter: jacob | Owner: Erik
Type: Bug | Romijn <erik@…>
Component: contrib.auth | Status: closed
Severity: Normal | Version: master
Keywords: dceu13 | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"c8756e17fbd5293ee1e0582201c41e2febc58ae1"]:
{{{
#!CommitTicketReference repository=""
revision="c8756e17fbd5293ee1e0582201c41e2febc58ae1"
Removed obsolete comment. Refs #20079.
Thanks Gavin Wahl.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:16>
Django
Reporter: jacob | Owner: Erik
Type: Bug | Romijn <erik@…>
Component: contrib.auth | Status: closed
Severity: Normal | Version: master
Keywords: dceu13 | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"6908b6593949a205d05da342060a9d952bd7b77c"]:
{{{
#!CommitTicketReference repository=""
revision="6908b6593949a205d05da342060a9d952bd7b77c"
[1.6.x] Removed obsolete comment. Refs #20079.
Thanks Gavin Wahl.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20079#comment:17>