Closed Bug 1439879 Opened 3 years ago Closed 3 years ago
navigations that redirect from a controlled scope to uncontrolled scope do not clear their controller in e10s mode
+++ This bug was initially created as a clone of Bug #1433454 +++ Crash report bp-ce361a64-3475-42dc-b35c-1d4ef0180217 ============================================================= Top 10 frames of crashing thread: 0 libxul.so mozilla::dom::ClientSource::WindowExecutionReady(nsPIDOMWindowInner*) [clone .cold.182] 1 libxul.so nsGlobalWindowInner::ExecutionReady dom/base/nsGlobalWindowInner.cpp:1918 2 libxul.so nsGlobalWindowOuter::SetNewDocument(nsIDocument*, nsISupports*, bool) 3 libxul.so nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) 4 libxul.so nsDocumentViewer::Init layout/base/nsDocumentViewer.cpp:666 5 libxul.so nsDocShell::SetupNewViewer(nsIContentViewer*) 6 libxul.so nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) 7 libxul.so nsDocShell::CreateContentViewer(nsTSubstring<char> const&, nsIRequest*, nsIStreamListener**) 8 libxul.so nsDSURIContentListener::DoContent(nsTSubstring<char> const&, bool, nsIRequest*, nsIStreamListener**, bool*) 9 libxul.so nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) 10 libxul.so nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) ============================================================= This crash happens on my profile in the tab that is opened, whenever I (left-)click outgoing YouTube links from video descriptions. Interestingly enough, it doesn't happen for outgoing twitter.com links, but every other outgoing link I've tried crashed. I cannot reproduce this on a fresh profile, even after adding some of my add-ons and changing preferences that I thought could be connected in accordance with my main profile. Logging into my google account also didn't help in trying to reproduce on the fresh profile.
Can you please open "about:support" in the affected profile, copy to text, and then paste here? Thanks!
Thanks. Can you try safe mode to see if it helps? https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode Trying to isolate if its an addon or a preference. Sorry for having so many things to check.
Still happens in safe mode: bp-8e85e176-280b-4e88-93b2-590150180221
Hmm. We also have a way to set permissions on a per-site basis. Can you go to: Menu -> Options -> Privacy & Security -> History Change the combobox from "Remember History" to "Use custom settings for history". Then click the "Exceptions..." button. This should bring up a dialog that lists sites with overridden permissions. Do you have any per-site permissions in there?
Although these prefs might be enough to somehow trigger this: network.cookie.cookieBehavior: 3 network.cookie.lifetimePolicy: 2
Also, can you provide me a link to the youtube page you are using to trigger this? Are you logged in on youtube?
I already use custom settings and I have dozens of sites in there. http(s)://youtube.com are "Allow first party only". I'm logged in on youtube and reproduce e.g. with https://www.youtube.com/watch?v=9o-77YQ1Rcc expand description and click on the http://ShopDeFranco.com link -> new tab opens and immediately crashes.
Interesting. I'll have to do some research. I don't see a way from the UI to set "Allow first party only" for a site. But I do see we have support for displaying it.
I think I can write a test to try to provoke this using SpecialPower.pushPermissions passing ACCESS_ALLOW_FIRST_PARTY_ONLY. I'll probably need to look at that another day, though. It would be interesting to know if the crash goes away if you clear that one permission. Of course, I don't know how to add the permission back if you do that.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Yes, the crash has gone away once I removed the youtube exceptions. Not sure how this was initially set, probably by some add-on.
I can reproduce on youtube. Its a combination of: 1. Setting cookie options to "Keep until I close" 2. AND having "Allow first party only" for youtube I had to manually hack the permissions.sqlite file to get "Allow first party only".
It seems when we have this permission set for a site we ignore the "keep until I close" setting that is browser-wide: https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/dom/base/nsContentUtils.cpp#9028 This allows us to get a service worker on youtube.com when most sites won't have a service worker because of the "keep until I close". Then when youtube does its weird redirect thing we must be hitting an unexpected path that inherits the service worker early, but then later sees it shouldn't have it because of the global "keep until I close" setting.
Johnathon, this bug is triggering a diagnostic assertion when the browser is configured as described in comment 12. I'm trying to figure out the best way to deal with this. One approach would be to remove the support for "Allow first party only" permission. We could migrate profiles that currently use it to ACCEPT or just default back to the global setting. AFAICT we don't expose "Allow first party only" in the UI any more and we don't have many tests that set it. If we're not going to actively support the feature, maybe it would be best to remove it for now. Alternatively, I could add a test that uses it and try to figure out a workaround for the problem. I can do this, but I have a hard time justifying it if "Allow first party only" is not really going to be used. Do know our plans for "Allow first party only" permission? What do you think about removing it for now?
I don't think we have any plans to remove this UI right now. Dan might know more about the area of code to comment on the crash.
Flags: needinfo?(jkt) → needinfo?(dveditz)
When was the "Allow first party only" added to the exceptions UI? And when was it removed from visibility? Was it part of the first-party-isolation project, or something else? I'm not sure who would know the answers to these questions, so needinfo'ing a bunch of people.
Looks like it's not anything Tom ni for as it came in from Monica in 2013: https://bugzilla.mozilla.org/attachment.cgi?id=720868&action=diff
Support for this at the DB level was added in bug 770691 and UI for it landed in bug 770705 but it went away when about:permissions was removed.
(In reply to Jonathan Kingston [:jkt] from comment #15) > I don't think we have any plans to remove this UI right now. There is no way to set it in the UI today. I had to hack the permissions.sqlite to even get a browser to try to use this feature. I assume it was exposed via a legacy addon or previous UI in the past, though. The crash is specific to assertions I added for storage and service workers. We don't have good test coverage of this feature in general, partly because we don't let anyone set it normally.
(In reply to François Marier [:francois] from comment #18) > Support for this at the DB level was added in bug 770691 and UI for it > landed in bug 770705 but it went away when about:permissions was removed. And it looks like about:permissions was removed two years ago in bug 933917.
(In reply to Ben Kelly [:bkelly] from comment #13) > It seems when we have this permission set for a site we ignore the "keep > until I close" setting that is browser-wide: Any explicit setting for a site--whether that's block, allow, allow-for-session, or allow-first-party-only--overrides the browser-wide cookie setting. Allow-first-party-only is a setting we'd like to support but we've been frustrated by changing front end UI. I would guess the security team agrees with annevk in bug 933917 comment 3 that removing about:permissions should have been WONTFIXed until there was a replacement. It was considered "OK" to do because you could manipulate permissions through the site identity panel, but then that got revamped since such that you really can't anymore. You can clear non-default settings but not otherwise change cookie settings there. We do have plans to add UI support for this to the cookie exceptions dialog you can reach from about:preferences, it's just pretty far down the Security team's list. In any case, I can reproduce this crash by setting youtube cookies to "Allow" and the global setting to save cookies until I quit. This is easily accomplished with our existing UI and is a common setting: clear out the cruft on shutdown but stay logged in to sites you use all the time. These are the settings I use, in fact, with "Allow" for gmail and bugzilla cookies rather than youtube.
I think it's all been said, we might add this to the cookie exceptions, but it's not an immediate plan.
(In reply to Daniel Veditz [:dveditz] from comment #21) > We do have plans to add UI support for this to the cookie exceptions dialog > you can reach from about:preferences, it's just pretty far down the Security > team's list. Is there a bug on file for this? Even if its not being worked now, it might be nice to have it open to signal the intent to keep it. Right now this feature seems pretty well abandoned by looking at the code in the tree. > In any case, I can reproduce this crash by setting youtube cookies to > "Allow" and the global setting to save cookies until I quit. This is easily > accomplished with our existing UI and is a common setting: clear out the > cruft on shutdown but stay logged in to sites you use all the time. These > are the settings I use, in fact, with "Allow" for gmail and bugzilla cookies > rather than youtube. Ah, thanks. I will focus on this use case then. Sorry for getting side tracked on the unusual permission setting.
(In reply to Daniel Veditz [:dveditz] from comment #21) > Any explicit setting for a site--whether that's block, allow, > allow-for-session, or allow-first-party-only--overrides the browser-wide > cookie setting. Also, I just want to say that having the global setting vary on two dimensions: * Accept/Block/Block Foreign * Keep until expire/browser closed And then having the override only be a single value: * Accept/Block/Keep until closed Is really weird and confusing.
The assertion was added in bug 1425975 so this affects FF59. Note, however, the assertion only fires in nightly and dev-edition builds. In Release and beta we will not crash, but some sites may be covered by a service worker even though the user has configured things to block storage.
Investigating a potential security issue here.
I'm doing some instrumentation at the point of the assertion: ### ### [0x7f0fc7171a00] ClientSource::WindowExecutionReady origin:https://teespring.com current: new:https://teespring.com/stores/defranco-top-sellers controller:https://www.youtube.com/ Assertion failure: nsContentUtils::StorageAllowedForWindow(aInnerWindow) == nsContentUtils::StorageAccess::eAllow, at /srv/mozilla-central/dom/clients/manager/ClientSource.cpp:272 This shows that the service worker controller is for a different origin than the window. This is very bad. I need to investigate further how its happening.
In nightly 60 I am able to follow the steps in comment 8. With default cookie prefs this does not crash. But using the console you can see that the teespring.com site ends up controlled by the youtube service worker. Fortunately it does not occur in FF58 or FF59.
I have a theory about what is happening here. My guess is: 1. Youtube is opening a page on its origin and gets its service worker 2. YT then does document.open() to the target cross-origin site 3. This creates a new inner window and ClientSource 4. The channel that loads the new document is created with nsIContentPolicy::TYPE_OTHER and the original caller document as its node. 5. We then set the original caller document's service worker controller on the load info because we think its a subresource load here: https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/netwerk/base/LoadInfo.cpp#135-142 6. This results in the old controller being propagated to the new document incorrectly. I will need to confirm this tomorrow. Christoph, do you know why document.open() is being loaded with nsIContentPolicy::TYPE_OTHER()? This seems a bit wrong to me. It was added in bug 1038756. Ideally I'd like to make document.open() use TYPE_DOCUMENT. If that is not good for some reason, maybe we could make a TYPE_INTERNAL_DOCUMENTOPEN. I will also be adding some new diagnostic asserts to prevent this kind of mismatch in the future.
TYPE_OTHER is being set for document.open() here: https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/dom/html/nsHTMLDocument.cpp#1493
(In reply to Ben Kelly [:bkelly] from comment #29) > Christoph, do you know why document.open() is being loaded with > nsIContentPolicy::TYPE_OTHER()? This seems a bit wrong to me. It was added > in bug 1038756. I couldn't find a specific reason why we would load window.open() with TYPE_OTHER instead of TYPE_DOCUMENT. I agree that we should change that to make sure window.open is loaded with TYPE_DOCUMENT and if that is not possible for any reason, I would agree to adding TYPE_DOCUMENT_INTERNAL or something that then resolves to TYPE_DOCUMENT. I think that was not intentional and I couldn't find any reason by browsing through Bug 1038756. Is it possible that this channel does not do the actual load for window.open() but rather the docshell is performing that? The only thing I don't understand is the following. If we are really loading window.open() with TYPE_OTHER and a regular webpage would use a strict CSP using default-src 'none' then opening the window would be blocked by CSP because TYPE_OTHER is governed by default-src. It seems we don't have any CSP test exercising that hence I created Bug 1440582 to write some tests for that scenario.
Thanks for digging into it. Just to clarify, this is about document.open() and not window.open(). I believe they take different code paths.
I've tried some initial testing this morning and I don't think its as simple as comment 29 suggests. The TYPE_OTHER channel is not for a URL load. Its for a document.open() without a URL. Sorry for the confusion.
Upon further investigation this is actually fallout from bug 1431847. When a document load occurs the controlling ServiceWorkerDescriptor will get added to the LoadInfo by ServiceWorkerManager. If a redirect occurs, though, we clear this controller in ClientChannelHelper. This ensures that we will lookup the next possible matching ServiceWorkerDescriptor on the redirect. In bug 1431847 I started adding support to perform service worker interception in the parent. This involves serializing the controlling ServiceWorkerDescriptor from child-to-parent and then back from parent-to-child. There is a bug, however, in that we only add the ClientChannelHelper in the child process. So redirects on the parent do not get the controller cleared properly. We'll need to fix this, but in theory it should not have been a major problem since this entire parent-side intercept is disabled behind a pref. The problem, however, is that the code to copy ServiceWorkerDescriptor from parent-to-child is performed regardless of pref. And with the controller being cleared on redirect only on the child, this code was reapplying the controller from the parent each time. The quick fix here is to disable the parent-to-child controller copying when the pref is disabled.
Marking this sec-critical since its a same-origin policy bypass. This only effects nightly 60.
Note we have a test that covers this situation: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/navigation-redirect.https.html We didn't catch the problem because: 1. The test does not explicitly check if the resulting iframe is controlled. 2. We do not have assertions catching the cross-origin controller. 3. The test does not use the non-standard cookie prefs, so we did not trigger the crash reported in this bug. I filed bug 1440705 to add the assertions in (2). I will look at possibly enhancing the test for (1) as well, but not in this bug.
Component: DOM → DOM: Service Workers
Summary: Crash [@ mozilla::dom::ClientSource::WindowExecutionReady ] on outgoing YouTube description links → navigations that redirect from a controlled scope to uncontrolled scope do not clear their controller in e10s mode
I have confirmed locally that the assertions I have in mind for bug 1440705 do catch this problem on navigation-redirect.https.html. I have also confirmed adding the additional preference checks also fix the problem. Patch coming here shortly.
Comment on attachment 8953527 [details] [diff] [review] Add some more SW preference checks to e10s http channel code. r=asuth Andrew, this patch wraps all of the code that passes the controller from parent-to-child in checks for ServiceWorkerParentInterceptEnabled().
Attachment #8953527 - Flags: review?(bugmail)
Comment on attachment 8953527 [details] [diff] [review] Add some more SW preference checks to e10s http channel code. r=asuth This is not reviewed yet, but I'm flagging now since I will be mostly out this afternoon. [Security approval request comment] How easily could an exploit be constructed based on the patch? I think moderately difficult. Its not obviously about redirects, but does point to service workers and networking. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I don't think so. Its clearly closing some behavior off related to e10s networking, but its unclear about the effect and that a redirect is needed. Which older supported branches are affected by this flaw? None. This was introduced in bug 1431847 which is only on ff60 trunk. If not all supported branches, which bug introduced the flaw? Bug 1431847. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? N/A How likely is this patch to cause regressions; how much testing does it need? Minimal risk. Its disabling code that behind a pref that is default off. Before adding the pref this code did not exist. I have run tests locally against the patch, but I have not done a try push.
Comment on attachment 8953527 [details] [diff] [review] Add some more SW preference checks to e10s http channel code. r=asuth Review of attachment 8953527 [details] [diff] [review]: ----------------------------------------------------------------- This makes sense in the context of the excellent comment 34, thank you.
Attachment #8953527 - Flags: review?(bugmail) → review+
Comment on attachment 8953527 [details] [diff] [review] Add some more SW preference checks to e10s http channel code. r=asuth Actually, looking at the sec bug approval process: https://wiki.mozilla.org/Security/Bug_Approval_Process#Process_for_Security_Bugs I don't think I need sec-approval here. I think this bug meets the conditions for case B. We know what caused this, it does not affect other branches, and we have not shipped the problem on anything other than nightly.
2 years ago
You need to log in before you can comment on or make changes to this bug.