Closed Bug 1825390 Opened 2 years ago Closed 2 years ago

Consider exposing resource://devtools as "content accessible"

Categories

(DevTools :: General, task)

task

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: ochameau, Unassigned)

References

Details

Work on bug 1792803 and bug 1824886 would benefit from exposing the whole resource://devtools URL to the content pages.

Bug 1824886 needs to use devtools/server/actors/highlighters.css from the web pages principal.

And bug 1792803 highlights that the JSON Viewer also loads its modules from the JSON URL's principal and is currently using some custom resource:// mappings bound to a subset of devtools folders (devtools/shared/ and devtools/client/jsonview/). We could drop these mappings in favor of resource://devtools which is used everywhere else in DevTools!

JSON Viewer has been exposeing to content via contentaccessible=yes flags various files from devtools folder, for years:
https://searchfox.org/mozilla-central/rev/0907529ff72c456ddb47839f5f7ba16291f28dce/devtools/shared/jar.mn#18-20
We've not received any report about this.
But still... I'm wondering what could go wrong by exposing most DevTools code to web pages?

Freddy, this is a continuation of our discussion from bug 1792803.

In bug 1792803 comment 7 It looks like you were on the fence of trying to tweak the principal of JSON Viewer to be able to load the currently non-content-accessible resource://devtools URLs. Instead, you suggested to keep using content-accessible URLs.
Emilio is having similar conclusion on bug 1824886 for highlighters.css, using a content-accessible URL would be easier than hacking the trigerring principal.

Now, I'd like to open the question of exposing most DevTools modules as content-accessible, by doing this:

diff --git a/devtools/shared/jar.mn b/devtools/shared/jar.mn
index 8e84209135c0c..6b95706ed58b3 100644
--- a/devtools/shared/jar.mn
+++ b/devtools/shared/jar.mn
@@ -14,10 +14,7 @@ devtools.jar:
 # they're also used in the RemoteNodePickerNotice (which is in devtools/server).
     content/shared/images/command-pick.svg (images/command-pick.svg)
     content/shared/images/command-pick-remote-touch.svg (images/command-pick-remote-touch.svg)
-%   resource devtools %modules/devtools/
-%   resource devtools-client-jsonview resource://devtools/client/jsonview/ contentaccessible=yes
-%   resource devtools-client-shared resource://devtools/client/shared/ contentaccessible=yes
-%   content devtools-jsonview-styles %modules/devtools/client/jsonview/css/ contentaccessible=yes
+%   resource devtools %modules/devtools/ contentaccessible=yes
 # The typical approach would be to list all the resource files in this manifest
 # for installation.  Instead of doing this, use the DevToolsModules syntax via
 # moz.build files to do the installation so that we can enforce correct paths

As you see here, we were already exposing quite a few modules already.
Almost everything from devtools/client/shared/ and devtools/client/jsonview.
I imagine that if exposing files to content is an issue... we might already be having an issue here?

So I supposed this isn't a big deal and exposing everything would be just simplier to maintain and having a unique mapping would be easier to understand.

Now, if that's an issue, we would need to learn about the typical issues raised by exposing files to content.
Because we have not been paying attention and reviewed the files exposed to content so far.
You can identify a subset of modules loaded from JSONView with this query:
https://searchfox.org/mozilla-central/search?q=define%28function%28require%2C+exports%2C+module%29+%7B&path=devtools%2F&case=true&regexp=false
There is also a bunch of React modules from devtools/client/shared/vendor/

For my quest raised in bug 1792803, we are willing to migrate DevTools from Common JS to ES Modules.
Because ES Modules are using absolute URLs to load dependencies and we can't hack/redirect the ES loader to use distinct path/URL based on some environment, each shared module has to be loaded via a unique URL which should work on all environment.
So, if we can't make resource://devtools exposed to content, nor can't make JSONViewer use a resource-privileged principal,
I'd suggest moving all files that need to be loaded from content in new folders in devtools/, with a new dedicated resource://devtools-content-accessible/ URI exposed to content.
This will be surely harder to understand and maintain. But Modules from this folder will always be loaded via this new URI, which will unlock the ESMification. This will also make it super explicit which are the files exposed to content!
It would be about moving things like this:
/devtools/client/jsonview/ => /devtools/content-accessible/client/jsonview/
/devtools/client/shared/vendor/react*.js => /devtools/content-accessible/client/shared/vendor/
/devtools/client/shared/random-json-viewers-deps*.js => /devtools/content-accessible/client/shared/

