1456652 - (CVE-2018-12370) SameSite cookie Reader view patch bypass

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.117 Safari/537.36

Steps to reproduce:

1. Go to https://test.shhnjk.com/read_bypass.html
2. Click reader view icon
3. Click on "Click ME" link
4. Exit Reader view in new tab


Actual results:

SameSite cookie not sent.


Expected results:

SameSite cookie is sent when exiting Reader view at the end.

Firefox doesn't fully track "Site for cookie" when exiting Reader view. Using various tricks in Reader view (target blank with # and meta refresh), attacker can start Reader view from attacker site and navigate to victim site without exiting Reader view. Which makes it possible to end in a state where Reader view holds CSRF payload within Reader view, and it is triggered when exiting Reader view.

Comment 1

[Jun (Request needinfo for comment, I ignore other updates)]

Sorry, Actual results and expected results are opposite. I'm struggled because Monorail and Bugzilla has opposite questions :(

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

302 to mgoodwin.

Comment 3

[Jun (Request needinfo for comment, I ignore other updates)]

Sorry for the spam, but you need to visit https://shhnjk.azurewebsites.net/SameSite.php to set SameSite cookie before trying repro steps.

Comment 4

[:Gijs (he/him)]

This is because reader mode manually handles meta redirects by determining the URL and then navigating to it ( . There's no reasonable way to pass a triggering principal when doing that. ( https://dxr.mozilla.org/mozilla-central/rev/26e53729a10976f52e75efa44e17b5e054969fec/toolkit/components/reader/AboutReader.jsm#659 )

Maybe we can switch the load to use the docshell directly, and then pass the correct information.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #4)
> Maybe we can switch the load to use the docshell directly, and then pass the
> correct information.

It turns out this is tricky because it essentially requires passing the information from the first reader mode page load to the next. I thought we could just set a triggering principal on the reader mode load, but that doesn't work because `http://foo.com/` isn't allowed to load about:reader?... (of course!).

So we need the triggering principal / URL in the next about:reader page that loads... We'd have to add another url parameter or something. 

Alternatively, we could look at doing the fetch in the same page, and use history.replaceState() to update the URL bar and history entry (to the target of the redirect). However, I don't see a way to specify an explicit triggering principal for an XHR or fetch request initiated from JS. Am I just missing how to do that?

Comment 6

[Jun (Request needinfo for comment, I ignore other updates)]

I might be wrong but I think what you need to figure out is that how not to forget "site for cookies" when exiting Reader view. If you test my PoC, SameSite cookie isn't sent when https://shhnjk.azurewebsites.net/SameSite.php is rendered in Reader view. SameSite cookie is only sent when user exit from Reader view.

So I think SameSite cookie is handled correctly inside Reader view, but not when exiting from Reader view.

Comment 7

[:Gijs (he/him)]

(In reply to Jun from comment #6)
> I might be wrong but I think what you need to figure out is that how not to
> forget "site for cookies" when exiting Reader view. If you test my PoC,
> SameSite cookie isn't sent when
> https://shhnjk.azurewebsites.net/SameSite.php is rendered in Reader view.
> SameSite cookie is only sent when user exit from Reader view.

Ah, I did some of my testing in an earlier version of 60 beta, which presumably didn't have some of the other fixes yet... I can reproduce what you're seeing now.

> So I think SameSite cookie is handled correctly inside Reader view, but not
> when exiting from Reader view.

Well... this is an interesting question. It amounts to whether the navigation from read:foo.com/site to foo.com/site is same-site or not. I think you could reasonably argue that it's same-site, but even if you don't think so, that this isn't a security issue as written.

My reasoning for the latter is as follows. Suppose you had a site (V) using same-site: strict for cookies, and site (A) tried to exploit a CSRF issue in (V) by sending people to a deep-link in (V). same-site cookies would stop this, because the deep-linked website wouldn't have cookies.

If (V) doesn't navigate when accessed without cookies (e.g. to an "authentication denied" or "something went wrong" style page with a different URL), then the URL in the URL bar would be the one (A) wanted it to be, right? But then, if the user went to the URL bar and hit enter to try to access the page again (because it "didn't work", whatever (A) promised would happen on (V) if they were trying to social engineer like you are here) then (V) *would* be exploited, because at this point it's just a user-requested access of the victim page, and same-site cookies won't help anymore (I'm assuming everyone agrees this is effectively by design, even if it may be considered unfortunate).

In the same vein, why is exiting reader mode here different from the user requesting the URL directly? And in particular, given that (A) has no control over the content of (V), how would you encourage the user to either hit 'enter' in the URL bar, or click "exit reader mode", rather than closing the tab (which after all is newly opened in this exploit...)?



To defend against any of this, I believe it ought to be considered good practice to have any access to a deep-link in same-site-cookie-protected (V) redirect to a "safe" page that tries to explain what happened to the user. I think that is what happens by design in lots of sites (e.g. bank sites or email accounts where the (hopefully same-site) cookies contain session information will redirect from deep-link pages to the sign-in page if accessed without cookies). Would you agree with that? If so, I believe the initial reader mode view here won't have cookies and would thus be redirected to the "safe" page, which would also avoid what happens in the steps from comment #0 when exiting reader mode.



Anyway, it's possible to pass a triggering principal when exiting reader mode, at least, so that might help for the issue as written and be strictly more correct than what we do now, so I will try to write a patch for it. But I'm not convinced it's a security / same-site cookie issue in practice, per the above.

Comment 8

[Jun (Request needinfo for comment, I ignore other updates)]

(In reply to :Gijs (he/him) from comment #7)
> In the same vein, why is exiting reader mode here different from the user
> requesting the URL directly? And in particular, given that (A) has no
> control over the content of (V), how would you encourage the user to either
> hit 'enter' in the URL bar, or click "exit reader mode", rather than closing
> the tab (which after all is newly opened in this exploit...)?

I agree with this point that there's not much difference with social engineering victim to hit enter again... 

 
> To defend against any of this, I believe it ought to be considered good
> practice to have any access to a deep-link in same-site-cookie-protected (V)
> redirect to a "safe" page that tries to explain what happened to the user. I
> think that is what happens by design in lots of sites (e.g. bank sites or
> email accounts where the (hopefully same-site) cookies contain session
> information will redirect from deep-link pages to the sign-in page if
> accessed without cookies). Would you agree with that? If so, I believe the
> initial reader mode view here won't have cookies and would thus be
> redirected to the "safe" page, which would also avoid what happens in the
> steps from comment #0 when exiting reader mode.

Let's say attacker's payload is "https://twitter.com/settings/account?payload". Assuming that Twitter has SameSite Strict cookie in their session cookie, they will redirect to following URL when session is not present.

https://twitter.com/login?redirect_after_login=%2Fsettings%2Faccount%3Fpayload

When victim access above URL with session again (either by hit enter on address bar or exiting from Reader view), payload triggers anyway. Of course, this is just a case for Twitter and it depends on website.

Comment 9

[:Gijs (he/him)]

(In reply to Jun from comment #8)
> Let's say attacker's payload is
> "https://twitter.com/settings/account?payload". Assuming that Twitter has
> SameSite Strict cookie in their session cookie, they will redirect to
> following URL when session is not present.
> 
> https://twitter.com/
> login?redirect_after_login=%2Fsettings%2Faccount%3Fpayload
> 
> When victim access above URL with session again (either by hit enter on
> address bar or exiting from Reader view), payload triggers anyway. Of
> course, this is just a case for Twitter and it depends on website.

Personally, I think this is a bug in Twitter's login support. They should probably validate the redirect URL, and as soon as they see any character outside /[0-9a-z\/.]/i strip that and everything that follows from the URL they put in redirect_after_login . It may be worth contacting them about this.

I'll attach a patch but I think we should open up this bug. Mark, let me know if you agree. :-)

Comment 10

[:Gijs (he/him)]

Attachment: Patch v1

Comment 11

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8971183 [details] [diff] [review]
Patch v1

Review of attachment 8971183 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Should we add a testcase similar to toolkit/components/reader/test/browser_bug1453818_samesite_cookie.js ? If so, I'll follow up.

Thanks!

Comment 12

[Mark Goodwin [:mgoodwin]]

(In reply to :Gijs (he/him) from comment #9)
> I'll attach a patch but I think we should open up this bug. Mark, let me
> know if you agree. :-)

I do agree - though maybe that's dveditz' call?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Thanks for fixing Gijs!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe6dcb67de4
use triggering principal when leaving reader mode, r=mgoodwin

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7fe6dcb67de4

Comment 16

[Daniel Veditz [:dveditz]]

Unfortunately this doesn't meet the severity criteria for the bug bounty

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Given where we are in the ESR60 lifecycle, is this something we should consider taking there too (yes, I'm aware of the severity)?

Comment 18

[:Gijs (he/him)]

Comment on attachment 8971183 [details] [diff] [review]
Patch v1


(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Given where we are in the ESR60 lifecycle, is this something we should
> consider taking there too (yes, I'm aware of the severity)?

It's simple enough that I think that would work, yeah.


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
See previous comments. Simple fix, minor security impact, safe to take.

User impact if declined: potential samesite security issues
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): low, small localized change to reader mode that ensures we pass the right principal when navigating
String or UUID changes made by this patch: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8971183 [details] [diff] [review]
Patch v1

Small hardening mode patch for Reader Mode. Well-baked on Nightly/Beta. Approved for ESR 60.1.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f620aed6dc0d