Closed Bug 1453818 Opened 6 years ago Closed 6 years ago

SameSite cookie bypass via Reader view

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + verified
firefox61 --- verified

People

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

References

Details

(Keywords: sec-low, Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

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

Steps to reproduce:

1. Go to https://shhnjk.azurewebsites.net/SameSite.php (Sets SameSite cookie)
2. Go to https://test.shhnjk.com/trigger_read.html
3. Click on Reader view.
4. Click on CLICK ME link.


Actual results:

SameSite Strict cookie sent.


Expected results:

Link click from Attacker site shouldn't send SameSite Strict cookie to Victim site. This is implemented correctly in normal webpage, but it's not in Reader view.
Mark/Christoph, I hear you know about our samesite cookie implementation. Can you take a look? I tried to find the offending code (I assume we're somehow treating about:reader as trusted when we shouldn't) but didn't manage.
Flags: needinfo?(mgoodwin)
Flags: needinfo?(ckerschb)
Flags: needinfo?(ckerschb)
I am looking into this.
Flags: needinfo?(mgoodwin)
Christoph asked me to summarize my concerns.

I should note I haven't debugged this, just looked at some code.

My impression is that the combination of trying to figure out if something is third party through this:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#707

given this:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#113-117

means we presumably only ever say something is third party here:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#502-519

which will fail for any non-file URIs that don't produce a base domain. I believe this also affects other vectors beside reader mode, like sandboxed iframes (which will have a null principal, which means we bail out in GetURIFromWindow, which means we never mark as non-same-site). I tried to test this, and initial tests seem to suggest that my suspicion is correct.

Other vectors that we should keep an eye on:
- anything else that gets a null principal (e.g. json viewer)
- anything else that gets a codebase principal for a non-http/https/file URI (so about: URIs as in this case, but probably worth checking if there's cases where we get javascript: URIs as the document URI (x-ref bug 836567) or blob: URIs)

Fundamentally, I think the correct fix is to make the GetURIFromWindow code failing cause the later code to mark as third-party. I don't know if there will be a lot of fall-out, but I don't think we have a lot of options short of that, or we'll just be playing whack-a-mole a lot. If there are other places that rely on the third party stuff that we don't want to touch, we could look at making NS_IsSameSiteForeign say "yes" for any channel / URI that isn't http/https, and only delegate for things where we know nsIThirdPartyUtil gives us the "right" answer.
(This should move to Core::DOM:Security but I can't do that and not get it in the wrong sec group)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Here is a WIP patch, but it doesn't address the problem really, but it shows where in the code we have to modify code to make that work correctly.

Two observations:
a) I think we should use IsThirdPartyURI instead of IsThirdPartyChannel because IsThirdPartyChannel does all sorts of weired things.

b) We could treat about: URIs as third party. Gijs suggested we could do that to fix that specific problem but he also mentioned problems regarding sandboxed iframes. E.g.
data:text/html,<iframe sandbox src="https://test.shhnjk.com/trigger_read.html">
also receives the cookie

c) I think this is also a problem for secure cookies. Using a similar mechanism like this setup but using http instead of https.
Group: firefox-core-security → core-security
Component: Untriaged → DOM: Security
Product: Firefox → Core
(In reply to :Gijs (he/him) from comment #3)
> Fundamentally, I think the correct fix is to make the GetURIFromWindow code
> failing cause the later code to mark as third-party. I don't know if there
> will be a lot of fall-out, but I don't think we have a lot of options short
> of that, or we'll just be playing whack-a-mole a lot. If there are other
> places that rely on the third party stuff that we don't want to touch, we
> could look at making NS_IsSameSiteForeign say "yes" for any channel / URI
> that isn't http/https, and only delegate for things where we know
> nsIThirdPartyUtil gives us the "right" answer.

Given that we are about to uplift those changes to 60 I suggest we only fix things within NS_IsSameSiteForeign because changes in there only affect same-site cookies and nothing else. I am worried that if we change something in thirdpartyUtil it will cause other regressions.

Probably it's just best to explicity check for about reader and use IsThirdPartyURI instead of IsThirdPartyChannel which I could imagine also fixes the nullPrincipal problem.
Group: core-security → dom-core-security
Comment on attachment 8967768 [details] [diff] [review]
bug_1453818_same_site_about_reader.patch

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

::: netwerk/base/nsNetUtil.cpp
@@ +2160,5 @@
>      return false;
>    }
>  
> +  nsCOMPtr<nsIURI> channelURI;
> +  NS_GetFinalChannelURI(aChannel, getter_AddRefs(channelURI));