An alternative would be to somehow implement an allow list in order to cherry-pick some modules to be exposed to content out of the resource://devtools by-default non-content-accessible mapping. But I have no clue how to implement such a thing.

Julian, you are probably the person who had the most experience around this topic in DevTools. Let me know if I missed something or have a another idea.

Flags: needinfo?(jdescottes)
Flags: needinfo?(fbraun)

Based on discussions within DevTools team, if exposing files to the web is to be avoided, we could also investigate some options other than the only one being raised in bug 1792803.
In that other bug, so far, I only suggested aligning to PDF.js current implementation and use request.owner = resourcePrincipal; trick.
The advantage of this is that we would stop exposing devtools file to content! But it may open some new even more complex edgecase in JSON Viewer.
If that's not satisfying either, we could investigate yet other ways around this.
We could investigate redirecting the JSON files URLs to one privileged URI whose document would load the JSON file. The privileged URI would be able to load from resource://devtools/.

Flags: needinfo?(jdescottes)

Dan, can you help evaluate this?

Flags: needinfo?(fbraun) → needinfo?(dveditz)

Gijs, do you have an opinion on what do that, or someone to ping about this?

Note that the decision here should be ideally also applied to PDF.js. The security concerns are about the same except that pdf.js has less modules being exposed.

Flags: needinfo?(gijskruitbosch+bugs)

I'm about to disappear on PTO so don't have a lot of time to dedicate to researching the specific solution you want here for you, and/or to make this a short comment, unfortunately. So I'll try to braindump some stuff in the hope that it is useful. Apologies for the wall-of-text-ness. If you're still stuck in 2 weeks time, ping me again please. I've copied in some other folks who may be of help. For people reading this now: unfortunately discussion / useful information is spread between here and bug 1792803 and (to a lesser extent), bug 1824886, so it's worth checking those if looking for context.

I've copied you in on bug 1600769 which shows at least one avenue via which having arbitrary scripts be content-accessible can hurt us.

