1639478 - Assertion failures triggered by extensions CSS injected into a webpage (through manifest or tabs.insertCss) with system-principal restrictions from Bug 1613609

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

With the system-principal restriction applied in Bug 1613609, an extension that injects a CSS from a moz-extension is triggering in the content process the following assertion failure:

Assertion failure: false (SystemPrincipal must not load remote documents.), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityManager.cpp:864

In a debug build the content process crashes itself because of the assertion, while in a non debug build the page loading seems to get stuck (on any website, see Bug 1639429)

The regressing change from Bug 1613609 has been currently backed out, but this is something to take into account before landing those changes again.

We should also investigate why none of the existing test cases seems to have caught this when Bug 1613609 patches have been landed.

By triggering the issue using the "multi accounts container" extensions (and the loading a webpage, e.g. https://bugzilla.kernel.org), the JS stack from the content process where the assertion is triggered is the following one:

0 CSSCache/<(url = ""moz-extension://UUID/css/content.css"") ["resource://gre/modules/ExtensionContent.jsm":226:11]
1 get(key = ""moz-extension://UUID/css/content.css"") ["resource://gre/modules/ExtensionUtils.jsm":104:19]
    this = [object Map]
2 get(url = ""moz-extension://UUID/css/content.css"") ["resource://gre/modules/ExtensionContent.jsm":127:24]
    this = [object Map]
3 loadCSS/<(url = ""moz-extension://UUID/css/content.css"", "0", "moz-extension://UUID/css/content.css") ["resource://gre/modules/ExtensionContent.jsm":382:45]
4 map(callbackfn = [function]) ["self-hosted":240:0]
    this = moz-extension://UUID/css/content.css
5 loadCSS() ["resource://gre/modules/ExtensionContent.jsm":382:20]
    this = [object Object]
6 preload() ["resource://gre/modules/ExtensionContent.jsm":386:9]
    this = [object Object]
7 preloadContentScript(contentScript = "[object WebExtensionContentScript]") ["resource://gre/modules/ExtensionProcessScript.jsm":406:57]
    this = [object Object]

the C++ stack trace:

#0  0x00007ffff7b05361 in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffff3810, rem=rem@entry=0x7fffffff3810)
    at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:48
#1  0x00007ffff7b0aeb7 in __GI___nanosleep (requested_time=requested_time@entry=0x7fffffff3810, remaining=remaining@entry=0x7fffffff3810) at nanosleep.c:27
#2  0x00007ffff7b0adee in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55
#3  0x00007fffef7f2b6c in ah_crap_handler(int) () at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#4  0x00007ffff07e0e05 in WasmTrapHandler(int, siginfo_t*, void*) () at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#5  0x00007ffff7f7f3c0 in <signal handler called> () at /lib/x86_64-linux-gnu/libpthread.so.0
#6  0x00007fffee0c79f7 in nsContentSecurityManager::CheckAllowLoadInSystemPrivilegedContext(nsIChannel*) ()
    at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#7  0x00007fffee0c8026 in nsContentSecurityManager::doContentSecurityCheck(nsIChannel*, nsCOMPtr<nsIStreamListener>&) ()
    at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#8  0x00007fffeb95e235 in nsBaseChannel::AsyncOpen(nsIStreamListener*) () at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#9  0x00007fffee80b721 in mozilla::css::Loader::LoadSheet(mozilla::css::SheetLoadData&, mozilla::css::Loader::SheetState) ()
    at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#10 0x00007fffee81142e in mozilla::css::Loader::InternalLoadNonDocumentSheet(nsIURI*, mozilla::css::Loader::IsPreload, mozilla::css::SheetParsingMode, mozilla::css::Loader::UseSystemPrincipal, nsIPrincipal*, mozilla::Encoding const*, nsIReferrerInfo*, nsICSSLoaderObserver*, mozilla::CORSMode, nsTSubstring<char16_t> const&) ()
    at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#11 0x00007fffee811721 in mozilla::css::Loader::LoadSheet(nsIURI*, mozilla::css::SheetParsingMode, mozilla::css::Loader::UseSystemPrincipal, nsICSSLoaderObserver*) ()
    at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#12 0x00007fffee814fa0 in mozilla::PreloadedStyleSheet::PreloadAsync(mozilla::NotNull<mozilla::dom::Promise*>) ()
    at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
#13 0x00007fffee90c4af in nsStyleSheetService::PreloadSheetAsync(nsIURI*, unsigned int, JSContext*, JS::MutableHandle<JS::Value>) ()
    at /home/rpl/mozlab/projects/investigate/mc-investigate/objdir-frontend-debug/dist/bin/libxul.so
...

Comment 2

[Frederik Braun [:freddy]]

Currently, we send "code as-text" from the parent to the child [1] and wrap it into a data URL [2] that is loaded with the system principal [3]. Emilio explained the reason we use the SystemPrincipal for styles is that the rules are opaque to the content and do not leak to websites with e.g., cssRules.

Instead of wrapping text in a data URL, I suggest we do the following

• before passing from the parent to the content, wrap text in a blob URL with the moz-extension's global
• send blob (instead of text) to the content process
• rewrite the caching logic to use the blob URL as a key or generate a cache-key in the parent
• pass blob URL to sheetloader

[1] message Handler for parent-to-content process https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/toolkit/components/extensions/ExtensionContent.jsm#1258-1281
[2] content process creates data URL on the fly at https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/toolkit/components/extensions/ExtensionContent.jsm#269
[3] using System Principal in Loader at https://searchfox.org/mozilla-central/rev/df4c90d4b8c92c99f76334acfe4813c573c12661/layout/style/Loader.cpp#1556

Comment 3

[Frederik Braun [:freddy]]

I think this could be really necessary for a sound security model where scripts (and resources in general) are attributable to their source.
Luca, can you help us get an estimate on how much work this would be? Was my suggestion on comment 2 reasonable?

Maybe we can even pick this up in the our team's work queue.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Frederik Braun [:freddy] from comment #3)

I think this could be really necessary for a sound security model where scripts (and resources in general) are attributable to their source.
Luca, can you help us get an estimate on how much work this would be? Was my suggestion on comment 2 reasonable?

Maybe we can even pick this up in the our team's work queue.

I guess that we have already considered adding an additional optional parameter to styleSheetService.preloadSheetAsync as an alternative strategy, is that right?
("on paper" that seems like a strategy that may require less changes and less changes for unexpected regressions, but from your comment I'm guessing that we would still prefer to avoid to special case this requests and instead making them more clearly related to the extension that is injecting it)

About the "extension blob url" strategy described in comment 2

I kind of recall that when we originally worked on Bug 1420485 (which was mainly aiming to reduce the memory usage for extension using a lot of calls to tabs.insertCss with a css string) I was also originally proposing to use extension Blob urls (instead of hashing the string as we did in the patch that was actually landed in the end), I think that I changed strategy that in response to an explicit suggestion to "don't use Blobs" but I didn't find any trace of that in the mozreview comments part of that bugzilla issue.

Having said that, in the userScripts API we are actually using blob urls for the js code strings, and so it doesn't seem that using extension Blob urls is something to be considered completely off the table, and so the strategy described in comment 2 may be worth a try.

(and, by following the related code through layout/style/PreloadedStyleSheet.cpp and layout/style/Loader.cpp, it looks that we would load the extension Blob url using a system principal because there is no document associated to that loader, I didn't dig too much but from a quick glance it seems that the strategy you proposed should work, the extension blob url should be loading successfully and still be opaque to the webpage).

As a side note:

• to make the "extension Blob url strategy" to work I think we will have to use a Blob constructor from a Sandbox instance that we are not going to nuke when the related extension global is gone (at the moment all the extension principal Sandbox instances we do already create and use in the main process are all tied to a specific extension global living in the extension child process, and we destroy them when that extension page is gone).
• based on comment 0, none of the existing tests (at least the one we had at the time) was actually able to trigger the issue and/or fail visibly, it would be good to also be sure we can trigger the original issue in a new test case (my apologies, I wanted to look into that but never got to get back to this issue and I forgot)

Comment 5

[Frederik Braun [:freddy]]

Attachment: newplot.png

Given the (hacky) URL annotation in bug 1699101, we found that most stylesheets loaded with SystemPrincipal privileges are indeed from content-styles. That's great news! We would like to bump the priority of rewriting the URLs to blobs. Telemetry is available in https://sql.telemetry.mozilla.org/queries/81971/source#203117, but I'll share a screenshot as an attachment. I'm also happy to join a deeper discussion about how to move forward with this and how Security Engineering can provide support and resourcing.

Comment 6

[Inicio Shea]

Attachment: same problem

Thanks from smucunhoanal1986.