[Django] #27518: HTTP Referer leaks password reset link
Django
-------------------------------------+-------------------------------------
Reporter: Romain | Owner: nobody
Garrigues |
Type: New | Status: new
feature |
Component: | Version: 1.10
contrib.auth |
Severity: Normal | Keywords: password reset
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Hi security team!
I read an article titled "Is Your Site Leaking Password Reset Links?"
(https://robots.thoughtbot.com/is-your-site-leaking-password-reset-links)
and I just realised that by using classic Django password_reset_confirm
view, my reset password link was effectively sent to other websites in the
HTTP Referer header.
The use case is this one:
- A customer receives a link to be able to reset his password on a Django
powered website,
- He clicks on this link, arrives on a page with the password change form,
and if on that page, there are calls to external resources, like cdn, the
whole url will be sent in the HTTP header of the request,
- If he directly resets this password, no issue, the token is no more
valid,
- If for any reason he doesn't reset his password straight away, some
external website could get this url and change the password in behalf of
the user.
Removing the HTTP Referer header
(http://stackoverflow.com/questions/6817595/remove-http-referer) can be a
solution, but wouldn't it interesting to implement some checks in Django
password_reset_confirm view?
After some discussions with the security team, it has been classified as
not really serious and could be discussed in public.
I will propose 2 approaches to solve it, with their respective issues.
--
Ticket URL: <https://code.djangoproject.com/ticket/27518>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Romain Garrigues | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: password reset | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Romain Garrigues:
Old description:
> Hi security team!
>
> I read an article titled "Is Your Site Leaking Password Reset Links?"
> (https://robots.thoughtbot.com/is-your-site-leaking-password-reset-links)
> and I just realised that by using classic Django password_reset_confirm
> view, my reset password link was effectively sent to other websites in
> the HTTP Referer header.
>
> The use case is this one:
> - A customer receives a link to be able to reset his password on a Django
> powered website,
> - He clicks on this link, arrives on a page with the password change
> form, and if on that page, there are calls to external resources, like
> cdn, the whole url will be sent in the HTTP header of the request,
> - If he directly resets this password, no issue, the token is no more
> valid,
> - If for any reason he doesn't reset his password straight away, some
> external website could get this url and change the password in behalf of
> the user.
>
> Removing the HTTP Referer header
> (http://stackoverflow.com/questions/6817595/remove-http-referer) can be a
> solution, but wouldn't it interesting to implement some checks in Django
> password_reset_confirm view?
>
> After some discussions with the security team, it has been classified as
> not really serious and could be discussed in public.
> I will propose 2 approaches to solve it, with their respective issues.
New description:
Hi!
I read an article titled "Is Your Site Leaking Password Reset Links?"
(https://robots.thoughtbot.com/is-your-site-leaking-password-reset-links)
and I just realised that by using classic Django password_reset_confirm
view, my reset password link was effectively sent to other websites in the
HTTP Referer header.
The use case is this one:
- A customer receives a link to be able to reset his password on a Django
powered website,
- He clicks on this link, arrives on a page with the password change form,
and if on that page, there are calls to external resources, like cdn, the
whole url will be sent in the HTTP header of the request,
- If he directly resets this password, no issue, the token is no more
valid,
- If for any reason he doesn't reset his password straight away, some
external website could get this url and change the password in behalf of
the user.
Removing the HTTP Referer header
(http://stackoverflow.com/questions/6817595/remove-http-referer) can be a
solution, but wouldn't it interesting to implement some checks in Django
password_reset_confirm view?
After some discussions with the security team, it has been classified as
not really serious and could be discussed in public.
I will propose 2 approaches to solve it, with their respective issues.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:1>
Django
Reporter: Romain Garrigues | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Romain Garrigues):
I gave a first try to the approach described in the article.
I kept the password_reset_confirm interface, and redirects to a
password_reset_confirm_secure view.
I didn't know how to keep the original params from the view other than
saving them in the session (redirect can't receive them in the keyword
args, as they are used in a reverse url call).
This solution has 2 main issues:
- Saving params in the session seems a bit hacky, and not everything is
serialisable,
- While it keeps backward-compatibility if people are using
django.contrib.auth.urls, it won't work if you don't include these urls
but have custom ones. It them means that you will have to define a new url
for django_reset_confirm_secure view...
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:2>
Django
Reporter: Romain Garrigues | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Romain Garrigues):
* Attachment "password_reset_security_issue_2.txt" added.
Solution 1 - redirect without token in the url
Django
Reporter: Romain Garrigues | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Romain Garrigues):
I also tried the second approach proposed in the article:
1/ check the token,
2/ generate a new one by changing the user password, making the old one
invalid (as the token is based mainly on user password and last_joined
fields),
3/ store it in the session,
4/ use this newly generated one for password reset confirmation.
It has the benefit of being almost backward compatible (I think), except
that, as we change the user password, if the user remember the password
when he access the form, quit the page and wants to login, he won't be
able to do it anymore.
On the other hand, he was in the process of resetting it, so it doesn't
seem critical to generate a random password at the meantime, and he can
still reset it anyway.
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:3>
Django
Reporter: Romain Garrigues | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Romain Garrigues):
* Attachment "password_reset_security_issue.txt" added.
Solution 2 - Invalidate the token in the url and generate a new one in the
session.
Django
-------------------------------------+-------------------------------------
| Garrigues
Type: New feature | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Romain Garrigues
Comment:
I definitely prefer the last proposed solution, even if not perfect...
Before going further to manage all situations and add tests on it, do you
think it makes sense to go on that way?
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: New feature => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
@Romain Garrigues: So what I imagine is the following:
* The user gets the link: ''/reset/Mq/asdga-yxflkjxc78121/''
* You store ''asdga-yxflkjxc78121'' in the session and redirect to
''/reset/Mq/set-password/'' (luckily our regex allows for this)
* if the token is ''set-password'' and the session has the proper value
we can reset the password
This allows us to:
* Do not alter the password for the user twice
* Keep the most compatibility with the existing system (same URL etc, no
changes needed unless you manually changed the regex)
* Not leak any information (only Mq which is the user id, which is
guessable anyways)
* We do not have to store any extra information in the session
Is that clear enough? If yes, do you want to try a patch against the class
based views in master?
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Romain Garrigues):
Hi @Florian!
I get the idea and definitely motivated to try!!
I guess we should update the CBV **and** the FBV version (even if
deprecated), and add tests for both?
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Typically we don't add or change behavior of deprecated things.
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: password reset | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by Romain Garrigues):
I understand @Tim, but isn't this a special situation, as we are fixing a
kind-of-security issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:13>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:14>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:15>
Django
-------------------------------------+-------------------------------------
Type: | Garrigues
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: password reset | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ede59ef6f39ff8a6443c2b24df0208ef6ec41ee0" ede59ef6]:
{{{
#!CommitTicketReference repository=""
revision="ede59ef6f39ff8a6443c2b24df0208ef6ec41ee0"
Fixed #27518 -- Prevented possibie password reset token leak via HTTP
Referer header.
Thanks Florian Apolloner for contributing to this patch and
Collin Anderson, Markus Holtermann, and Tim Graham for review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27518#comment:16>