Is this (channel URI vs. passing the actual channel with loadinfo etc.) actually better for things like blob: or about:blank where we inherit a principal?
See Also: → 1454027
Jun's original bug wrt reader mode seems like a sec-low. No scripting in reader mode so an attack would require convincing a user to _use_ reader mode and then click on links. I'm sure it could happen, but not reliably.

I'm more concerned about what this means for other scenarios like the ones Gijs mentioned in comment 3, and I'd like to see tests for them. The core concept is "Site for cookies" https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.2.1 (note that "registered domain" is our eTLD+1 -- that tripped me up in the past)

Anything document that doesn't have a domain has an empty string as the site for cookies. That can't match the requests domain (assuming an http request) so those are always going to be cross-site requests.
  file:// urls
  about: urls    (special case: about:blank, about:srcdoc inherit domain)
  blob: urls
  data: urls

We have to special-case typing into the addressbar (site for cookies is "null", not the empty string) and clicking on bookmarks. Those shouldn't count as cross-site requests. But I don't think it matters if we treat link clicks from about:newtab or the actual about:home page as cross-site requests. Might even be safer in the possibility of a malicious snippet slipping into the system. But I'm open to arguments that we should treat those as "browser UI". The Old newtab was essentially bookmarks and we'd like those to count as same-site, and the new newtab is still partially bookmarks.

Sandboxed frames and documents _can_ count as same-site. The spec is pretty clear those should use the document's URI rather than the document's unique origin. Need to test this (both <iframe> sandboxes and if possible CSP sandboxes).

