New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent password reset token leak via HTTP referer #707

Merged
merged 1 commit into from Sep 27, 2016

Conversation

Projects
None yet
7 participants
@derekprior
Copy link
Contributor

derekprior commented Sep 23, 2016

The password reset token is included in the URL emailed to users who
request a password reset. When the user clicks the link, they are
brought to the password reset form. If instead of completing the form
the user clicks a link to another HTTPS resource that may be in the site
navigation, then the password reset token will be leaked to that
external site via HTTP referer (sic).

Taking advantage of this would require an attack to:

  1. Control a site that is linked to on the password reset token
  2. Capture any HTTP referers that contain the token
  3. Use the referer URL to complete the password reset before the user
    does so themselves.

This is a rather involved exploit, but something we should close
nonetheless. We address it in this change by requiring that the token be
present in the session rather than in the URL before the page is
displayed to the user. If the user hits the edit action with a token in
the URL, the token will be stored in the session and the user will be
redirected to the same URL minus the token.

This is a bit more complicated the another change I considered where we
simply rotate the token before the form is displayed. While this change
is more involved and less localized, it has the advantage of maintaining
the side-effect-free get (kind of - we do mutate the session).

session[:password_reset_token] = params[:token]
redirect_to edit_user_password_url(@user)
else
render template: 'passwords/edit'

This comment has been minimized.

@houndci-bot

houndci-bot Sep 23, 2016

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Prevent password reset token leak via HTTP referer
The password reset token is included in the URL emailed to users who
request a password reset. When the user clicks the link, they are
brought to the password reset form. If instead of completing the form
the user clicks a link to another HTTPS resource that may be in the site
navigation, then the password reset token will be leaked to that
external site via HTTP referer (sic).

Taking advantage of this would require an attack to:

1. Control a site that is linked to on the password reset token
2. Capture any HTTP referers that contain the token
3. Use the referer URL to complete the password reset before the user
does so themselves.

This is a rather involved exploit, but something we should close
nonetheless. We address it in this change by requiring that the token be
present in the session rather than in the URL before the page is
displayed to the user. If the user hits the edit action with a token in
the URL, the token will be stored in the session and the user will be
redirected to the same URL minus the token.

This is a bit more complicated the another change I considered where we
simply rotate the token before the form is displayed. While this change
is more involved and less localized, it has the advantage of maintaining
the side-effect-free `get` (kind of - we do mutate the session).

@derekprior derekprior force-pushed the dp-pw-token-session branch from 6f43aae to ad294a9 Sep 23, 2016

@mike-burns

This comment has been minimized.

Copy link
Member

mike-burns commented Sep 23, 2016

I like this for the GET safety -- some browsers are eager to optimize GET requests, and mail clients certainly do whatever they want. But I'm not sure if the GET safety is relevant in practice.

@derekprior

This comment has been minimized.

Copy link
Contributor Author

derekprior commented Sep 23, 2016

Interesting note: this approach (vs #706) will continue to be valid in face of #682 which will be in Clearance 2.0. The change in #706 will no longer work with #682.

@georgebrock

This comment has been minimized.

Copy link
Member

georgebrock commented Sep 23, 2016

I like this for the GET safety -- some browsers are eager to optimize GET requests, and mail clients certainly do whatever they want. But I'm not sure if the GET safety is relevant in practice.

I think it is relevant in practice:

  1. There are some reports of links only shared via email being visited in some situations, e.g. https://www.stonetemple.com/does-google-sniff-gmail-for-urls/ indicates some of the links they created for testing were visited by (non-Google) bots after they were shared by email.
  2. According to the HTTP spec, there's no reason for a mail client not to start visiting links in emails any time it wanted to.
@derekprior

This comment has been minimized.

Copy link
Contributor Author

derekprior commented Sep 23, 2016

Ok, I'm convinced this is the best approach. Any comments on the code itself?

@georgebrock

This comment has been minimized.

Copy link
Member

georgebrock commented Sep 26, 2016

The code looks good to me.

@derekprior derekprior merged commit ad294a9 into master Sep 27, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound 1 violation found.
@jeroenvisser101

This comment has been minimized.

Copy link
Contributor

jeroenvisser101 commented Oct 3, 2016

Just wanted to note that this is not just for links, but also for externally hosted images and CSS/JS. That means that an image host, CDN or advertisement could possibly hijack this as well. I feel this might increase the attack vector a bit. 👍 for fixing!

@derekprior

This comment has been minimized.

Copy link
Contributor Author

derekprior commented Oct 3, 2016

Oh of course. I hadn't considered that. Thanks for pointing it out, @jeroenvisser101.

@derekprior derekprior deleted the dp-pw-token-session branch Oct 6, 2016

@o-hudson

This comment has been minimized.

Copy link

o-hudson commented Oct 24, 2016

A downside of this approach is that the token persists in the session if the user doesn't complete the form and navigates to other pages on the site. If the session is then stolen (perhaps through cross-site scripting) the password can be reset by a malicious user.

@jeroenvisser101

This comment has been minimized.

Copy link
Contributor

jeroenvisser101 commented Oct 24, 2016

@o-hudson that's correct. We might want to add expiration timestamp on the session (to decrease the chance of it being usable when stolen).

@derekprior

This comment has been minimized.

Copy link
Contributor Author

derekprior commented Oct 24, 2016

I had not thought of that. Theft via XSS vulnerability would indeed be a problem, but it's just one of many different problems you'd have as a result of XSS.

On thing we could do is add a before_action that clears this session key on all actions other than PasswordsController#edit and PasswordsController#update, thus making the session value very short-lived.

Controlling the expiration of the session also decreases the window, but as a library I don't want to enforce the expiration on all uses of session.

@jeroenvisser101

This comment has been minimized.

Copy link
Contributor

jeroenvisser101 commented Oct 24, 2016

Controller the expiration of the session also decreases the window, but as a library I don't want to enforce the expiration on all uses of session.

@derekprior For expiration, I'd say we could do this without expiring the session itself (say we'd add session[:password_reset_token_expires_at] or something)

but in that regard it's just one of many different problems you'd have as a result of XSS.

I agree, XSS should be avoided to begin with. (or any way to steal a session)

@sairam

This comment has been minimized.

Copy link

sairam commented Jun 1, 2017

I was listening to the older podcast and thought of a different solution for the same problem.

This approach may solve it without usage of session or data store.

key = hash(global_secret, password_token)
http://example.com/reset?token=#{password-reset-token}&key=#{key} # url to send
  1. user who clicks the link will come to the website.
  2. the key is validated against the token
  3. if valid, user is redirected to url without key param
  4. if key invalid or not present, remove the token from the url.

this way usage of session can be avoided, but requires a global_secret to be part of the configuration/environment variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment