Closed Bug 1576254 Opened 5 months ago Closed 4 months ago

Allow enabling WASM on Chrome content (including Extensions) but disabling it on web content

Categories

(Core :: Javascript: WebAssembly, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor 23719])

Attachments

(9 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
33.48 KB, patch
Details | Diff | Splinter Review
50.00 KB, application/x-tar
Details

To support extensions that rely on WASM for performant operation (such as HTTPS Everywhere) we would like to enable WASM for them only - while keeping it disabled for web content.

CreationOptions are intended to be immutable and not change during realm operation.
Behaviors change, and clamping/jittering should reside on behaviors.

This is not a good solution, but I'm posting it to get feedback on what you'd
like me to do here. Should I move all the WASM bools down to Realm?

Depends on D43296

Priority: -- → P3

Two questions:

  • For the same reason that wasm is disabled in content, is the JS JIT disabled in content as well? If the JS JIT isn't also disabled in chrome JS, then I'd expect to see all the JIT flags treated symmetrically with wasm.
  • Do the flags actually need to be mutated dynamically or could they be in the immutable RealmCreationOptions?
Flags: needinfo?(tom)

(In reply to Luke Wagner [:luke] from comment #3)

Two questions:

  • For the same reason that wasm is disabled in content, is the JS JIT disabled in content as well? If the JS JIT isn't also disabled in chrome JS, then I'd expect to see all the JIT flags treated symmetrically with wasm.

Yes; the JS JIT is disabled in content by disabling javascript.options.ion, javascript.options.baselinejit, and javascript.options.native_regexp. It would be great to have them treated the same - but those all exist in the global JitOptions object; they're not even on the compartment level. Should I try to move those?

When I mentioned the other WASM options, I meant wasmVerbose, wasmBaseline, wasmIon, wasmCranelift, and wasmGc.

  • Do the flags actually need to be mutated dynamically or could they be in the immutable RealmCreationOptions?

This is going to be a pref, so I typically try to let prefs be updated dynamically if they can. (Clamping/Jittering definitely can easily so that should move.) But you're right, even if this is on Behaviors it's not going to get enabled for a Realm that exists, so it should go to CreationOptions.

Flags: needinfo?(tom)

Ah, thanks for explaining. Actually, this reminds me of another recent discussion where someone (my memory is hazy) had been considering making the wasm flags process-wide too. Lars/Benjamin/Jan: are any of you considering this?

If we did want process-wide flags, then if we want to support Tor's HTTP-everywhere use case, we could have a separate process-wide flag that enabled wasm only when cx->runningWithTrustedPrincipals(). Running with trusted principals already has testing support, which is nice.

That was actually me, and the specific use case was for using huge memory reservations with Wasm (bug 1518210).

The patches ended up using WasmProcess.h with an ad-hoc JS API for disabling the flag that needs to be called once per process before any Wasm objects are created. It's not a great solution, so better support for global flags would be great.

The JIT prefs are per-process and in the browser read once (at startup). This simplified a number of things and I'd suggest doing the same for other prefs if possible. There are different JS runtimes in the browser (main runtime, workers, worklets, PAC) and per-process makes it much easier to reason about our behavior and it avoids 'missing' one.

Whiteboard: [tor] → [tor 23719]

Another hiccup I just found. In XPCJSContext.cpp we call JS::ContextOptionsRef(cx) which is fine, because a Context always has options; but calling JS::RealmBehaviorsRef(cx) is no good because that context doesn't have a realm yet....

So what do we think Tom should do in the short-term? If we want to move the wasm flags in the short-term, then it might make sense to have this change rebased on top of that.

If not, then I think the most future-friendly short-term change (that won't create work we have to undo later) is to keep cx->options().wasm() where it is and add a sibling flag cx->options().enableWasmWhenRunningWithTrustedPrinciples() (that enables wasm when cx->runningWithTrustedPrinciples()) since that allows us to later make both these flags per-process in a mechanical way.

Sound reasonable?

(In reply to Luke Wagner [:luke] from comment #9)

If not, then I think the most future-friendly short-term change (that won't create work we have to undo later) is to keep cx->options().wasm() where it is and add a sibling flag cx->options().enableWasmWhenRunningWithTrustedPrinciples() (that enables wasm when cx->runningWithTrustedPrinciples()) since that allows us to later make both these flags per-process in a mechanical way.

Sound reasonable?

That sounds good to me.

One thing to double check is the behavior of runningWithTrustedPrincipals. If we're not in a realm, that returns true. Else it returns true if realm->principals == runtime->trustedPrincipals. trustedPrincipals is only set on the main runtime (here). That means if there are runtimes that don't use principals, we always treat them as trusted. I don't know if we have those in the browser (worker realms have their own GetWorkerPrincipal() at least..) but I'm concerned about new things like worklets. Anyway it might make sense to tighten runningWithTrustedPrincipals to return false if rt->trustedPrincipals == nullptr. Or something.

Flags: needinfo?(luke)

Thanks for pointing that out! That seems like a problem for more than just this (runningWithTrusted prinicipals seems to be used for some callstack permission check). Yes, it seems like we should have the null-trustedPrincipals case return false.

Flags: needinfo?(luke)

So I wrote up this code and tested it. runningWithTrustedPrincipals is not sufficient - it is not true for Extensions. What I want to test is nsIPrincipal::isAddonOrExpandedAddonPrincipal() - I verified this by using cx->realm()->principals()->dump();

However, it seems that inside js/src - there is no notion of what the JSPrincipals are, it's just an opaque data structure. I want to do something like nsJSPrincipals::get(cx->realm()->principals())->isAddonOrExpandedAddonPrincipal() (similar to here ) - but nsJSPrincipals lives in caps/ not js/src

Flags: needinfo?(luke)

I suppose one easy option is to ignore "trusted principals" and add a virtual method to JSPrincipals, say, isSystemOrAddon() and use that instead (and then the pref would be enableWasmInSystemOrAddonCode(). I actually wonder whether maybe native stack limits should be expanded for add-on code too but (1) that's a separate question, (2) GetNativeStackLimit() is probably pretty hot, and thus benefiting from doing a single pointer compare vs. virtual call.

Flags: needinfo?(luke)

Hey baku, I have some worker principal problems. I am trying to add a virtual method onto JSPrincipals that basically reports nsIPrincipal::GetIsAddonOrExpandedAddonPrincipal() I did that in this patch. But WorkerPrincipal is tricky because we only ever construct one of them as a placeholder-type object.

I cut it over to be a real object in this commit. I initially held a RefPtr to WorkerPrivate like in Worklet but this failed when I tried to AddRef on other threads here.

So I change WorkerPrincipal to just store a bool and call mWorkerPrivate->GetPrincipal()->GetIsAddonOrExpandedAddonPrincipal() in those places but that fails because GetPrincipal() can only be called on Main Thread while in all four of these callsites we assert mWorkerPrivate->AssertIsOnWorkerThread();

I'm not sure what to do here; or why these thread restrictions are in place. Could you advise on how I can satisfy the goal of having a WorkerPrincipal object have a isSystemOrAddonPrincipal() method?

Try run with patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba89a242aee0df469b0c5ef9da2fab1188a3f0a7

Flags: needinfo?(amarchesini)

I think I might have figured this out by making a UsesAddonOrExpandedAddonPrincipal() next to UsesSystemPrincipal()...

In a future commit we will tie this boolean to its own preference value, but here we
initialize it with the same value as the wasm boolean.

We also update wasm::HasSupport to check the to-be-added isSystemOrAddonPrincipal()
method on JSPrincipals to determine which member (wasm or wasmForTrustedPrinciples)
to consult.

Depends on D43296

WorkletPrincipal inherits JSPrincipals so we need to add the isSystemOrAddonPrincipal
method to it. However Worklets are accessed off-main-thread, so we cannot directly
consult the nsIPrincipal stored in the Worklet's LoadInfo. Instead we copy the relevant
information into a member that can be accessed off the main thread.

Depends on D47474

Unlike WorkletPrincipal, a WorkerPrincipal had been a simple static object shared by
all Workers. We never needed to consult it about an individual Worker before. Now we
do. So we cut it over from a static object to individual objects for each Worker.

We have the same off main thread access problem for the Principal as we had for the
Worklet. In this case; however, WorkerPrivate had a similar method UsesSystemPrincipal
that returns a bool that was initialized from the Principal on the main thread. We
copy that pattern and add a UsesAddonOrExpandedAddonPrincipal method that will be called
by the isSystemOrAddonPrincipal method we must implement so we can inheirt from
JSPrincipal.

Depends on D47475

Finally, here we add the virtual method isSystemOrAddonPrincipal to the
JSPrincipal object.

We also add it to nsJSPrincipal (where it has an easy implementation), and
to carry classes that are used by JS tests and the shell.

Depends on D47476

I have no idea where or how RustJSPrincipal is used, or how this method should work here.

Depends on D47477

Alright, I think I for this figured out. I attached the patchset, here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b737a75784b590964e846646bb1fb2eaad6805 (hopefully green, I've done several green partials.)

The last patch, for RustPrincipal, is illustrative, but I need some feedback on what to do there. I'll need to find a reviewer for the Worker and Worklet code; baku is pretty overloaded right now.

Flags: needinfo?(amarchesini)
Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f49089ef86ea
Move Clamping and Jittering from RealmCreationOptions to Behaviors r=luke
https://hg.mozilla.org/integration/autoland/rev/138a502097d4
Add a wasmForTrustedPrinciples bool onto ContextOptions that (currently) behaves the exact same as the wasm bool r=luke
https://hg.mozilla.org/integration/autoland/rev/b99e1c5e8d22
Update wasmForTrustedPrinciples to use a separate pref r=luke
https://hg.mozilla.org/integration/autoland/rev/59f5758eb829
Add isSystemOrAddonPrincipal to WorkletPrincipal r=baku
https://hg.mozilla.org/integration/autoland/rev/2498e68e641c
Cut WorkerPrincipal over to a real object and implement isSystemOrAddonPrincipal r=baku
https://hg.mozilla.org/integration/autoland/rev/7c7ccb6e9ce5
Add isSystemOrAddonPrincipal to JSPrincipal and nsJSPrincipals r=luke
https://hg.mozilla.org/integration/autoland/rev/35dfc96baca1
Add isSystemOrAddonPrincipal to RustJSPrincipal r=luke

Hey Tom, can you please give a short summary of why and how of this bug?

Since it specifically targets extensions, exposes a new public capability/api, and has potential security implications, the Web Extensions team would like to be informed of the implications.

  1. Was HTTPS Everywhere the only motivating factor for this bug? What exactly is their usecase, and how do they want/need to use wasm, from the background page only, or also from content scripts?

  2. From reading the discussion above, it looks like the JS team would like to end up in a place where wasm/JIT flags are per-process, right? But from landed patches, it looks like wasm is also enabled for content scripts (ExpandedAddonPrincipal), which would make process-wide flags for this impossible, even under Fission.

We generally try to avoid exposing more security sensitive capabilities to content scripts, since they run in the content process, and as a defense-in-depth principle, exploiting background pages is considered a higher fence.

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

Hey Tom, can you please give a short summary of why and how of this bug?

Since it specifically targets extensions, exposes a new public capability/api, and has potential security implications, the Web Extensions team would like to be informed of the implications.

I think there's a misunderstanding - this doesn't expose any new capabilities to extensions. They can't control wasm through this patch; and it actually won't impact Firefox at all.

Firefox has a pref to control whether wasm is on/off - it's on of course. But for Tor Browser, at medium/high security level, they disable wasm (and IonMonkey) to reduce attack surface for browser exploits. However because disabling wasm disables it everywhere; HTTPS Everywhere became very slow. So we developed this patch that allows them to turn wasm off for web content but keep it enabled for the system/add-ons.

It's a separate pref that extensions cannot control - it needs to be set by the user (or an old-style system add-on).

Hopefully that addresses your concerns.

Flags: needinfo?(tomica)

Thanks for clearing up my misunderstanding.

As a side note, even if this is limited to the Tor Browser, my recommendation about constraining this to only extension background pages (if possible) still stands, but you probably already understand the implications of that.

Flags: needinfo?(tomica)

jandem - to Tomislav's point, for the future work around per-process flags (which I don't fully understand) is it beneficial to not enable wasm in content scripts?

Even still - it's possible (I'm not certain) the content process runs JS in the system principal context - if it does, it wouldn't matter about content scripts since a system principal compartment would have wasm and the non-system principal wouldn't.

Flags: needinfo?(jdemooij)
Attached file esr68patches.tar

Attaching esr68 backports of this for Tor. One giant patch and a tar of the individual ones.

Flags: needinfo?(jdemooij)

(In reply to Tom Ritter [:tjr] (needinfo for responses to sec-[approval/ratings/advisories]) from comment #28)

jandem - to Tomislav's point, for the future work around per-process flags (which I don't fully understand) is it beneficial to not enable wasm in content scripts?

Sorry for the delay. The JIT prefs are already per-process and I was suggesting doing the same for the Wasm prefs. I think that's still possible with your principal-based approach. I don't know how this would affect content scripts though.

You need to log in before you can comment on or make changes to this bug.