HTTP Referer leaks password reset link

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

[Romain Garrigues]

I gave a try to the approach described in the article as solution 2 in "Plugging The Leak" section.
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...

[Romain Garrigues]

Solution 1 - redirect without token in the url

[Romain Garrigues]

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.

[Romain Garrigues]

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

[Romain Garrigues]

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?

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

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

[Tim Graham]

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

[Romain Garrigues]

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

[Tim Graham <timograham@…>]

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.