In terms of how to architect this, a few goals that come to mind:

  • expose as little to the web as possible. All of it is attack surface as a general rule, even when we can't immediately point to an exact mechanism by which it becomes so (at the very least it's probably a fingerprinting / version determination vector, as these files will change).
  • it should be possible (ie we should write C++ code that allows this if it doesn't exist) to have resources that can be loadable by the JSON viewer or PDF viewer or other in-content experiences provided by the browser, without that immediately implying those resources will also be loadable by the whole wide world (web).
  • websites that provide a JSON/PDF that we then open inline (e.g. in a frame) in our viewer should not end up being same-origin with a document that has these magical extra privileges to load things web content can't (i.e. it should not be the case that the JSON/PDF viewers running our own code end up with the same principal as an HTML page loaded in the same browsingcontext.) - otherwise, the things we expose into these documents end up as web-accessible anyway.

The other puzzle piece here is that :mossop is looking at replacing our use of chrome and resource more generally. If we establish that we basically end up needing:

  1. non-accessible, chrome only stuff
  2. quasi accessible stuff (PDF/JSON viewer)
  3. fully web-content accessible stuff (e.g. UA sheets)

where (1) and (3) already exist as chrome+resource vs. chrome+resource with contentaccessible=yes. I think the current plan is that the content-accessible and non-content-accessible bits become separate protocols altogether (which is doubly nice because it simplifies some security checks). Perhaps (very loosely held opinion, I haven't had time to think about this much) standing up a separate protocol for (2) is an appropriate solution in that scenario. Mossop could probably offer more thoughtful commentary there.

One risk here with creating (2) is some kind of slippery slope: everything ends up moving from (3) to (2) because we end up needing it from JSON viewer / PDF viewer, at which point we might as well not bother and make all of chrome/resource accessible from the magical JSON/PDF viewer doc (but not web content). From a security PoV I would like not to do that, but I can see how for code architecture reasons it's difficult to "segregate" the bit that needs to run in the web content area from 'all the other bits', and how we'd want to reuse code rather than rewrite it "just" for the content-accessible bits. I'm afraid I don't have a good solution for this, short of documenting the reasons we don't want to "just" expose everything and trusting in our reviewer system to make reasonable decisions.

The second bit of info that is perhaps helpful is that although I can't remember what occasion I had to dig into this (maybe it was the <embed> stuff for PDFs? Maybe something else...), the last time I looked at chromium here, what they do for PDFs appeared to be roughly:

  1. blank document whose origin, URL, etc. (presumably also their equivalent of 'principal') matches the request. So https://example.com/example.pdf gets an HTML doc that is ostensibly blank except for an <embed> or similar (maybe in a shadow DOM or whatever, I can't remember off-hand), that is same origin with https://example.com in general
  2. the embed then loads the "magical" viewer bits, and is not same-origin with https://example.com/ (and can't be inspected with devtools).

There are some WPT tests that directly or indirectly depend on some of the mechanics around PDFs, IIRC.

Whereas AIUI we currently don't have this double nesting layer, we just load the magical non-same-origin document in-place. Here's an example page where you can see this when using the chrome devtools vs. our devtools. AIUI right now the JSON and PDF viewer code manipulates the channel principal, e.g. https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/devtools/client/jsonview/converter-child.js#112 and https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/toolkit/components/pdfjs/content/PdfStreamConverter.sys.mjs#1205 .

Perhaps we should investigate the merits of introducing this extra layer of nesting/indirection. It's somewhat orthogonal though, in that the principal/same-origin issue still exists, just 1 BC further down in the browsing context tree.

In terms of the principal for the resulting document, null principal is fundamentally the safest choice (and what past me advocated for in bug 1305012), but in order to then load anything there's no way to distinguish it from null principal'd web content - so to make anything loadable by the resulting doc, we'd have to make anything it needed content accessible, which we don't want to do. We could make that distinction if we used a resource principal, but then (a) the document becomes significantly more powerful in what it can do, which is also unfortunate from a security perspective and (b) all such documents are same origin (including JSON/PDF files from different origins!), which is probably not great.

So I think what we might want is something that is like null principal, but can also load these twilight-zone "neither web content nor completely chrome privileged" files. But adding a new kind of principal for this feels like a significant and/or annoying undertaking. I think we could approximate it by generating a uuid-based content principal for the custom scheme I suggested above (ie generate a new uuid-based content origin for each PDF/JSON load). That would allow it to load resources from that custom scheme, and the uuid would mean none of the docs are same-origin with each other.

:Mossop/:ckerschb, does this seem like a reasonable solution, and/or am I overthinking this? (Quite possible!)

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dtownsend)
Flags: needinfo?(ckerschb)

This is a tricky situation and I don't have a really good answer off the shelve for loading these twilight-zone files. FWIW, we recently added Precursor Principals to NullPrincipals (see Bug 1715167) which is basically a nested ContentPrincipal within a NullPrincipal.

Maybe, again maybe, we could leverage that architecture which could allow us to lock the privileges using a NullPrincipal but then do some semi-privileged things in the twilight zone.

Nika, is such an architecture completely off the table in your opinion? Or might this be an avenue worth exploring?

Flags: needinfo?(ckerschb) → needinfo?(nika)

I don't love the idea of using precursor principals for security in this way. The intention was for it to reflect the original origin of the page which created the null principal so we can do things like origin isolation when working with null principals, and IMO we shouldn't be using it to do a security check like this. If we had a null principal for a JSON viewer document, I'd personally expect its precursor principal to be the principal of the site we loaded the JSON from, so that the correct content process is used.

Off the top of my head, we currently have 3 places where we replace content from the internet with our own document, and in all 3 cases we use a different strategy behind the scenes, so there's certainly not a one-size-fits-all approach here.

  1. When loading a view-source: document, we load using a custom protocol and normal channel. The document is transformed into a view-source document at the final stage when sending the data to the docshell, and the resulting document has the same principal as the original document, but shows a view-source document instead of the original page.

  2. When loading a document in reader mode, it is loaded as about:reader?url=http://example.com. The actual document being displayed is then loaded using a JSWindowActor and independent channel, and the resulting document has the about:reader principal. This makes it completely inaccessible to web content, even if they hold a reference to the window it is loaded in. Because all about:reader pages have the same principal, there is special handling within process selection code to ensure they get the correct process assignment (https://searchfox.org/mozilla-central/rev/d31e56f7b3c2c18b8071a7b2a2fb6b4e01e3d3e8/dom/ipc/ProcessIsolation.cpp#234-236).

  3. When loading the PDF.js viewer, the document is loaded using a stream converter. The URI appears to be the original URI, but the underlying document is loaded with the PDF.js viewer's principal (resource://pdf.js). Due to how late this happens during the load (during a stream converter), during process selection the principal hasn't been tampered with yet, meaning that it is loaded into the original principal's process. All pdf.js files have the same principal.

  4. When loading a JSON viewer, the document is also loaded using a stream converter. The URI appears to be the original URI, but the underlying document ends up being loaded with a unique null principal. Like with pdf.js, this happens late enough during the load that process selection sees the pre-transform URL, and the load happens based on the original principal's process. All JSON viewers have separate principals.


I believe this has been somewhat hashed over in other places (like bug 1792803), but the simplest approach is likely to make the JSON viewer act more like the PDF.js viewer, create an explicit document on a resource:// URL, force the use of that principal, and load from there rather than building a string literal in the stream converter. That unfortunately doesn't give us a unique origin for each load of the URI, but is consistent with PDF.js, and still prevents web content from accessing the window.

As far as I can tell, the special principal type you're hoping for is effectively an [ExpandedPrincipal moz-nullprincipal:{...}?http://example.com moz-twilight-zone://] (an expanded principal is a unique principal with combined access to all listed URLs, we mostly use them for webextension content scripts these days). We don't support loading a document with an expanded principal however (and there's lots of code which depends on that), so using one of those is certainly not a plug & play option.

If we're trying to stick within principal types which are already supported for documents, the simplest option is probably using a new UUID-based content principal scheme like :gijs suggested. This would likely require at least some special casing though, as we'd need to keep track of both a UUID and the original origin which should be used for process selection purposes (currently we use these principals late enough that it doesn't matter, but that might change in the future). We might be able to add some special-casing to include the query in the origin, like we do for moz-nullprincipal, meaning it could look like moz-twilight-zone-nullprincipal://{...}?http://example.com.

This could also be done as a variant of a null principal rather than a content principal. As I mentioned above, I'd prefer not to use the precursor field for this, so we'd need to make it a new thing. I suppose it'd probably be possible to add an additional flag into the null principal to indicate that it should be allowed to access information from an additional scheme, though it feels a touch odd to have a null principal document with elevated permissions compared to normal web content.

Finally it might be possible to allow the access using some other mechanism in parallel to the principal, though that runs the risk of needing to add, trust, and verify a new flag everywhere.

I haven't developed a strong opinion on this at the moment, so currently just presenting the obvious options.

Flags: needinfo?(nika)

Gijs, Christoph, Nika, Thanks a lot for this valuable feedback!

It sounds like getting rid of content-accessible is a key take away.
First, this prevents random webpages from exploiting this content in various ways (example bug 1600769).
But also, it has the side effect of forcing us to use a distinct and privileged principal for the involved browser feature.
The distinct principal is going to increase security by ensuring that the replaced document from the random web can't have access to the browser feature JS code. By the sole fact that the principals are different.
And the privileged principal will allow the browser feature to have access to resource://, or similar browser scheme.
Also, on the long term, this principal could be fine tuned to help platform during the process selection step.

Having said that, it sounds like we have to get back to bug 1792803 comment 4 and bug 1792803 comment 7.
I was about to make JSONViewer use the exact same pattern as PDF.js in D158318.
by reusing this trick:
https://searchfox.org/mozilla-central/rev/bf87e869ece2406e9e425a74e8bf77aba0975625/toolkit/components/pdfjs/content/PdfStreamConverter.sys.mjs#1149-1162

    var uri = lazy.NetUtil.newURI("resource://pdf.js/web/viewer.html");
    var resourcePrincipal =
      Services.scriptSecurityManager.createContentPrincipal(
        uri,
        aRequest.loadInfo.originAttributes
      );
    aRequest.owner = resourcePrincipal;

I've been discouraged from using this trick, but this isn't clear if UUID-based content principal would allow setting the right principal differently than this. Would it? Any idea how?
Otherwise, if we still have to change the principal via nsIChannel.owner attribute, then it isn't crystal clear for me what is the benefit? The resource principal is distinct from the web origin and should disallow any interaction between the web origin JS and the browser feature.
If that's any useful, then could such principal be instantiated via nsIScriptSecurityManager.createContentPrincipal(FromOrigin)? If not, could it be done by instantiating ContentPrincipal class differently, or would it require implementing a subclass, or something?

Nika, see previous comment. (FYI, I'm also hoping to hear back from gijs getting back from pto later this week)

Flags: needinfo?(nika)

(In reply to Alexandre Poirot [:ochameau] from comment #9)

Otherwise, if we still have to change the principal via nsIChannel.owner attribute, then it isn't crystal clear for me what is the benefit? The resource principal is distinct from the web origin and should disallow any interaction between the web origin JS and the browser feature.

It probably got lost in my wall of text, but:

We could make that distinction if we used a resource principal, but then (a) the document becomes significantly more powerful in what it can do, which is also unfortunate from a security perspective and (b) all such documents are same origin (including JSON/PDF files from different origins!), which is probably not great.

In addition to these reasons, (c) figuring out the principal corresponding to the source of the document (ie content principal of https://example.com/ for a load of https://example.com/file.pdf) is tedious and special-case-y (there's a hashmap value set that we read in some cases, but IIRC it's only accessible same-process, and right now it's PDF-js specific) and error-prone (ie if someone implements a new feature that needs the document's principal and they don't know about this weird edgecase, and "just" use the documentPrincipal exposed via the Browsing/WindowContext machinery, we don't get what we need/want).

So standardizing on some new kind of principal that means "like null principal but by the way the original one was this website, and also I want to be able to load resource scripts" as a separate kind of principal would be a more explicit way of encoding what's happening, fixing (a) and (b) above, and mitigating (c) because shared "given a principal, what domain string should I display to the user" code can be implemented once (maybe similar to this code for URIs), instead of needing increasing amounts of context/duplication in order to deduce the "right" string).

Anyway, in the immediate term, I don't think these reasons are strong enough not to "just" switch to the PDF.js solution for now, if that indeed solves your problem.

The channel "owner" stuff is definitely weird, but it would work like it does for pdfjs as a near-term solution. Beyond that I don't know if I have much to add on top of :gijs's comment above.

Flags: needinfo?(nika)

Thanks a lot Gijs, Nika for the feedback. comment 11 really helped understand the whole story around this new kind of principals.

I'll migrate JSONViewer to the owner thing and use resource principal in the meantime.

Now I'm curious about how to implement such new principal.
If you have some, even vague, idea as to how to implement such thing... I can give it a try if that doesn't require extensive C++ expertise.

Flags: needinfo?(dveditz)
Flags: needinfo?(dtownsend)

Bug 1792803 will migrate JSONViewer to resource://devtools/ and I'll remove the resource://devtools-client-jsonview resource://devtools-client-shared` which were using contentaccessible flag.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.