Closed Bug 1453818 Opened 3 years ago Closed 3 years ago
Site cookie bypass via Reader view
59 bytes, text/x-review-board-request
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.
I am looking into this.
(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.
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?
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.
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 :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.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
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
(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. :-)
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/fd0857bfc9d4 Add test for same-site cookies in reader mode. r=ckerschb,Gijs
Backed out for ESlint failure on browser_bug1453818_samesite_cookie.js Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fd0857bfc9d4777ed7039024c02d39e223b5bf08&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=174626989 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174626989&repo=autoland&lineNumber=276 and a TV fail https://treeherder.mozilla.org/logviewer.html#?job_id=174629563&repo=autoland&lineNumber=2085 Backout link: https://hg.mozilla.org/integration/autoland/rev/897e4a639ef9cb8e1b4f6b0c17f3aa9bd9fa2bd7
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/aabb1f930a36 Add test for same-site cookies in reader mode. r=ckerschb,Gijs
Let's get these tests uplifted to Beta also.
Whiteboard: [domsecurity-active] → [domsecurity-active][checkin-needed-beta]
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.
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).
You need to log in before you can comment on or make changes to this bug.