Closed Bug 1340719 Opened 3 years ago Closed 2 years ago

Don't allow add-ons to generate docgroup mismatches


(Core :: XPConnect, defect)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: billm, Assigned: billm)


(Blocks 1 open bug)


(Keywords: addon-compat)


(1 file)

It's pretty easy for add-ons to cause DocGroup mismatches in the child process. For example, if a message manager message sent to one tab accesses content from a tab in a different TabGroup. That's not so much problem right now because the assertions in bug 1337537 are DEBUG-only. (It is a problem if you run a DEBUG build with add-ons. Sorry about that.)

This patch changes our implementation of wrappers so that they don't allow access to objects in the wrong DocGroup. So if you're in a runnable that was labeled with DocGroup A and you try to access DocGroup B (and A != B), then we will throw instead of asserting.

It's kind of annoying that we need to do this since the plan is that there will be no more legacy add-ons when Quantum DOM ships. And WebExtensions can't cause this problem. However, I would like to be able to land parts of Quantum DOM and enable them to get testing before Firefox 57. And I'd also rather avoid adding a dependency on add-on deprecation, which is a massive effort that could be delayed or changed. Also, the patch is pretty simple.

One problem with the patch is how it handles frame scripts. All frame scripts share a single TabChildGlobal. If we had a separate TabChildGlobal for add-ons, then we would be creating an extra global per tab, which seems like a lot. To work around this, I've turned on the DocGroupValidation wrappers for all frame scripts, even our own. This will slow down our own frame scripts a bit. However, almost all our code in the content process runs from JSMs, so I don't think this is a problem. I also made a pref that causes this validation to be disabled during tests. That way, we don't accidentally come to rely on it in our own code.

The code itself is pretty simple. It's a proxy that implements every trap, checks the docgroup, and then delegates to the base implementation. It's just a lot of boilerplate.
Attached patch patchSplinter Review
Attachment #8838777 - Flags: review?(bobbyholley)
Comment on attachment 8838777 [details] [diff] [review]

Review of attachment 8838777 [details] [diff] [review]:

Really sorry for the delay here. r=me with comments addressed.

Please file a bug about removing this stuff when XPCOM addons go away. I'd appreciate if you could follow up and do the removal when that happens.

