1533925 - Allow embedded web extensions to serve/register about: pages in the extension process

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Matthew N. [:MattN]]

Similar to about:compat, the Lockbox desktop extension would like to register about:logins and not have the moz-extension: URI showing in the address bar.

about:compat is supposed to already being achieving this but it ends up showing the moz-extension: probably due to wanting to be in a different process than the initial about: load (thanks to Gijs/kmag for pointing this out):

E10SUtils.getRemoteTypeForURI("about:compat", gMultiProcessBrowser, E10SUtils.DEFAULT_REMOTE_TYPE)
"web"

E10SUtils.getRemoteTypeForURI("moz-extension://84998523-505c-a347-a547-0c85b377a03f/aboutCompat.html", gMultiProcessBrowser, E10SUtils.DEFAULT_REMOTE_TYPE)
"extension"

Comment 1

[Matthew N. [:MattN]]

If I change code to ensure that the processType isn't the issue then I still get a redirect and I'm thinking it comes from https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/docshell/base/nsAboutRedirector.cpp#181-187,192-194 but need to confirm.

Comment 2

[Matthew N. [:MattN]]

I think there are four things that are wanted:
a) Loading about:logins should render the moz-extension: page
b) about:logins should remain as the visible URI in the address bar
c) The principal of the page should be one that can reference/load other moz-extension: resources (stylesheets/JS)
d) The page should have access to the web extension APIs.

I'm not sure if I was clear about those four wants when I was discussing this on IRC so maybe I'm trying to do something which isn't possible.

If I hack getRemoteTypeForURIObject to always return EXTENSION_REMOTE_TYPE for "about:logins" then I still haven't found how to achieve all four wants.

When aLoadInfo.resultPrincipalURI is set to the moz-extension: URI we only get (a), (c) and (d). This is how we get a location change notification to display the moz-extension: URI in the address bar:

OnNewURI returns true so therefore OnLoadingSite returns true therefore onLocationChangeNeeded is true so FireOnLocationChange is called with mCurrentURI (which is set to the moz-extension: URI inside of OnNewURI retrieved from NS_GetFinalChannelURI of OnLoadingSite).

If I don't set aLoadInfo.resultPrincipalURI then we get (a) and (b) IIRC.

(I'm testing with about:compat since it's in a similar situation and is already in-tree. I tried changing URI_SAFE_FOR_UNTRUSTED_CONTENT but still couldn't achieve (a) through (d)).

Is it possible to have (a) through (d) or do I have to make a tradeoff to give up some (without modifying the URLbar code or using an <iframe> on about:logins)?

Comment 3

[Kris Maglione [:kmag]]

So, there are basically two ways to do this:

1. Set the channel owner to the correct result principal:

diff --git browser/extensions/webcompat/AboutCompat.jsm browser/extensions/webcompat/AboutCompat.jsm
--- browser/extensions/webcompat/AboutCompat.jsm                                                    
+++ browser/extensions/webcompat/AboutCompat.jsm
@@ -18,18 +18,15 @@ AboutCompat.prototype = {   
   QueryInterface: ChromeUtils.generateQI([Ci.nsIAboutModule]),
   getURIFlags() {
     return Ci.nsIAboutModule.ALLOW_SCRIPT |
            Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD;
   },
     
   newChannel(aURI, aLoadInfo) {
     const uri = Services.io.newURI(this.chromeURL);
-    aLoadInfo.resultPrincipalURI = uri;            
     const channel = Services.io.newChannelFromURIWithLoadInfo(uri, aLoadInfo);
     channel.originalURI = aURI;
                                
-    if (this.uriFlags & Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT) {
-      channel.owner = null;                                                
-    }                      
+    channel.owner = Services.scriptSecurityManager.createCodebasePrincipal(uri, aLoadInfo.originAttributes);
     return channel;
   },
 };  

This is what we used to do before we had LoadInfo. It's the simplest approach, but it has the drawback of leaving the documentURI set to "about:compat" (in this case), which means that relative URI loads won't work.

2. Fix DocShell's handling of these channels to use the original channel URI rather than the result principal URI:

diff --git browser/extensions/webcompat/AboutCompat.jsm browser/extensions/webcompat/AboutCompat.jsm
--- browser/extensions/webcompat/AboutCompat.jsm
+++ browser/extensions/webcompat/AboutCompat.jsm
@@ -21,15 +21,11 @@ AboutCompat.prototype = {
            Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD;
   },
 
   newChannel(aURI, aLoadInfo) {
     const uri = Services.io.newURI(this.chromeURL);
     aLoadInfo.resultPrincipalURI = uri;
     const channel = Services.io.newChannelFromURIWithLoadInfo(uri, aLoadInfo);
     channel.originalURI = aURI;
-
-    if (this.uriFlags & Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT) {
-      channel.owner = null;
-    }
     return channel;
   },
 };
