Opened 8 years ago

Closed 8 years ago

#27518 closed Cleanup/optimization (fixed)

HTTP Referer leaks password reset link

Reported by: Romain Garrigues Owned by: Romain Garrigues
Component: contrib.auth Version: 1.10
Severity: Normal Keywords: password reset
Cc: 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 (last modified by Romain Garrigues)

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.

Attachments (2)

password_reset_security_issue_2.txt (4.1 KB ) - added by Romain Garrigues 8 years ago.
Solution 1 - redirect without token in the url
password_reset_security_issue.txt (1.7 KB ) - added by Romain Garrigues 8 years ago.
Solution 2 - Invalidate the token in the url and generate a new one in the session.

Download all attachments as: .zip

Change History (18)

comment:1 by Romain Garrigues, 8 years ago

Description: modified (diff)

comment:2 by Romain Garrigues, 8 years ago

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...
Version 0, edited 8 years ago by Romain Garrigues (next)

by Romain Garrigues, 8 years ago

Solution 1 - redirect without token in the url

comment:3 by Romain Garrigues, 8 years ago

I also tried the approach proposed in the article as solution 1 in "Plugging The Leak" section:
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.

Last edited 8 years ago by Romain Garrigues (previous) (diff)

by Romain Garrigues, 8 years ago

Solution 2 - Invalidate the token in the url and generate a new one in the session.

comment:4 by Romain Garrigues, 8 years ago

Owner: changed from nobody to Romain Garrigues
Status: newassigned

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?

comment:5 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted
Type: New featureCleanup/optimization

comment:6 by Florian Apolloner, 8 years ago

@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?

comment:7 by Romain Garrigues, 8 years ago

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?

comment:8 by Tim Graham, 8 years ago

Typically we don't add or change behavior of deprecated things.

comment:9 by Romain Garrigues, 8 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

comment:10 by Romain Garrigues, 8 years ago

I understand @Tim, but isn't this a special situation, as we are fixing a kind-of-security issue?

comment:11 by Romain Garrigues, 8 years ago

Has patch: unset
Needs tests: unset
Patch needs improvement: unset

comment:12 by Romain Garrigues, 8 years ago

Has patch: set

comment:13 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:14 by Romain Garrigues, 8 years ago

Patch needs improvement: unset

comment:15 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:16 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In ede59ef6:

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.

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