Bug 1456652 (CVE-2018-12370)

SameSite cookie Reader view patch bypass

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: s.h.h.n.j.k, Assigned: Gijs)

Tracking

(Blocks 1 bug, {sec-low})

60 Branch
mozilla61
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 fixed, firefox59 unaffected, firefox60 wontfix, firefox61 fixed)

Details

(Whiteboard: [domsecurity-active][adv-main61+])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Flags: needinfo?(ckerschb)
(Reporter)

Comment 1

a year ago
Sorry, Actual results and expected results are opposite. I'm struggled because Monorail and Bugzilla has opposite questions :(
302 to mgoodwin.
Flags: needinfo?(ckerschb) → needinfo?(mgoodwin)
(Reporter)

Comment 3

a year ago
Sorry for the spam, but you need to visit https://shhnjk.azurewebsites.net/SameSite.php to set SameSite cookie before trying repro steps.
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Security
Product: Firefox → Core
Version: unspecified → 60 Branch
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Comment 5

a year ago
(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?
(Reporter)

Comment 6

a year ago
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.
Flags: sec-bounty?
(Assignee)

Comment 7

a year ago
(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.
Flags: needinfo?(s.h.h.n.j.k)
(Reporter)

Comment 8

a year ago
(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.
Flags: needinfo?(s.h.h.n.j.k)
(Assignee)

Comment 9

a year ago
(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. :-)
(Assignee)

Comment 10

a year ago
Posted patch Patch v1Splinter Review
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

a year ago
Attachment #8971183 - Flags: review?(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!
Attachment #8971183 - Flags: review?(mgoodwin) → review+
(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?
Flags: needinfo?(mgoodwin) → needinfo?(dveditz)
Group: dom-core-security
Flags: needinfo?(dveditz)
Thanks for fixing Gijs!
Priority: -- → P1
Whiteboard: [domsecurity-active]

Comment 14

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe6dcb67de4
use triggering principal when leaving reader mode, r=mgoodwin

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7fe6dcb67de4
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Unfortunately this doesn't meet the severity criteria for the bug bounty
Flags: sec-bounty? → sec-bounty-
Keywords: sec-low
Given where we are in the ESR60 lifecycle, is this something we should consider taking there too (yes, I'm aware of the severity)?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 18

10 months ago
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8971183 - Flags: approval-mozilla-esr60?
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.
Attachment #8971183 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main61+]
Alias: CVE-2018-12370
You need to log in before you can comment on or make changes to this bug.