diff --git docshell/base/nsDocShell.cpp docshell/base/nsDocShell.cpp
--- docshell/base/nsDocShell.cpp
+++ docshell/base/nsDocShell.cpp
@@ -10810,17 +10810,17 @@ bool nsDocShell::OnNewURI(nsIURI* aURI, 
 
 bool nsDocShell::OnLoadingSite(nsIChannel* aChannel, bool aFireOnLocationChange,
                                bool aAddToGlobalHistory) {
   nsCOMPtr<nsIURI> uri;
   // If this a redirect, use the final url (uri)
   // else use the original url
   //
   // Note that this should match what documents do (see Document::Reset).
-  NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
+  aChannel->GetOriginalURI(getter_AddRefs(uri));
   NS_ENSURE_TRUE(uri, false);
 
   // Pass false for aCloneSHChildren, since we're loading a new page here.
   return OnNewURI(uri, aChannel, nullptr, nullptr, mLoadType, nullptr,
                   aFireOnLocationChange, aAddToGlobalHistory, false);
 }
 
 void nsDocShell::SetReferrerInfo(nsIReferrerInfo* aReferrerInfo) {

This is the behavior that we used to have before bug 1319111, and is similar to the way we handle pages like about:neterror. The window.location and onLocationChange URIs for these loads will be "about:compat", but document.documentURI will be "moz-extension://...".

Unfortunately, I don't think the DocShell change will be quite as simple as that, and we'll need to talk to Boris about our options if we decide to go this route. It does seem like the preferred option, though.

Comment 4

[Kris Maglione [:kmag]]

As a side note, we probably want to change E10sUtils to allow about modules to either specify their preferred process type, or provide the URL they'll redirect to so that we can lookup the preferred process type based on that URL. I really don't want us to have to hard code the handling for every about URI we need special process handling for.

Comment 5

[Matthew N. [:MattN]]

Thank you very much for your suggestions kmag! I wouldn't have figured out the info about bug 1319111 otherwise.

We may want to go with option 1 for 67 since it's less risky (relative URLs can be fixed via <base>) and can be done within the embedded extension so there would be no uplift needed except for the process selection… How bad is it to have about:logins loaded in the web content process instead of the extension process (if we don't uplift a fix for E10SUtils)?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #5)

We may want to go with option 1 for 67 since it's less risky (relative URLs can be fixed via <base>)

You can, but you'd need to create the <base> element dynamically, since the extension base URL isn't predictable. That's certainly doable, though.

and can be done within the embedded extension so there would be no uplift needed except for the process selection… How bad is it to have about:logins loaded in the web content process instead of the extension process (if we don't uplift a fix for E10SUtils)?

Well, it's certainly not ideal. The main problem is that the page will only get content script APIs if it's loaded in a web content process. And if you have to allow for that, it means that any compromised web content process will be able to get access to any data about:logins has access to (unless until Fission ships).

Comment 7

[Matthew N. [:MattN]]

(In reply to Kris Maglione [:kmag] from comment #6)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #5)

We may want to go with option 1 for 67 since it's less risky (relative URLs can be fixed via <base>)

You can, but you'd need to create the <base> element dynamically, since the extension base URL isn't predictable. That's certainly doable, though.

Right, I have a solution for that already.

and can be done within the embedded extension so there would be no uplift needed except for the process selection… How bad is it to have about:logins loaded in the web content process instead of the extension process (if we don't uplift a fix for E10SUtils)?

Well, it's certainly not ideal. The main problem is that the page will only get content script APIs if it's loaded in a web content process. And if you have to allow for that, it means that any compromised web content process will be able to get access to any data about:logins has access to (unless until Fission ships).

Right, that is worse than being in the same process as other installed extensions.

(In reply to Kris Maglione [:kmag] from comment #4)

As a side note, we probably want to change E10sUtils to allow about modules to either specify their preferred process type, or provide the URL they'll redirect to so that we can lookup the preferred process type based on that URL. I really don't want us to have to hard code the handling for every about URI we need special process handling for.

I added an about URI flag for this which I think will be good for uplift to 67. We can look into specifying the URL for 68+.

Comment 8

[Matthew N. [:MattN]]

Attachment: Bug 1533925 - Allow about: pages to be forced into the extension process. r=kmag

Some extensions want to implement about: pages and we want those pages to be loaded in the extension process, not in the web content process, so that:

1. a compromised web process won't get access to the about: page content
2. the extension page can use all the APIs that extension pages normally get, instead of only content script APIs.

Post-Fission we will need to know which extension process to choose.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33097537a6ed
Allow about: pages to be forced into the extension process. r=kmag

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/33097537a6ed

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 9053764 [details]
Bug 1533925 - Allow about: pages to be forced into the extension process. r=kmag

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: N/A
• User impact if declined: about: pages registered by extensions (e.g. about:compat & about:logins) will be rendered in web content processes and therefore only have access to content script APIs, not the other APIs available to extension pages. This means that a compromised web content process could get access to extension pages which would be really bad for about:logins which will be provided by Lockbox in 67.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None, about:logins will be registered in the extension
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial four line patch that is dead code unless used by a Mozilla extension (like Lockbox).
• String changes made/needed: None

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9053764 [details]
Bug 1533925 - Allow about: pages to be forced into the extension process. r=kmag

P1, small footprint patch needed for Lockbox integration with 67 so no impact on the release itself, uplift approved for 67 beta 7, thanks.

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d7c011c3dd7f

Comment 14

[Daniel Bodea [:danibodea]]

Is there a way I could verify the changes made in this bug? Should I verify them (especially considering the QE-verify- tag)?
Thanks.

Comment 15

[Matthew N. [:MattN]]

No, you don't need to verify bugs with qe-verify-