Our code does not take that nesting into account so we're not calculating a true "site for cookies", we're trying to approximate from the requesting channel. If the top URL is foo.com and the framed content is bar.com then everything from the framed content counts as cross-site (site for cookies is emptystring). If foo frames bar which frames foo the nested foo also has an emptystring site for cookies, and everything from there will be cross-site. That seems like it's going to really break embedding stuff so we should test that that's really what Chrome does. We can probably spin this off into a separate bug.
Group: dom-core-security
Keywords: sec-moderate
I guess while I was replying above (left to make a pot of coffee and took a break) Jun filed bug 1454027 on the nesting thing.
(In reply to Daniel Veditz [:dveditz] from comment #8)
> Sandboxed frames and documents _can_ count as same-site. The spec is pretty
> clear those should use the document's URI rather than the document's unique
> origin. Need to test this (both <iframe> sandboxes and if possible CSP
> sandboxes).

Right, can but aren't necessarily, and if evil.com can create a sandbox iframe of evil.com and then get the user to click the link in there to go to good.com, we lose. :-(
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Given that we are about to uplift those changes to 60 I suggest we only fix
> things within NS_IsSameSiteForeign because changes in there only affect
> same-site cookies and nothing else. I am worried that if we change something
> in thirdpartyUtil it will cause other regressions.

Hm. I defer to you, Mark, François and Valentin here, but given the spread of issues, and the fact that AFAICT the only consumers of thirdpartyutil are other cookie checks (e.g. if you enable third-party cookie blocking) and tracking protection, I would be worried that if we tried a localized fix, we'd just end up with similar issues being noticed for the other consumers.

For instance, in bug 1453814, I'd wonder if you could bypass tracking protection by just creating 301/302 redirects for any of your tracking scripts and making them same-origin with the base domain of the site. (NB: haven't tried, not at all familiar with tracking protection generally, so could be way off the mark - also I very much doubt we'd treat that as a security problem, just as a bug with tracking protection.)

In a certain semantic sense, I would hope that the number of issues that would arise from making a method more accurately do what the method says it does would be quite limited. Of course practice is more murky. But given this is now public, and that we do still have 1.5 week before beta ships, I think if we move fast we can attempt the more thorough fix. I'm also hoping we have reasonable test coverage for the cookie/TP side of things, though that may be a foolish hope...

For sure for about: URIs specifically, we don't use cookies and TP wouldn't/shouldn't bother us.
(In reply to :Gijs (he/him) from comment #11)
> AFAICT the only consumers of thirdpartyutil are
> other cookie checks (e.g. if you enable third-party cookie blocking) and
> tracking protection

Err, the only consumers of thirdpartyutil::IsThirdPartyChannel and so on, that is - `GetBaseDomain` as a utility function is more widely relied upon via codebaseprincipals.
Flags: sec-bounty?
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
Comment on attachment 8967768 [details] [diff] [review]
bug_1453818_same_site_about_reader.patch

Most likely Bug 1454027 will fix the problems described in this bug as well. However, it would be nice to write a testcase for this problem.

Gijs, do we have a similar test in the tree which I could use to base my patch upon?
Attachment #8967768 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Gijs, do we have a similar test in the tree which I could use to base my
> patch upon?

There are a few browser mochitests in toolkit/components/reader/ that you should be able to use as a basis for a new test, yes. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: ckerschb → francois
Francois, the changes within Bug 1454027 also fix the problem mentioned here. I am going to land Bug 1454027 today. Once landed, please also verify once more that the problem mentioned here works correctly again - thanks!
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> Francois, the changes within Bug 1454027 also fix the problem mentioned
> here. I am going to land Bug 1454027 today. Once landed, please also verify
> once more that the problem mentioned here works correctly again - thanks!

Following the steps from comment 0, I can confirm that this is now fixed in Nightly.
Comment on attachment 8969149 [details]
Bug 1453818 - Add test for same-site cookies in reader mode.

https://reviewboard.mozilla.org/r/237862/#review243680

that looks good to me, but probably gijs should sign off on it too to make sure we got everything correct within that test!
Attachment #8969149 - Flags: review?(ckerschb) → review+
Comment on attachment 8969149 [details]
Bug 1453818 - Add test for same-site cookies in reader mode.

Gijs, can you sign off on that test as well please?
Attachment #8969149 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8969149 [details]
Bug 1453818 - Add test for same-site cookies in reader mode.

https://reviewboard.mozilla.org/r/237862/#review243860

::: toolkit/components/reader/test/browser_bug1453818_samesite_cookie.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

We normally license tests public domain (and yes, I'm aware some of the other tests in this directory aren't good examples).

::: toolkit/components/reader/test/browser_bug1453818_samesite_cookie.js:4
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Nit: please add `"use strict";` at the top after the license header.

::: toolkit/components/reader/test/browser_bug1453818_samesite_cookie.js:48
(Diff revision 1)
> +  let tab = gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
> +  await promiseTabLoadEvent(tab, TEST_ORIGIN1 + "setSameSiteCookie.html");
> +  gBrowser.removeTab(tab);

Please use BTU.withNewTab() here too.

::: toolkit/components/reader/test/browser_bug1453818_samesite_cookie.js:73
(Diff revision 1)
> +  info("Waiting for the page to load in normal mode...");
> +  await pageLoaded;
> +
> +  await clickLink(browser);
> +  await checkCookie(sameSiteEnabled, browser);
> +  gBrowser.removeTab(tab);

Nit: `await BrowserTestUtils.removeTab(tab)`

::: toolkit/components/reader/test/linkToGetCookies.html:7
(Diff revision 1)
> +<html>
> +    <head>
> +        <meta charset="utf-8">
> +    </head>
> +    <body>
> +        <p><a href="http://example.com/browser/toolkit/components/reader/test/getCookies.html" id="link">Cross-origin link to getCookies.html</a></p>

I'm... surprised that this always shows a reader mode icon. In fact, I expect it doesn't, but you're clicking the button in such a way that it works even though the button is not visible.

Can you add some boilerplate from one of the other test files in this directory to ensure the button shows up?
Attachment #8969149 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks for your comments Gijs.

I fixed all of them and also removed an unused CustomEvent I had forgotten to remove in getCookies.html.
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd0857bfc9d4
Add test for same-site cookies in reader mode. r=ckerschb,Gijs
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aabb1f930a36
Add test for same-site cookies in reader mode. r=ckerschb,Gijs
Flags: needinfo?(francois)
https://hg.mozilla.org/mozilla-central/rev/aabb1f930a36
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Let's get these tests uplifted to Beta also.
Whiteboard: [domsecurity-active] → [domsecurity-active][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/81a11d030c5a
Whiteboard: [domsecurity-active][checkin-needed-beta] → [domsecurity-active]
Since the patch for the various issues here ended up as part of bug 1454027 I'm going to set the severity according to the original Reader issue rather than the extended worries.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-moderatesec-low
Flags: qe-verify+
I have managed to reproduce the issue using Fx 61.0a1 with the buildID: 20180321040527.

The issue was tested using Fx 62.0a1 buildID: 20180520220103, Fx 61.0b6 buildID: 20180517141400 and Fx 60.0.1 buildID:  	20180516032328 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 LTS. I can confirm that on those builds the issue is no longer occuring (no cookies are sent when the Click Me link is clicked).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: