Closed Bug 1674383 Opened 4 years ago Closed 4 years ago

Permit SharedArrayBuffer in WebExtension

Categories

(WebExtensions :: Compatibility, enhancement, P2)

Product:
WebExtensions
Component:
Compatibility
Version:
79 Branch
Type:
enhancement

Tracking

(firefox90 fixed)

Status:
RESOLVED FIXED
Milestone:
90 Branch
Tracking Flags:
Tracking Status
firefox90 --- fixed

People

(Reporter: ekangmonyet, Assigned: anatal)

References

Details

Attachments

(1 file)

Allow the usage of SharedArrayBuffer by priviliged addons
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

  1. Create an extension page
  2. Use a library locally (e.g. ffmpeg.wasm) that uses SharedArrayBuffer or create one using new SharedArrayBuffer()

Actual results:

SharedArrayBuffer is undefined, corsOriginIsolated is false

Expected results:

SharedArrayBuffer works as usual.
Possibly use extension id as origin and isolate it?

Hi,
According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer, that standard feature needs to be enabled by the user in Firefox's flags for now.

Is this related maybe to this? https://bugzilla.mozilla.org/show_bug.cgi?id=1644840
Setting a component for this in order to get the dev team involved.
(If the team feels it's an incorrect one please feel free to change it to a more appropriate one.)

Best regards,
Clara

Component: Untriaged → JavaScript: Standard Library
Product: Firefox → Core

Thanks for the reply.

It works if I serve the file over HTTPS with COOP and COEP headers set correctly. But I can't do this in WebExtension.

Yeah, good point. Anne, is there a plan for how to cope with this issue?

I'm not sure what the threat model is for extensions. Hopefully Philipp and Tom can help sort this out.

Flags: needinfo?(tom)
Flags: needinfo?(philipp)

I think traditionally the attitude has been that Web Extensions grant privileges, and a malicious one could certainly do a lot of bad things to the user. Along those same lines, granting a WebExtension SAB - even though it could enable it to read memory from other WebExtensions - would be in line with previous capability grants.

That doesn't mean it might not require some sort of permission though, if one could be chosen in an understandable way....

Flags: needinfo?(tom)

Redirecting to Jorge for the product perspective. I don't have a good overview of the possible threats, though based on the MDN article it looks like this may be viable if we can separate where the SAB is used. I guess this may mean one process per extension?

Flags: needinfo?(philipp) → needinfo?(jorge)

Are there good use cases for supporting this in extensions? Does it work for Chrome extensions?

Flags: needinfo?(jorge)

Yes, there are good use cases for supporting this in extensions. We have an experiment in the works (project Bergamot, on mana) that depends on this functionality for good performance. For the time being, I think it would be OK to put SAB-in-extensions behind a pref if people are nervous / uncertain, but we may come to depend on this long-term.

We need to move this forward in order to improve Bergamot's wasm port performance, and I'd like to continue with the necessary discussion in order to make this a reality. I can take the responsibility for the technical bits but we need to reach an agreement in terms of what we should be able to do, and how. Flagging jorge for more feeback, and martin for formal official support.

Flags: needinfo?(mlopatka)
Flags: needinfo?(jorge)

Shane, can you weigh in on this? Should this be allowed behind a pref, a permission, something else?

Flags: needinfo?(jorge) → needinfo?(mixedpuppy)

We could do it with a permission like we do for geolocation. We could also consider a "secureContext" permission, which would (in theory) just grant the extension rights to any api that requires a secure context[1]. This also might be able to be bypassed in the short term if the addon has an experimental api (though I'm not certain how this is/could be used in chrome level code). We could also just consider installed extensions as a secure context without any permission, though it would be good to have input on that from Andreas, or perhaps dveditz.

I would lean towards just enable it for extensions, or use a "secureContext" permission if we really want to know what extensions might use any of these APIs.

[1] https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts/features_restricted_to_secure_contexts

Flags: needinfo?(mixedpuppy)

Another direction and/or second thought, harking back again to geolocation, is to be pedantic and require a permission for each secureContext api. It could be a silent permission, but that would force the addon to be explicit in the manifest that it will use API X.

It could be a silent permission, but that would force the addon to be explicit in the manifest that it will use API X.

Yes, that might be a good compromise. I can't see a SAB permission being useful to end users, but it could be useful for our own monitoring.

dveditz, what are your thoughts on extensions having access to APIs restricted to secure contexts, and SAB in particular?

Flags: needinfo?(dveditz)

SAB is dangerous for web content because it (without other remedies) allows:

  1. creating high-precision timers,
  2. which allows Spectre-like attacks,
  3. which allow reading in-process memory that page normally wouldn't be able to (script on evil.com reading mybank.com cookies).

Any extension with <all_urls> can already read mybank.com cookies, without jumping through those extra hoops.

IMO enabling SAB for extensions with <all_urls> and/or with a silent permission is fine.

In which process is SAP going to be used by the extensions? is it going to be used in extension pages and extension shared workers running in the child extension process?

if that is the case, my current opinion is that the same reasons described in comment 14 (related to what does make dangerous to expose SAP in web content) may also be applicable to the extensions process:

at the moment we do only allow a single extension process and in that single child process we do run the privileged extensions along with any other third party extensions (included unverified ones) and so "allowing SAP to arbitrary extensions" may be (at least potentially) dangerous in ways different from the <all_urls> permission.

From my perspective allowing SAB in the extension child process but only to privileged extensions may be reasonable and less concerning (in the end those extensions can already run code using the system principal).

In which process is SAP going to be used by the extensions? is it going to be used in extension pages and extension shared workers running in the >child extension process?

In terms of Bergamot (which is expected to be delivered as a system addon) and based on a previous chat I had with dveditz, it would run in the extension process, in the wasm sandbox, and the operations we need SAB would run inside workers.

Just to provide more context, this is the code we are talking about: https://github.com/mozilla-extensions/bergamot-browser-extension

"Secure Context" is not the stumbling block here. Web Extensions come from a signed, local archive; they are not loaded over insecure HTTP. If they aren't already considered a Secure Context we should fix that. (I'm excluding content scripts which I assume are treated like the pages they are added to, and in any case we don't need to allow SAB in content scripts.) SAB requires much more than just a Secure Context.

One of the prerequisites for re-enabling SAB is that the page has to use the Cross-Origin-Opener-Policy header with the value same-origin, which requests a separate process for just that origin, not lumped with an entire "site" as in normal Fission. We need a similar kind of opt-in/permission for Web Extensions, and we'll have to be able to check for it in the SAB code since there aren't any HTTP headers in an extension context. Do we need to actually spin up a separate Web Extension process, or can we trust them on pain of review and banning not to abuse the feature against other extensions? For at least a while we could get away with just being stingy on handing out the permission (that is, no automatic signing for those)

I don't know how often Extensions have web-accessible documents, but we'd probably want to forbid those specific pages from using SAB, too.

The other things web pages have to do to use SAB is set the Cross-Origin-Embedder-Policy header to require-corp. This tells the browser to block any cross-origin loads unless they are CORS enabled or have a Cross-Origin-Resource-Policy header. This prevents a Spectre-like attach on loaded resources unless the cross-origin site has explicitly opted into sharing. Hardly any sites are using CORP headers, though, so it might be a stumbling block if the same manifest option that turns on the COOP-equivalent also required COEP in every context. Is there an existing manifest structure that can associate options with specific web extension resources? Is it good enough for SAB if COEP is specified in a meta http-equiv element? What do we do if background scripts want to use SAB? COOP can't be in an http-equiv because we need to know about the request before we've picked a process in which we eventually parse and find the <meta> tag. I don't know about COEP, but there is the worry about resources that are loaded or pre-loaded before we could get to the http-equiv so maybe we don't allow that either.

Limiting the permission to only Privileged Extensions could be a choice to reduce the worry about abuse and review overhead, but might ultimately prove too restrictive. And doesn't help solve the problem that an extension might want to use SAB on some contexts but not have COEP on others that need to load web resources.

Flags: needinfo?(dveditz)

Daniel, why do we need a similar mechanism? What information are we protecting that extensions cannot already access? Is the threat here reading the memory of other extensions? (As in, I don't understand all the COEP/CORP mentions, as far as I know extensions can already fetch any resource they like.)

Flags: needinfo?(dveditz)

Yeah, I forgot extensions could fetch anything without needing CORS; for some reason I thought they could only do that for the URLs they've added to the permissions list (which is often <all_urls>, but still...). So no need for a COEP stand-in. There is a potential issue of an extension attacking the memory of another extension since they all run in the same process, but a malicious extension can already do so many bad things that we simply have to be alert and kill those as fast as we can. We don't need a COOP stand-in.

So the main question then is whether we want to require an explicit manifest permission, which will help with auditability and maybe discourages abuse but otherwise doesn't do much. If someone is going to use SAB they are probably CPU-intensive and could give people a bad experience if they're not aware.

Flags: needinfo?(dveditz)

It sounds like we have 2 options then:

  1. Only allow this for privileged extensions.
  2. Gate it through a silent permission.

The first one is safer but more limited. It's not clear to me if this is something extension developers have a need for, though, so I'm not sure if we win much by taking the risk and going with the second option.

Extension developers probably don't need this much yet - using threads and shared memory is still very non-ergonomic and there are few use cases, though the translation use case is certainly valid. (The ergonomics will change, in time.) IMO it also feels safer to keep this limited to privileged extensions for now.

Thanks for all the comments.

So it's safe to say we reached an agreement and are going with the first option then (Only allow this for privileged extensions)?

Yes, lets keep it to privileged extensions, but also require a specific privileged permission[1].

[1] https://searchfox.org/mozilla-central/rev/63fcc3f1a2cc73488d8986f4cf91fce2cd4b7564/toolkit/components/extensions/Extension.jsm#164

Thanks Shane! I'll take this

Assignee: nobody → anatal
Flags: needinfo?(mlopatka)
Component: JavaScript: Standard Library → Compatibility
Product: Core → WebExtensions
Priority: -- → P2
Severity: -- → N/A

thoughts in allowing access from privileged (or permission-ed) addons:

https://searchfox.org/mozilla-central/rev/2819d981824ad8a707885346d7ddb3bd3e077c9c/dom/base/nsGlobalWindowInner.cpp#2594-2611
5:10
https://searchfox.org/mozilla-central/rev/2819d981824ad8a707885346d7ddb3bd3e077c9c/dom/chrome-webidl/WebExtensionPolicy.webidl#46-50
5:14

In nsGlobalWindowInner::IsSharedMemoryAllowed we could check if the principal owning the window is an addon principal with privilege doing something like:

if (auto policy = BasePrincipal::Cast(principal)->AddonPolicy()) {
  // WebExtensionPolicy also has a method of hasPermission if we implement a permission for this
  if (auto privileged = policy->IsPrivilaged) {
     return true;
  }
}

I'm not sure how to get a hold of the correct prinicpal for this, is it on browsing context?
Tom, do you know?

Flags: needinfo?(tomica)

(In reply to Shane Caraveo (:mixedpuppy) from comment #25)

I'm not sure how to get a hold of the correct prinicpal for this, is it on browsing context?

There's a GetPrincipal right there on the nsIGlobalWindowInner
https://searchfox.org/mozilla-central/rev/49e0a92839/dom/base/nsGlobalWindowInner.cpp#2224

Flags: needinfo?(tomica)

Allow the usage of SharedArrayBuffer by priviliged addons

(In reply to Tomislav Jovanovic :zombie from comment #26)

(In reply to Shane Caraveo (:mixedpuppy) from comment #25)

I'm not sure how to get a hold of the correct prinicpal for this, is it on browsing context?

There's a GetPrincipal right there on the nsIGlobalWindowInner
https://searchfox.org/mozilla-central/rev/49e0a92839/dom/base/nsGlobalWindowInner.cpp#2224

I tried using GetPrincipal() here but for some reason both mDoc as mDocumentPrincipal are null when we are in the extension process (i.e., when ExtensionPolicyService::GetSingleton().IsExtensionProcess() is true) we want to validate. Do you have more suggestions, Tom?

Flags: needinfo?(tomica)

I tried using GetPrincipal() here but for some reason both mDoc as mDocumentPrincipal are null when we are in the extension process (i.e., when ExtensionPolicyService::GetSingleton().IsExtensionProcess() is true) we want to validate. Do you have more suggestions, Tom?

I'm really not an expert here, but that is weird. Is IsSharedMemoryAllowed() being called too early perhaps, before everything is properly set up? Is it maybe this call in nsGlobalWindowOuter::SetNewDocument?
https://searchfox.org/mozilla-central/rev/3151f97de2/dom/base/nsGlobalWindowOuter.cpp#2233-2244

Sorry I can't be of more help here, you probably want someone like smaug or Nika.

Flags: needinfo?(tomica)
Blocks: 1710546
Pushed by bugmail@asutherland.org: https://hg.mozilla.org/integration/autoland/rev/eab06e94978e Allow the usage of SharedArrayBuffer by priviliged addons r=mixedpuppy,asuth,nika

https://hg.mozilla.org/mozilla-central/rev/eab06e94978e

Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
status-firefox90: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

I am sorry for disturbing people, but what "privileged extension" means? Does it mean third parties can't create extensions using SAB?

(In reply to Evengard from comment #32)

I am sorry for disturbing people, but what "privileged extension" means? Does it mean third parties can't create extensions using SAB?

At the moment, I believe that is the case, yes. I think it would be reasonable to expect that limitation to be removed eventually, but the security implications will need to be understood properly first.

That's unfortunate, as it limits severely the usage of WASM in extensions (which requires SAB for multithreading stuff). I thought about using FFMPEG.WASM (actually the same stuff the person who started this bug) for a WebExt, but it doesn't even have a version running without SAB.

The mentioned bug isn't accessible =(

See Also: → 1673477
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: