1425197 - browser.devtools.inspectedWindow.eval isn't blocked on addons.mozilla.org

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, defect, P3)

Description

[Abdulrahman Alqabandi]

Attachment: devtools-panels-poc.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

1. Unpack attached PoC addon
2. Go to about:debugging and temporarily load the unpacked addon
3. Go to 'addons.mozilla.org'
4. Open web inspector


Actual results:

We execute arbitrary JS on addons.mozilla.org


Expected results:

Since tabs.executeScript()  doesn't work on the addons page, I think its reasonable to do the same for devtools.inspectedWindow()

Comment 1

[Abdulrahman Alqabandi]

Another way to eval JS:
browser.devtools.panels.elements.createSidebarPane("My pane").then(function onCreated(sidebarPane) {sidebarPane.setExpression('alert(1)', 'title')});

Though, I'm pretty sure it's using devtools.inspectedWindow.eval, but in case it's not then keep it in mind when verifying fix.

Comment 2

[:Gijs (he/him)]

Another one for Luca. :-)

Comment 3

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

Right now we explicitly deny inspectedWindow.eval from working on a system principal target:

- https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/devtools/server/actors/webextension-inspected-window.js#425-439

The sidebar setExpression API method uses the same Remote Debugging Actor to eval the expression in the target window.

We can definitely prevent it from working on a website that has access to mozAddonManager, I'm adding a needinfo assigned to Paul first, to check what is his feeling about this from a security point of view and if we should also take other websites into account as well.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

This is somewhat a known issue - see the discussion bug 1415644. Ideally, extensions should NOT be able to evaluate script in the addons.mozilla.org domain, but our current security model is inconsistent. The aim of bug 1415644 is to fix the inconsistencies and provide a unified approach to restricting access to privileged domains. 

There is a WIP patch in 1415644 to create a blacklist of origins for which to block access. Ideally we would use the same whitelist for this bug, though not sure if this is possible (approach in 1415644 is to create a new function in WebExtensionPolicy::IsRestrictedSite, i assume devtools could call this).  But either way, I think the bloacklist should be stored as a preference, and then all places in the codebase needing to enforce this, use the same pref. (something like the webchannel.allowObject.urlWhitelist pref). 

So from a pure securiry model perspective, I'd rather just block this - they ability to modify devtools on semi-privileged Mozilla sites doesn't seem like a valuable enough use case to warrant the risk here. 

That said, there are some mitigations here: IIUC devtools APIs require the "devtools" permissions (I assume the user gets warned about this at install time?). Also from testing I think that devtools pages are ONLY loaded if the user opens devtools. Thats not a super high barrier (page could coerce user into pressing keyboard shortcut) but it at least some mitigation.

Ultimately  I think my view here is that the risk isn't worth the inconsistency in the security model, and we should just prvent access to use inspectedWindow.eval and sidebarPane.setExpression in semi-privileged origins.

Comment 5

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