::: dom/base/test/browser_docgroup_forbid.js
@@ +2,5 @@
> +
> +function frameScript() {
> +  let e = Services.ww.getWindowEnumerator();
> +  let exception = false;
> +  while (e.hasMoreElements()) {

Can we be a bit stricter about what windows we actual expect to exist, and fail the test if they're not set up the way we expect? The current setup makes it harder to understand what the expectations are here.

@@ +7,5 @@
> +    try {
> +      var window = e.getNext().QueryInterface(Components.interfaces.nsIDOMWindow);
> +      var doc = window.document;
> +    } catch (e) {
> +      exception = true;

Can you return the exception string instead of a boolean, and /regexp/.test() it when determining whether or not to pass the test? String-checking expected exceptions is a lot more robust.

::: js/xpconnect/idl/nsIXPConnect.idl
@@ +306,5 @@
>      const uint32_t INIT_JS_STANDARD_CLASSES  = 1 << 0;
>      const uint32_t DONT_FIRE_ONNEWGLOBALHOOK = 1 << 1;
>      const uint32_t OMIT_COMPONENTS_OBJECT    = 1 << 2;
> +    const uint32_t VALIDATE_DOCGROUP_ACCESS  = 1 << 3;

This doesn't need to be an initialization flag IIUC. The caller can just use an xpc:: API set it on the compartment private after creating it. I'd rather keep the initialization flags limited to stuff that can't be handled ex-post-facto.

::: js/xpconnect/wrappers/DocGroupValidationWrapper.cpp
@@ +26,5 @@
> +    if (window->GetDocGroup()->AccessAllowed())
> +        return true;
> +
> +    // If DocGroup validation is disabled, don't throw.
> +    if (!Preferences::GetBool("extensions.throw_on_docgroup_mismatch.enabled", true))

Please make this a bool var cache.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +443,5 @@
> +    JSCompartment* origin = js::GetObjectCompartment(obj);
> +    JSCompartment* target = js::GetContextCompartment(cx);
> +
> +    XPCWrappedNativeScope* originScope = CompartmentPrivate::Get(origin)->scope;
> +    if (nsContentUtils::IsSystemPrincipal(originScope->GetPrincipal()) ||

Just do AccessCheck::isChrome(obj).

@@ +476,5 @@
> +        return &DocGroupValidationWrapper<AddonWrapper<PermissiveXrayDOM>>::singleton;
> +    else if (wrapper == &WaiveXrayWrapper::singleton)
> +        return &DocGroupValidationWrapper<WaiveXrayWrapper>::singleton;
> +
> +    // |wrapper| is not supported for validation, so we don't do it.

Hm, what are these cases? Can you enumerate them, and assert against the ones you don't expect?

@@ +597,5 @@
>          // then we try to "upgrade" the wrapper to an interposing one.
>          if (CompartmentPrivate::Get(target)->scope->HasInterposition())
>              wrapper = SelectAddonWrapper(cx, obj, wrapper);
> +
> +        wrapper = SelectDocGroupValidationWrapper(cx, obj, wrapper);

I'd prefer a setup of the form:

if (NeedsDocGroupValidation(cx, obj)) {
  wrapper = AddDocGroupValidation(wrapper);

Rewrap is getting hot these days, so keep the NeedsDocGroupValidation inline and check the most-likely-to-fail bits first. Ideally we'd just check the bit on the compartment private first, but I guess we also need to look up the addon of the object (which isn't expensive, but is out-of-line, and this stuff all adds up) - can we maybe set the compartment private bit at creation time if we have an addon id?

Probably also worth passing targetCompartmentPrivate now that we have it as a local, possibly some other stuff. Just try to make the overhead minimal in the common case.

::: xpcom/threads/Dispatcher.h
@@ +79,5 @@
>    // Ensure that it's valid to access the TabGroup at this time.
>    void ValidateAccess() const
>    {
>      MOZ_ASSERT(!sRunningDispatcher || mAccessValid);

Can you change this to MOZ_ASSERT(AccessAllowed()), and then reshuffle the comments (description on this function goes on AccessAllowed()).
Attachment #8838777 - Flags: review?(bobbyholley) → review+
I was talking to Olli and I think it makes sense to add telemetry to record the add-on IDs of add-ons where this happens.
Flagging this for addon-compat. This bug is changing how frame/process scripts work. If an event fires in a frame or process script, then you're only allowed to access the tab in which the event fired. If you try to touch DOM content in other tabs, you'll get an exception. This change is required for Quantum DOM. I don't expect it to break too much stuff since we didn't see many problems in Firefox.
No longer blocks: 1353224
Keywords: addon-compat
Pushed by
Throw an exception if accessing Xray from wrong docgroup (r=bholley)
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1354637
Pushed by
Revert "Bug 1340719 - Throw an exception if accessing Xray from wrong docgroup (r=bholley)"
I backed this out since it broke "View Source". I filed bug 1354698 about the fact that we have no tests for view source. If I fix bug 1354249, then that should fix view source as well, and then I can land again.
Depends on: 1354249
Resolution: FIXED → ---
Can you change the commit message for this to be more accurate before re-landing? We're not complaining if you access from the wrong docgroup, but rather if you access from the wrong tabgroup. We still are not enforcing docgroup-level code isolation.
Flags: needinfo?(wmccloskey)
I've got tests for View Source cooking in bug 1354698, and these should land today pending a try build.

Note that the test forces the extensions.throw_on_docgroup_mismatch.enabled to true in order to test this case, as it seems as if we disable that pref for test runs. I'm a little worried about us not running tests with that pref enabled if that's what we're going to ship - I can imagine other breakage slipping through if we don't default to testing in that configuration.

At any rate, if and when this re-lands, just a heads up that the test explicitly sets extensions.throw_on_docgroup_mismatch.enabled to true to test the breakage scenario for View Source. If the pref name happens to change while this patch is being worked on, and we don't enable it by default for tests, the test will need to be updated to use the new pref.
Depends on: 1354891
Don't need this anymore.
Closed: 3 years ago2 years ago
Flags: needinfo?(wmccloskey)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.