Fix token leakage

[akayvanfar]

The current implementation of the password reset page using a reset token leaves the reset token in the url's query parameters, which run the risk of exposing it via the Referer. This solution, grabs the reset token, stores it in the session, and redirects the user to the reset password page (/edit) without the reset token in the query parameters. We then delete the token from the session on a successful update of the password.

Tests updated and 2 new ones added.

[akayvanfar]

ping

[icaruswings]

The clearance gem had the exact same issue and this change is very similar to what ended up going into that recently. Theres a bit more of conversation here about the problem/fix that might be of interest to people here also - thoughtbot/clearance#707

[akayvanfar]

@ethirajsrinivasan can you review pls?

[akayvanfar]

@icaruswings Sorry for the missed reply. Is that PR part of devise now?

[akayvanfar]

ping @ethirajsrinivasan

[jezstephens]

@rafaelfranca @lucasmazza sorry to bug you, I'm not sure who else can help get this merged. We encountered the same problem and it would be great to get it fixed for everyone and not just in our app. Any thoughts?

[stewartyu]

ping This security bug got reported to us, so it's great to see there's a PR that addresses it! Is there anything I can do to help get this through?

[akayvanfar]

I am getting no response from the devise team. Can someone review this please?

[tegon]

Sorry about this, I've labeled this one and I'll definitely review it.
I just ask for a little of patience because we have about 20 pull requests to review and almost 50 issues to check.

Thanks.

[rafaelfranca]

I think I discussed this with José some time ago. If I remember correctly this is only a problem if you allow third-party JavaScript code to be executed in your reset password page. If you does allow that, the token leaking in the Referer reader is only one of your problems. Please let me know if that is not the only case were this is a problem.

[stewartyu]

@rafaelfranca This is a problem if there are any resources on the page (i.e. analytics tags, external JS, CSS, images, etc). In this screenshot, you can see in this request that the referrer contains the token. An attacker could then use the same URL to change the user's password

[lucasmazza]

Would the Referrer-Policy header help on this? I think it can be an easier way to deal with this without having to rewrite the feature into a possibly backwards compatible way (specially for those who customised their controllers)

[stewartyu]

@lucasmazza I had the same thought, but when I put <meta content='no-referrer' name='referrer'> in the page, it didn't resolve the problem - it seems that it's not fully implemented in all browsers. According to this article, that header can't be relied on for security concerns: https://robots.thoughtbot.com/is-your-site-leaking-password-reset-links

[akayvanfar]

@stewartyu I used that thoughtbot piece to help guide this PR. So I believe this solution follows those best practices.

[rafaelfranca]

Again, if you add external resources to your page and don't trust them you have a really big problem. This problem is similar to a MITM attack in your email account. If there is someone listening to your emails your reset password token will also be leaked. Of course, different from the MITM attack, it is the site owner that is installing the MITM against all the users accounts. In my opinion the proper fix for this issue is to only use trusted external resources in your reset password page.

If you don't trust your external resources, leaking the reset token is just the beginning. They could be mining bitcoins in your clients machines, stealing their credit card information, passwords, session cookie (like this reset token that the PR is adding to the session).

Of course we can narrow the attack surface with this change, but I'm not sure if it is worth to make this change given the real security issues is be still there.

[mnussbaumer]

I was just reading this and I thought that there's 1 way to solve this problem, but not really know if it's worth it (instead of customising the rendering of the reset controller/action to use a clean layout without possibly dangerous external content)

A solution would be to have the reset tied to the IP requesting the reset token. This involves two additional columns - reset_ip_request, visited (Default false) - and a special treatment of the request when not matching IPs

• Real User asks for reset token; a token is set and reset_ip_request is also set to the IP of the browser asking it, visited is set to false (by default);.
• Email is sent with a token;
• Real User clicks the link, IP matches, reset is allowed, visited is set to true
• Real User decides not to reset, token is leaked;
• Someone was able to read the token;
• Someone visits the reset token page, IP's won't match, password isn't changed, possibility to reset by the real user when visiting from the original IP remains open (because visited is true)

Second case:

• Fake User requests a password reset so they have their IP associated with the request;
• User gets email - probably they won't follow through (right?), but imagining they did,
• User visits token page - IP's don't match at all & visited is false (which it is by default) - since visited being false means this is the first visit to reset the token and since the IP's don't match, all the request is reset - it will require a new request for resetting the password if the user wants to go through with it.
• Fake User tries to reset with the token, there's nothing to reset.

On the first case the real user could still go again to reset the password because since the first visit with the token matched the IP's (assuming it's the user that has access to the email that executes always the first visit), visited would be set to true, where in the second case the IP's didn't match on the first visit, so it means that either the Real User changed device/connection/isp changed ip, and a new password reset would need to be made - since most requests are presumably a request followed by an almost immediate visit to reset the password there shouldn't be major problems and a few notices should be enough to guide.

This would mean that you would need to compromise the email box in order to be able to reset a password.

[rafaelfranca]

@mnussbaumer I like your idea. Although the workflow for the second case is worst, for the first case we don't have an extra redirect like this current implementation.

[mnussbaumer]

@rafaelfranca but there wouldn't be any redirect, and in the second case it would only happen if a) someone requested the reset for you, in which case the reset being "reseted" is not a problem b) you requested the reset from a device and then opened the reset link from another device. Both of these could have a message when they happened: "It seems you requested to reset this password from a different device than the one you're currently using. To be able to reset your password safely please make sure you do both steps from the same device. Click the following link to request a new reset link..." wtv?

Perhaps it could be opt-in, :paranoid_resets. If this was something that could get merged I might look into making a PR? (I haven't really looked at the source so, this is assuming it's something doable without extreme pain)

(edit: actually - if the problem is the token appearing on the referrer link, then it could be, instead of setting the ip on the request to reset, setting the ip on the first visit to the reset link - since that will most certainly be the real user, unless the email has been jacked, because the token won't be known until a) the user visits the link triggering the external assets load b) the app being jacked, c) the mailbox of the user being jacked)

[tegon]

@mnussbaumer Are you willing to work on this? Otherwise we need to open an issue for it.