(In reply to Paul Theriault [:pauljt] from comment #4)
> This is somewhat a known issue - see the discussion bug 1415644. Ideally,
> extensions should NOT be able to evaluate script in the addons.mozilla.org
> domain, but our current security model is inconsistent. The aim of bug
> 1415644 is to fix the inconsistencies and provide a unified approach to
> restricting access to privileged domains.

Thanks Paul, that was exactly the bugzilla issue that I was thinking of when I was mentioning "other websites to take into account".

> 
> There is a WIP patch in 1415644 to create a blacklist of origins for which
> to block access. Ideally we would use the same whitelist for this bug,
> though not sure if this is possible (approach in 1415644 is to create a new
> function in WebExtensionPolicy::IsRestrictedSite, i assume devtools could
> call this).  But either way, I think the bloacklist should be stored as a
> preference, and then all places in the codebase needing to enforce this, use
> the same pref. (something like the webchannel.allowObject.urlWhitelist
> pref). 

I agree, I would prefer to use an approach that ensures that the list of the
blacklisted origins is kept in sync over the time and (the security model consistent
with other webextensions API which could present similar issues).

Using a new WebExtensionPolicy::IsRestrictedSite helper function from the
devtools actor seems the right way to do it and it should be doable if the
function is also exposed to privileged JS code (in the current version of that patch
it doesn't seem to be exposed to the privileged JS code, but it is something that 
we can still do before finalizing that patch).

> So from a pure securiry model perspective, I'd rather just block this - they
> ability to modify devtools on semi-privileged Mozilla sites doesn't seem
> like a valuable enough use case to warrant the risk here. 
> 
> That said, there are some mitigations here: IIUC devtools APIs require the
> "devtools" permissions (I assume the user gets warned about this at install
> time?). 

The "devtools" permissions is an implicit permission granted to the extensions that
have a "devtools_page" in their manifest (I mean that there is no "devtools" permission
in the "permissions" array of the extension), but the users get warned about it
at install time like for any other explicit permission (from the user point of view 
it is listed and described in the install doorhanger as any other extension permission, 
https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/locales/en-US/chrome/browser/browser.properties#103)  

> Also from testing I think that devtools pages are ONLY loaded if the
> user opens devtools. Thats not a super high barrier (page could coerce user
> into pressing keyboard shortcut) but it at least some mitigation.

Yes, no devtools API is available until the developer tools are opened on the tab.

I think it is also worth to mention that the devtools.inspectedWindow.eval method 
can be used even without a devtools panel or a devtools sidebar:

it can be also used from the invisible "devtools_page" that is created when the user open a developer toolbox on
a tab (e.g. some extension uses inspectedWindow.eval to check if the webpage has loaded a page which uses 
the JS framework that the extension supports before creating the actual extension devtools panel).

> 
> Ultimately  I think my view here is that the risk isn't worth the
> inconsistency in the security model, and we should just prvent access to use
> inspectedWindow.eval and sidebarPane.setExpression in semi-privileged
> origins.

Sounds right to me too.

Comment 6

[Abdulrahman Alqabandi]

Would like to note that this also works on other extensions pages (though not 100% sure if this is normal) as well as about:home (which contains the Mozilla.UItour capabilities)

Comment 7

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

(In reply to Abdulrahman Alqabandi from comment #6)
> Would like to note that this also works on other extensions pages (though
> not 100% sure if this is normal) as well as about:home (which contains the
> Mozilla.UItour capabilities)

Good point, we should definitely block it on a moz-extension:// url that is not related to the same extension.

On the about: urls, Currently the implementation is going to block only the ones with a system principal, but I'm thinking that it would be better to allow it only on about:blank instead and block it on every other about: url.

Comment 8

[:Gijs (he/him)]

(In reply to Luca Greco [:rpl] from comment #7)
> (In reply to Abdulrahman Alqabandi from comment #6)
> > Would like to note that this also works on other extensions pages (though
> > not 100% sure if this is normal) as well as about:home (which contains the
> > Mozilla.UItour capabilities)
> 
> Good point, we should definitely block it on a moz-extension:// url that is
> not related to the same extension.
> 
> On the about: urls, Currently the implementation is going to block only the
> ones with a system principal, but I'm thinking that it would be better to
> allow it only on about:blank instead and block it on every other about: url.

You may also want to make an exception for about:srcdoc (used for srcdoc iframes, though depending on how you're checking things you may just see the principal of the containing page instead).

Comment 9

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

Attachment: 1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor_.patch

In the attached patch the changes needed to deny devtools.inspectedWindow.eval API calls (and as well as the devtools sidebar.setExpression or devtools.inspectedWindow.reload) on the following additional targets:

- origins listed as restricted in the about:config preference
- moz-extensions urls not related to the same extension that has called the related devtools API methods
- about pages that are not about:blank or about:srcdoc

as well as any system principal target (which was already enforced in the current version of the RDP actor).

I'm adding Kris to review the changes to the dom/chrome-webidl/WebExtensionPolicy.webidl and toolkit/components/extensions/WebExtensionPolicy.h
(these changes are needed to expose the isRestrictedURI method to the privileged js code, because the existent canAccessURI also checks the "origin" permissions of the extension, which wasn't supposed to be part of the behavior of the devtools API methods and may introduce more regressions for the extensions that don't also include the "origin" permissions for their targets)

and Alex for the changes to the RDP actor (which is where we need to check if the current target is allowed and deny it if it is not) and to some tests that would fail with the additional behavior implemented by this patch (the new test cases related are not included in this patch, but I have a separate patch for them).

After creating the patch, I gave it a try on the attached "proof of concept" extension and on some real world extension (e.g. the React Devtools extension), and I decided to apply the further tweaks to ensure that we also log a warning message for the user when the devtools.inspectedWindow.eval requests have been denied because of these new checks (only once if the API method is called more than once on the same target's document, but ensuring that we log it again if the user reloads the tab).

The reason is that it is very likely that the extension is calling the devtools.inspectedWindow.eval API method from the devtools_page (which is invisible to the user) first and when it doesn't receive a valid result, it will not add its extension devtools panel and no message shown to the user to let him know why (and the extension may keep trying to call the eval API method, e.g. as the React DevTools extension actually does).

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8960981 [details] [diff] [review]
1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor_.patch

Review of attachment 8960981 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
@@ +31,5 @@
> +  });
> +
> +  await extension.startup();
> +
> +  const extensionUUID = await extension.awaitMessage("test-extension-uuid");

let url = WebExtensionPolicy.getByID(extension.id).getURL("fake-caller-script.js");

::: devtools/server/actors/webextension-inspected-window.js
@@ +33,5 @@
> +const deniedWarningDocuments = new WeakSet();
> +
> +function isSystemPrincipal(window) {
> +  const principal = window.document.nodePrincipal;
> +  return Services.scriptSecurityManager.isSystemPrincipal(principal);

return window.document.nodePrincipal.isSystemPrincipal;

@@ +519,5 @@
> +        });
> +      }
> +
> +      let docPrincipalURI = window.document.nodePrincipal.URI;
> +      let aboutURIWhitelist = /^about:(blank|srcdoc)/i;

These should *never* be in principal URIs. If you ever find principals with about: URLs, please file a security bug.

::: devtools/server/tests/browser/browser_webextension_inspected_window.js
@@ +19,4 @@
>  
> +  await extension.startup();
> +
> +  const extensionUUID = await extension.awaitMessage("test-extension-uuid");

let url = WebExtensionPolicy.getByID(extension.id).getURL("fake-caller-script.js");

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8960981 [details] [diff] [review]
1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor_.patch

Review of attachment 8960981 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Luca, I mostly have comments about your comments ;)

::: devtools/server/actors/webextension-inspected-window.js
@@ +23,5 @@
>  
> +const {WebExtensionPolicy} = Cu.getGlobalForObject(XPCOMUtils);
> +
> +// A weak set of the documents for which a warning message has been
> +// already logged (so that we don't keep to emitting the same warning if an

s/keep to emitting/keep emitting/

@@ +43,5 @@
> +  return {
> +    exceptionInfo: {
> +      isError: true,
> +      code: "E_PROTOCOLERROR",
> +      description: "Unknown Inspector protocol error",

nit: note that you always pass a `description` attribute as argument

@@ +65,5 @@
> +
> +  const {name} = extensionPolicy;
> +
> +  // Use the window.location if it is a system principal, otherwise use the
> +  // document principal URI to report the denied target url.

This is the typical comment that only rephrases the code. (i.e. I tend to find them useless, especially for simple one line code)
Instead of this comment, it would be interesting to mention that system principal has undefined URI and so we use the window location instead.

@@ +71,5 @@
> +    Services.io.newURI(window.location.href) : window.document.nodePrincipal.URI;
> +
> +  const error = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
> +
> +  const msg = `This extension "${name}" is not allowed to access ${reportedURI.spec}`;

s/This/The/

@@ +83,5 @@
> +
> +  let callerURI = callerInfo.url && Services.io.newURI(callerInfo.url);
> +
> +  // If the callerInfo.url doesn't have a file path, the error will be associated the
> +  // url of the extension manifest.json file.

s/associated the url of the extension/associated to the extension/

Also it isn't clear what "callerInfo.url doesn't have a file path" means. You are again commenting the code rather than explaining how we end up with such state. Do we pass resource://addonId/ url when interpreting the manifest?

@@ +84,5 @@
> +  let callerURI = callerInfo.url && Services.io.newURI(callerInfo.url);
> +
> +  // If the callerInfo.url doesn't have a file path, the error will be associated the
> +  // url of the extension manifest.json file.
> +  if (callerURI.filePath === "/") {

Can `callerInfo.url` be undefined/null?
line 84 tends to suggests it can, but this line assumes it can't.
So either tweak this line or 84 accordingly.

@@ +523,5 @@
> +      let aboutURIWhitelist = /^about:(blank|srcdoc)/i;
> +
> +      if (WebExtensionPolicy.isRestrictedURI(docPrincipalURI) || (
> +          docPrincipalURI.schemeIs("about") &&
> +          !aboutURIWhitelist.test(docPrincipalURI.spec))) {

Per Kriss comment, does that mean this check is useless or we don't properly whitelist about documents? What will be nodePrincipal.URI for about:blank and about:srcdoc documents? Will we still allow eval calls against them? If not, we propbably miss a unit test.

@@ +541,5 @@
> +      const windowAddonId = window.document.nodePrincipal.addonId;
> +
> +      if (windowAddonId && extensionPolicy.id !== windowAddonId) {
> +        // Log the error for the user to know that the extension request has been denied
> +        // (the extension may not warn the user at all).

You are repeating this comment 3 times. The function name is quite explicit. The first comment on top of logEvalDenied should be enough.

Comment 12

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

Attachment: 1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor_.patch

Patch updated based on last round of review comments (Comment 10 and Comment 11).

- minor tweaks (e.g. use `WebExtensionPolicy.getByID(extension.id).getURL("fake-caller-script.js");` to retrieve a valid extension url related to an installed extension, use document.nodePrincipal.isSystemPrincipal to identify system principals documents, also fix and rephrase some comments and remove
some unnecessary inline comment)

- fixes (e.g. fixed test for disallowed about pages, to just deny any document which has a principal with an "about:" scheme, and added an explicit test to verify that eval is allowed on about:srcdoc as expected)

Comment 13

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

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> nit: note that you always pass a `description` attribute as argument

The one included in the helper method is "Unknown Inspector protocol error" and it is there just in case
the caller doesn't override it with a more specific one. 
(I really don't like the format of these errors, unfortunately they are following the format of the parameter as 
received on Chrome from the same extension APIs, in a follow up I would like to just reformat the 
error on the RDP client side, in the WebExtensions API implementation for these methods, and then change 
this RDP actor to send a simpler error info format). 
 

> @@ +83,5 @@
> > +
> > +  let callerURI = callerInfo.url && Services.io.newURI(callerInfo.url);
> > +
> > +  // If the callerInfo.url doesn't have a file path, the error will be associated the
> > +  // url of the extension manifest.json file.
> 
> s/associated the url of the extension/associated to the extension/
> 
> Also it isn't clear what "callerInfo.url doesn't have a file path" means.
> You are again commenting the code rather than explaining how we end up with
> such state. 

It's a limitation of the current implementation, I've filed Bug 1448878 to explain what is missing
to make the callerInfo.url the actual fileName that has originated the API call (and also get a lineNumber
from where the API call is) and added a reference to that bug here (and where the callerInfo is being
created, there was already a TODO there but now it also include the bugzilla issue number).

I've also tweaked the comment a bit to make it a bit clearer that the only reason why the callerInfo.url is currently
the baseURI of the extension is a limitation of the current implementation.

> Do we pass resource://addonId/ url when interpreting the
> manifest?

I'm not sure that I get this question.

 
> @@ +84,5 @@
> > +  let callerURI = callerInfo.url && Services.io.newURI(callerInfo.url);
> > +
> > +  // If the callerInfo.url doesn't have a file path, the error will be associated the
> > +  // url of the extension manifest.json file.
> > +  if (callerURI.filePath === "/") {
> 
> Can `callerInfo.url` be undefined/null?
> line 84 tends to suggests it can, but this line assumes it can't.
> So either tweak this line or 84 accordingly.

It can't be undefined/null, it has to be a string as for the related protocol.js spec:

https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/devtools/shared/specs/webextension-inspected-window.js#25

Otherwise, the validation based on the spec will reject the RDP request as invalid.

> 
> @@ +523,5 @@
> > +      let aboutURIWhitelist = /^about:(blank|srcdoc)/i;
> > +
> > +      if (WebExtensionPolicy.isRestrictedURI(docPrincipalURI) || (
> > +          docPrincipalURI.schemeIs("about") &&
> > +          !aboutURIWhitelist.test(docPrincipalURI.spec))) {
> 
> Per Kriss comment, does that mean this check is useless or we don't properly
> whitelist about documents? What will be nodePrincipal.URI for about:blank
> and about:srcdoc documents? Will we still allow eval calls against them? If
> not, we propbably miss a unit test.

I added to the existent "inspectedWindow.reload with injectedScript" test one additional sub-frame
with an about:srcdoc document.
(while in a separate patch I've one additional test that checks some of the about urls where we expect
inspectedWindow.eval to be denied, e.g. about:home and about:newtab, as well as the ones from the
"extensions.webextensions.restrictedDomains" about:config preference).

Comment 14

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

Attachment: 1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor__r_kmag_ochameau.patch

Added reviewers in the commit message and set the r+ flag from last round of review comments.

Comment 15

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

Comment on attachment 8962388 [details] [diff] [review]
1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor__r_kmag_ochameau.patch

[Security approval request comment]

## How easily could an exploit be constructed based on the patch?

As it can also we verified by looking at the attached proof of concepts, taking advantage of the additional checks 
that are missing in the previous version is just a matter of including in an extension a "devtools_page" 
(invisible to the user) which uses the "browser.devtools.inspectedWindow.eval" method (or one of the other two
API methods that uses the same underlying Remote Debugging method, "browser.devtools.inspectedWindow.reload" and 
"browser.devtools.panels.elements.createSidebarPane + sidebar.setExpression"),
and then wait the user to open the toolbox on a target that may be of any value for the attacker.

It may be worth to mention that:

- the system principal targets were already blocked even before this patch

- the extension can't directly open the toolbox on its own, the user has to open it manually (but the extension may still try to trick the user into opening it)

The content of the patch itself shouldn't make it any simpler, but it may show some additional targets that may be 
interesting from an attacker point of view (e.g. one of the url listed in the related about:config preference,
or a tab which has loaded the extension page of a different extension)

## Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I kept out from the check-in comment words like "restricted", "sensible", "allow" or "deny", hoping to make it to 
sound a bit less like a security problem (but still be meaningful enough to us if we look at the commit message in 
the future), the tests include in the patch are currently only the ones that had to be fixed to prevent them from
failing when the patch lands, and they shouldn't be of any particular help for an attacker.

The inline comments are a bit more explicit, where for "explicit" I mean that they actually use words like "restricted", "denied" "allowed".

I'm not sure if these inline comments "paint a big bulls-eye" on their own, but I'm open to move them into a separate
patch (as the additional test case not yet included in this patch), if we think that it could be better.

## Which older supported branches are affected by this flaw?

The first version to support "devtools.inspectedWindow.eval" is Firefox 54
(same for for "devtools.inspectedWindow.reload", while the "sidebar.setExpression" API method has been landed in Firefox 57), and so it doesn't seem that this flaw has ever reached ESR yet (it will once Firefox ESR will be 60).

## If not all supported branches, which bug introduced the flaw?

Bug 1315251 has introduced the new WebExtensionInspectedWindowActor (landed in Firefox 53), but it has been exposed to the extensions only once we landed also Bug 1300584 (landed in Firefox 54)

## Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No, but it should not be hard to backport the fix to the versions where Bug 1415644 (which this patch depends from) have been landed.

Bring this change to Firefox 60 seems to be reasonable (especially because it would be the next ESR), and Bug 1415644 is being landed on Firefox 60 (and so it is also pretty doable).

Given the security level of this bug, backporting to 59 doesn't seem worth the risk, especially because of Bug 1415644, which has not been ported there.

## How likely is this patch to cause regressions; how much testing does it need?

Not very likely, most of the users uses the devtools extensions to inspect regular webpages (for which the behavior should be unchanged), it may affect some users if they were using these devtools extensions on some of the restricted urls (which seems more like an internal use case, where a developer is inspecting the production version of one of those webapps that are not going to be allowed by devtools.inspectedWindow.eval anymore).

The underlying remote debugging method (which is where the fix is actually applied) has a good number of tested scenarios, and there are more automated tests in the WebExtensions test suite for the valid use cases.

Comment 16

[Kris Maglione [:kmag]]

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> @@ +523,5 @@
> > +      let aboutURIWhitelist = /^about:(blank|srcdoc)/i;
> > +
> > +      if (WebExtensionPolicy.isRestrictedURI(docPrincipalURI) || (
> > +          docPrincipalURI.schemeIs("about") &&
> > +          !aboutURIWhitelist.test(docPrincipalURI.spec))) {
> 
> Per Kriss comment, does that mean this check is useless or we don't properly
> whitelist about documents? What will be nodePrincipal.URI for about:blank
> and about:srcdoc documents? Will we still allow eval calls against them? If
> not, we propbably miss a unit test.

For about:srcdoc and about:blank, the principal is either inherited from the caller that created the document, or is the null principal. For other about: pages, the principal is pretty arbitrary. It's whatever principal the given about: handler assigns to it. Sometimes the system principal, sometimes a null principal, sometimes another codebase principal. (In the case of about:newtab, it's apparently a codebase principal with URI about:newtab, which is unfortunate, but *shrug* not applicable here)

Comment 17

[Al Billings [:abillings - ex-MoCo]]

As a sec-moderate this doesn't need sec-approval to go in but I agree that we should backport it to 60 to get it into the ESR60 line.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82b0578b5b133e16fd0aa3cabf5f9e14d099d373

Comment 19

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/82b0578b5b133e16fd0aa3cabf5f9e14d099d373

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 21

[Julien Cristau [:jcristau]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Please request Beta approval on this when you're comfortable doing so.

re-adding needinfo, since it was cleared without an uplift request or comment.

Comment 22

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

Comment on attachment 8962388 [details] [diff] [review]
1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor__r_kmag_ochameau.patch

Approval Request Comment

[Feature/Bug causing the regression]:

Bug 1315251, which has introduced the WebExtensionInspectedWindowActor (landed in Firefox 53), and  Bug 1300584, which has exposed it to the extensions through the devtools API (landed in Firefox 54).

[User impact if declined]:
sec-moderate vulnerability on WebExtensions.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Not manually, but with automated tests.

[Needs manual test from QE? If yes, steps to reproduce]:
It should not be needed.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
It should not be risky.

[Why is the change risky/not risky?]:
Most of the change is applied on the "WebExtensionInspectedWindowActor" remote debugging actor, which is only used by the WebExtensions devtools APIs (and so only WebExtensions which are using the devtools APIs may be affected by it), and to its automated tests.
The riskier part of this change is likely to be the changes applied to WebExtensionPolicy.webidl and WebExtensionPolicy.h to expose `IsRestrictedURI` to privileged js callers, which are pretty small changes and they shouldn't be able to introduce any new regression related to the WebExtensionPolicy. 

[String changes made/needed]:
None.

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8962388 [details] [diff] [review]
1-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor__r_kmag_ochameau.patch

webextensions sec fix, approved for 60.0b10

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/612719d703997f07001e7da9ca5bfcf228a4c280

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing devtools' browser_webextension_inspected_window.js and browser_inspector_extension_sidebar.js (only on asan+debug and DevEdition+Nightly?):

https://hg.mozilla.org/releases/mozilla-beta/rev/23564d2a51721b751cf1bc3ff28a895821dff35f

Log 1: https://treeherder.mozilla.org/logviewer.html#?job_id=171877804&repo=mozilla-beta

09:11:04     INFO - *** Start BrowserChrome Test Results ***
09:11:04     INFO - checking window state
09:11:04     INFO - TEST-START | devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
09:11:05     INFO - GECKO(771) | Unable to read VR Path Registry from /Users/cltbld/Library/Application Support/OpenVR/.openvr/openvrpaths.vrpath
09:11:05     INFO - TEST-INFO | started process screencapture
09:11:05     INFO - TEST-INFO | screencapture: exit 0
09:11:05     INFO - Buffered messages logged at 09:11:04
09:11:05     INFO - Entering test bound setupExtensionSidebar
09:11:05     INFO - Extension loaded
09:11:05     INFO - Buffered messages finished
09:11:05     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js:36 - ReferenceError: WebExtensionPolicy is not defined
09:11:05     INFO - Stack trace:
09:11:05     INFO -     setupExtensionSidebar@chrome://mochitests/content/browser/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js:36:5
09:11:05     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
09:11:05     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:958:9
09:11:05     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
09:11:05     INFO - Leaving test bound setupExtensionSidebar


Log 2: https://treeherder.mozilla.org/logviewer.html#?job_id=171874823&repo=mozilla-beta

09:04:01     INFO - TEST-START | devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
09:04:01     INFO - GECKO(778) | Unable to read VR Path Registry from /Users/cltbld/Library/Application Support/OpenVR/.openvr/openvrpaths.vrpath
09:04:01     INFO - GECKO(778) | 2018-04-04 09:04:01.382 plugin-container[780:5123] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0x8b5f, name = 'com.apple.tsm.portname'
09:04:01     INFO - GECKO(778) | See /usr/include/servers/bootstrap_defs.h for the error codes.
09:04:01     INFO - TEST-INFO | started process screencapture
09:04:01     INFO - TEST-INFO | screencapture: exit 0
09:04:01     INFO - Buffered messages logged at 09:04:01
09:04:01     INFO - Entering test bound setupExtensionSidebar
09:04:01     INFO - Extension loaded
09:04:01     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
09:04:01     INFO - Buffered messages finished
09:04:01     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js:36 - ReferenceError: WebExtensionPolicy is not defined
09:04:01     INFO - Stack trace:
09:04:01     INFO -     setupExtensionSidebar@chrome://mochitests/content/browser/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js:36:5
09:04:01     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
09:04:01     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:958:9
09:04:01     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

Comment 26

[Andrei Vaida [:avaida]]

(In reply to Luca Greco [:rpl] from comment #22)
> [Is this code covered by automated tests?]:
> Yes.
> 
> [Has the fix been verified in Nightly?]:
> Not manually, but with automated tests.
> 
> [Needs manual test from QE? If yes, steps to reproduce]:
> It should not be needed.

Setting qe-verify- based on Luca's assessment on manual testing needs and the fact that this fix has automated tests.

Comment 27

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

I took a look at the above failures and it seems that there were actually two tests that were failing on the beta uplift of the patch like we landed it on mozilla-central:

- devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
- devtools/server/tests/browser/browser_webextension_inspected_window.js

In both the cases the issues seems to be just that WebExtensionPolicy is not already available as a global in these two test files, and so to make these tests to run successfully on beta I added the following further changes, which explicitly retrieves the WebExtensionPolicy object so that it can be used in the test:

```
diff --git a/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js b/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
--- a/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
+++ b/devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js
@@ -8,16 +8,21 @@ ChromeUtils.defineModuleGetter(this, "Co
                                "resource://testing-common/ContentTaskUtils.jsm");
 
 loader.lazyGetter(this, "WebExtensionInspectedWindowFront", () => {
   return require(
     "devtools/shared/fronts/webextension-inspected-window"
   ).WebExtensionInspectedWindowFront;
 }, true);
 
+ChromeUtils.defineModuleGetter(this, "ExtensionParent",
+                               "resource://gre/modules/ExtensionParent.jsm");
+
+const {WebExtensionPolicy} = ExtensionParent;
+
 const SIDEBAR_ID = "an-extension-sidebar";
 const SIDEBAR_TITLE = "Sidebar Title";
 
 let extension;
 let fakeExtCallerInfo;
 
 let toolbox;
 let inspector;
diff --git a/devtools/server/tests/browser/browser_webextension_inspected_window.js b/devtools/server/tests/browser/browser_webextension_inspected_window.js
--- a/devtools/server/tests/browser/browser_webextension_inspected_window.js
+++ b/devtools/server/tests/browser/browser_webextension_inspected_window.js
@@ -1,14 +1,19 @@
 /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
 /* Any copyright is dedicated to the Public Domain.
  http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
+ChromeUtils.defineModuleGetter(this, "ExtensionParent",
+                               "resource://gre/modules/ExtensionParent.jsm");
+
+const {WebExtensionPolicy} = ExtensionParent;
+
 const {
   WebExtensionInspectedWindowFront
 } = require("devtools/shared/fronts/webextension-inspected-window");
 
 const TEST_RELOAD_URL = `${MAIN_DOMAIN}/inspectedwindow-reload-target.sjs`;
 
 async function setup(pageUrl) {
   const extension = ExtensionTestUtils.loadExtension({
```

I'm going to attach a new patch for the beta uplift, one that contains these additional changes already applied on top of patch as landed on mozilla-central.

Comment 28

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

Attachment: beta-uplift-version-of-Bug_1425197___Add_additional_checks_of_the_target_URL_in_the_inspectedWindow_devtools_actor__r_kmag__r_ochameau.patch

Comment 29

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

Hi Julien,
attachment 8965352 [details] [diff] [review] is a version of the patch which includes the changes described in comment 27 to fix the unexpected failures that we got when the original patch has been uplifted to beta as it is (and then backed out because of these failures, sorry about that).

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a7905efa6eb152c4e0b043abf91b0b88149d0a6