1685123 - implement manifest sandbox support

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

with manifest v3 csp changes, the sandbox functionality becomes important as a way to allow extensions to continue using javascript frameworks that still use eval or other eval-like constructs. We should support sandbox for those use cases.

https://developer.chrome.com/docs/extensions/mv3/manifest/sandbox/

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1685123 part 1: sandboxed extensions pages are allowed to load their own resources

Comment 2

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f6a242f867ceb46f5ffe9d49da5a74925cec52f

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1685123 add sandbox to extension manifest and policy

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1685123 apply sandbox base csp to sandboxed pages

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1685123 part 4: apply sandbox csp from the extension manifest if defined

Comment 7

[Shane Caraveo (:mixedpuppy)]

This bug is going on hold for now, we will revisit whether we want to support sandbox sometime down the road. part 1 might be something we split out to its own bug.

Comment 8

[Phabricator Automation]

Comment on attachment 9195509 [details]
Bug 1685123 part 1: sandboxed extensions pages are allowed to load their own resources

Revision D100834 was moved to bug 1700762. Setting attachment 9195509 [details] to obsolete.

Comment 9

[BugBot [:suhaib / :marco]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D101241	Bug 1685123 apply sandbox base csp to sandboxed pages	mixedpuppy	robwu: Inactive

:mixedpuppy, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Comment 10

[Suhaib Mujahid [:suhaib]]

Sorry for the noise, there was a problem with a Bugzilla API that caused the bot to think more people were inactive than normal (bug 1789646).

Comment 12

[swleefers]

Would this sandbox allow for extensions that do browser automation, i.e. extensions that can execute Javascript received from a (trusted) native application? It is currently possible to have a native application send Javascript code for an extension to execute, which is mighty useful. It is code existing on the user's computer, sent to the Firefox extension by a native application that already has local execution privileges, so there is no real escalation of privileges by the extension's ability to do eval, as I understand it. (Eval is currently needed for that in combination with native messaging.)

Comment 13

[Rob Wu [:robwu]]

We have unintentionally allowed extensions to run code strings in extension contexts (bug 2011234) and are deprecating executeScript in moz-extension:-documents. While the file option has an alternative (simply using <script> elements in the extension document), the code option does not. We are going to implement the sandbox manifest key to allow extensions to continue string-based code execution, in unprivileged extension documents. We are planning to land this work before the executeScript-in-moz-extension:-documents functionality is finalized (bug 2015559).

Comment 14

[cyfung1031]

Just let you know that ScriptCat (https://github.com/scriptscat/scriptcat/) is waiting for the sandbox support since MV3 does not allow unsafe-eval.
In MV2, although sandbox is not supported, we can still use declare unsafe-eval to use string-based code execution.

We currently maintain both MV2 and MV3 versions - one for Blink one for Gecko.
We attach an iframe pointing to sandbox.html in Chrome MV3's Offscreen / Firefox MV3's Event Page. Inside sandbox.html, it can do string-based code execution without browser extension API, then communicate back to Chrome MV3's Service Worker / Firefox MV3's Event Page.

Comment 15

[cyfung1031]

Please be noted that, Chrome MV3 requires NOT TO ADD allow-same-origin for the use of sandbox, while Firefox MV2/MV3 requires ADD allow-same-origin to allow the use of localStorage.

https://developer.chrome.com/docs/extensions/reference/manifest/sandbox

screenshot for console error

Comment 16

[robbendebiene]

My extension is also slightly affected by the change in bug 2011234.

In short: I basically have a welcome/tutorial page that I ship with the add-on where users can try out the extension's functionality. This page technically only needs access to the browser.i18n API so I would be fine with the sandbox solution. Ideally the page has access to content script APIs but if not I can work around this by injecting a custom content script into it.

Now that bug 2015559 is marked as completed, does the following still apply?

We are going to implement the sandbox manifest key to allow extensions to continue string-based code execution, in unprivileged extension documents. We are planning to land this work before the executeScript-in-moz-extension:-documents functionality is finalized (bug 2015559).

I'm still lacking a proper solution on my end. I even started considering injecting the whole document in about:blank.

• I don't want to move the injected code into the content script world, because I mostly have little pieces of code only required on demand. It would also increase code complexity and produce clutter.
• I don't want to host the tutorial page on an external server.

Also will the sandbox manifest key also be available for MV2 extensions?

Comment 17

[Rob Wu [:robwu]]

(I wrote the following reply a while ago but see that I have not submitted it, so doing that now, while also responding to robbendebiene)

(In reply to cyfung1031 from comment #15)

Please be noted that, Chrome MV3 requires NOT TO ADD allow-same-origin for the use of sandbox, while Firefox MV2/MV3 requires ADD allow-same-origin to allow the use of localStorage.

What are you trying to say here?

The manifest sandbox feature is going to be an opaque origin for security reasons - CSP allow-same-origin together with allow-scripts would be a very ineffective sandbox. If implemented, our sandbox is not going to allow allow-same-origin either. This would rule out the ability to use localStorage and such, but that is completely by design (and documented CSP behavior when allow-same-origin is absent.

(In reply to robbendebiene from comment #16)

Now that bug 2015559 is marked as completed, does the following still apply?

We are going to implement the sandbox manifest key to allow extensions to continue string-based code execution, in unprivileged extension documents. We are planning to land this work before the executeScript-in-moz-extension:-documents functionality is finalized (bug 2015559).

In terms of timing, likely not, since the restriction is shipping in 152, currently beta, going to release in a bit over a week.
I would have liked to get this feature into 153, but the code would have to land next week, and there is not enough review capacity even if I were to author the patches now. The best chances of getting this landed in time is if I were to serve as a reviewer for the patch, but then I cannot also be the patch author.

Are you interested in contributing a patch? I can make a write-up of relevant considerations and implementation pointers if so.

I'm still lacking a proper solution on my end. I even started considering injecting the whole document in about:blank.

• I don't want to move the injected code into the content script world, because I mostly have little pieces of code only required on demand. It would also increase code complexity and produce clutter.
• I don't want to host the tutorial page on an external server.

Also will the sandbox manifest key also be available for MV2 extensions?

Yes. I see no reason for restricting it to MV3 only.

Comment 18

[robbendebiene]

(In reply to Rob Wu [:robwu] from comment #17)

Are you interested in contributing a patch? I can make a write-up of relevant considerations and implementation pointers if so.

My fear is that this task is a little more complex than what I've done so far.

I'm considering it, but I have a few questions:

• How difficult do you rate the patch?
• What is the state of the current patch (https://phabricator.services.mozilla.com/D101241)? Could I continue it?
• What is missing from the current patch?

Comment 19

[Rob Wu [:robwu]]

(In reply to robbendebiene from comment #18)

(In reply to Rob Wu [:robwu] from comment #17)

Are you interested in contributing a patch? I can make a write-up of relevant considerations and implementation pointers if so.

My fear is that this task is a little more complex than what I've done so far.

I'm considering it, but I have a few questions:

• How difficult do you rate the patch?

It is not trivial, but I think that you have the code comprehension and debugging skills to get it completed, based on what I witnessed on your previous patches. Getting the basics working would be very feasible, but there are other security-relevant considerations that are not as obvious. If anything is missing I'll flag it during review (or before - feel free to reach out earlier if you have questions).

Before getting to the implementation, it would also be valuable to come up with relevant test cases for desired behavior, when a page is part of manifest sandbox:

• The basic one is obviously: is the document sandboxed at all? This can be checked by confirming that origin is "null" instead of moz-extension://... from a script (or devtools, if checking manually).
  • Nice-to-have: test that a cross-origin fetch() from a sandboxed extension page does not leak the extension URL via the Origin header. The extension URL contains a random yet fixed UUID. I expect at least the Origin test to already pass (because origin is "null"` for sandboxed documents, that is the default web platform behavior).
• Unit test should also verify that extension APIs are unavailable (browser and chrome globals missing). This is already the case, and the unit test ensures that it continues to be like that (bug 1896824 is another bug -coincidentally reported by you- that is asking for APIs in iframes that are in a document whose parent is sandboxed; depending on the fix there an unwanted side effect could be the exposure of extension APIs; the unit test ensures that we don't).
• Something to keep in mind: There are two ways for a general web page to be sandboxed: through a HTTP header and through being embedded in another page that was already sandboxed. In extension documents we don't have a HTTP header, which is why there is a separate manifest sandbox key to control the behavior. The other case you already witnessed before in bug 1896824. If you are unsure whether a behavior is due to your implementation or existing behavior, you can try it out from a web-accessible extension document embedded in a sandboxed web page (e.g. with devtools).
• Unit test that checks whether scripting.executeScript is able to execute code in a sandboxed extension frame (basically tests at https://searchfox.org/firefox-main/source/toolkit/components/extensions/test/mochitest/test_ext_executeScript_mozextension.html, but then verifying whether you can run a content script). I expect execution to be blocked independently of the pref, because the implementation does not even consider moz-extension:-URLs for opaque origins (WebExtensionPolicy.cpp), so the logic controlling execution for moz-extension:-URLs does not even kick in.
• content_security_policy.sandbox validation (permits custom CSP and sandbox directive, but disallows allow-same-origin). Desired behavior when an invalid directive is used, what to do? (e.g. fall back to default. What does Chrome do?)
  Such a unit test would fit well at https://searchfox.org/firefox-main/rev/cf8510b7c3bb11cf4abc2f8aa431eebdcb38b41b/toolkit/components/extensions/test/xpcshell/test_ext_manifest_content_security_policy.js

• What is the state of the current patch (https://phabricator.services.mozilla.com/D101241)? Could I continue it?
• What is missing from the current patch?

I recommend starting from scratch, but use that patch to get pointers for relevant places of the implementation that you can evaluate. The implementation in the patch is more complicated than it needs to be (and many filed have been renamed, jsm->sys.mjs in the meantime, resolving merge conflicts will be more effort than worth it). You are free to copy parts (e.g. tests) if you think (or aren't sure) whether they're useful, I'll take a look during code review anyway.

Part of the complexity is because it was modeled after the existing CSP implementation for extension pages, whose complexity is caused by history that are not relevant to the sandbox CSP. In case you're interested in that history for context: the CSP in use for extension pages depends on multiple conditions (MV2/MV3, whether loaded temporarily) and preferences, and is enforced through two layers of CSP: a base CSP that cannot be relaxed under any circumstance (except theoretically through undocumented preferences), and a CSP that extensions can specify. There is also a validator in Schemas.sys.mjs to validate whether a custom CSP meets the requirements, but this is more for developer experience than security (because the base CSP enforces the bare minimum). As an example, Chrome used to rely on CSP validation only, which was insufficient (which I reported as Security: bypass of CSP validator to run remote code in extensions ). Their current implementation is now closer to Firefox's, through the use of a base CSP (minimum CSP in Chromium) plus the extension's own CSP.

In the feature here, the only requirement for CSP in sandboxed extension pages is that their origin is opaque, i.e. that the CSP effectively has a sandbox without allow-same-origin. The current patch does that by enforcing a base CSP that has a sandbox declaration, but that is stricter than necessary. Existing extension CSP validation logic is at AddonContentPolicy::ValidateAddonCSP, which checks more than strictly needed. You can try to fit the new logic there or to have a dedicated method to validate sandbox CSP instead. The nsCSPContext::GetCSPSandboxFlags method can do the parsing, after which you can check whether SANDBOXED_ORIGIN is present (Searchfox query). With the validation implemented, it needs to be hooked up to the schema validator and referenced in the schema. The existing patch shows up how to hook it up, but if you want a more recent example you can look up the patch of bug 1864284.

Note: The patch includes sandbox.content_security_policy and content_security_policy.sandbox. The former was Chrome's MV2 implementation, the latter is their MV3 implementation (which is possible because the top-level manifest key changed from a string to an object in MV3). The implementation of the MV3 implementation is the bare minimum (and sufficient for this bug). I'm not opposed to a MV2 implementation, but you don't have to spend energy on that if you don't want to.

Let me know if you're interested in giving it a try.

Comment 20

[robbendebiene]

Thanks for the details. I'm willing to give it a try.

Comment 21

[robbendebiene]

The last couple of days I've made good progress on the patch.
I have one question though:

In the feature here, the only requirement for CSP in sandboxed extension pages is that their origin is opaque, i.e. that the CSP effectively has a sandbox without allow-same-origin. The current patch does that by enforcing a base CSP that has a sandbox declaration, but that is stricter than necessary.

Does this mean the Base CSP is not necessary for this patch? If so how and where would I ensure that it is a valid sandbox CSP if I cannot rely on AddonContentPolicy::ValidateAddonCSP? My naive approach would be to do a onetime check of the CSP in the WebExtensionPolicy class and store the result for later checking. However if I do this I might as well implement the Base CSP.

Comment 22

[Rob Wu [:robwu]]

(In reply to robbendebiene from comment #21)

The last couple of days I've made good progress on the patch.
I have one question though:

In the feature here, the only requirement for CSP in sandboxed extension pages is that their origin is opaque, i.e. that the CSP effectively has a sandbox without allow-same-origin. The current patch does that by enforcing a base CSP that has a sandbox declaration, but that is stricter than necessary.

Does this mean the Base CSP is not necessary for this patch?

Yes, a base CSP is not strictly needed because the only thing we want to enforce is the lack of allow-same-origin in the CSP's sandbox directive. Unlike other CSP directives (script-src), the validation of that policy is straightforward.

If so how and where would I ensure that it is a valid sandbox CSP if I cannot rely on AddonContentPolicy::ValidateAddonCSP?

I would update that method to accept a new flag that ensures that the sandbox has the desired flag and otherwise try to repair the policy somehow such that the CSP has the right flag. I gave some pointers in my last comment starting from "Existing extension CSP validation logic".

You could also check in Document.cpp after applying the CSP that the SANDBOXED_ORIGIN flag is effective, and if not, apply another (second) CSP to make sure that it is set. Logically it would be equivalent to adding a base CSP that has sandbox set to all supported flags (now and in the future) minus the allow-same-origin flag. When multiple CSP policies are present, the most strict interpretation of the combination is applied, so the exact order is not super important.

My naive approach would be to do a onetime check of the CSP in the WebExtensionPolicy class and store the result for later checking. However if I do this I might as well implement the Base CSP.

The current postprocessor for contentSecurityPolicy in Schemas.sys.mjs returns the input as is because it is merely validating the input in order to warn, and not changing the returned CSP. You can have the same approach with a new postprocessor for the sandbox CSP, if you already enforce the CSP requirement in Document.cpp.

Comment 23

[robbendebiene]

Attachment: Bug 1685123 - Implement manifest sandbox support r=robwu

Comment 24

[robbendebiene]

Attachment: Bug 1685123 - Throw errors for “content_security_policy” manifest property with invalid types r=robwu

This change is required to allow throwing errors for the sandbox CSP implementation

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/83d64ccda069
https://hg.mozilla.org/integration/autoland/rev/565c93ec4ab3
Throw errors for “content_security_policy” manifest property with invalid types r=robwu
https://github.com/mozilla-firefox/firefox/commit/bfa5336c0905
https://hg.mozilla.org/integration/autoland/rev/acf1fc9b937f
Implement manifest sandbox support r=robwu

Comment 26

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/565c93ec4ab3
https://hg.mozilla.org/mozilla-central/rev/acf1fc9b937f

Comment 27

[Rob Wu [:robwu]]

Thanks Robin for your patches for this! This fixes a long standing request for the ability to have sandboxed pages in an extension document with their own CSP.

The latest motivation for revisiting this feature was the removal of executeScript in extension documents (bug 2011234). The feature as landed can be used by extensions to work with it, if the extension embeds a sandboxed document in an iframe, for example.

A potential thought is for sandboxed iframes to embed an extension frame and communicate with that to talk with the privileged parts of the extension. That does not work yet because of bug 1896824.

As a more direct migration aid for bug 2011234, Robin requested the ability to call executeScript for sandboxed extension documents. I discussed the request with a team member and we tentatively approved it, and a follow-up bug for that will be filed. For more context, see my approval comment and the thread at https://phabricator.services.mozilla.com/D308216#inline-1661830

Comment 28

[rbloor]

Documentation updates are available for review in:

• (BCD) Bug 1685123 implementation of WebEx manifest sandbox #30031
• (content) Bug 1685123 docs for implementation of WebEx manifest sandbox #44709