Kevin Grandon :kgrandon Reporter
	Description • 10 years ago

We see the following error when trying to create a custom element in a FirefoxOS addon:

Error sandboxing app://app/script.js : SecurityError: The operation is insecure.


This happens as soon as you call document.registerElement();

	Kevin Grandon :kgrandon Reporter
	Comment 1 • 10 years ago

I've learned that if the origin of the prototype for create element differs from the page, we may run into security exceptions.

Public spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27260

	Kevin Grandon :kgrandon Reporter
	Comment 2 • 10 years ago

Spoke to wchen over IRC, the exception is possibly being thrown here: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#6204

It's possible that the script loader put the prototype in a compartment different from the page and when we try to access the prototype from the page's compartment we can't because there's a security policy in place on the wrapper (https://dxr.mozilla.org/mozilla-central/source/js/src/proxy/Wrapper.cpp#119).

	Doug Sherk (:drs) (inactive)
	Comment 3 • 10 years ago

William, could you take a look at this?

	William Chen [:wchen] Assignee
	Comment 4 • 10 years ago

Kevin, do you think you could provide a test case that I can debug?

	Kevin Grandon :kgrandon Reporter
	Comment 5 • 10 years ago

Here is a packaged addon which causes the error to appear. The easiest way to see this effect is if you unpack this addon to the outoftree_apps/ folder inside of a gaia checkout. You may need to create the outoftree_apps/ folder if it doesn't exist. Assuming you have a gaia checkout inside of a folder named 'gaia', when unpacked the folder should live somewhere like:

gaia/outoftree_apps/fxos-customizer/

There should be a manifest file at:

gaia/outoftree_apps/fxos-customizer/manifest.webapp

	Kevin Grandon :kgrandon Reporter
	Comment 6 • 10 years ago

Here is a new build of the customizer which works with the gecko patch. I wasn't able to get it working with all apps, but it did work on the contacts and messaging apps. I'll work with the gaia-devs to start looking into some of these issues.

	William Chen [:wchen] Assignee
	Comment 7 • 9 years ago

So the situation we have is that the add-on script is executed with an extended principal giving it more privilege than the app. The current registerElement implementation throws a security exception for prototypes that can't be seen from the document's compartment (in this case, it's because the app's principal is less privileged than the principal for the add-on script).

It's actually not to clear what we should do here. In the attached patch, I've changed the implementation to allow the prototype from the add-on script and elements that are created will have the registered prototype. However, the prototype will only be visible from the add-on script and it will not be visible from the app. This means that any API that you added on the prototype won't be available to scripts in the app.

	William Chen [:wchen] Assignee
	Comment 8 • 9 years ago

bholley: How do you think we should behave when we call registerElement with a privileged prototype?

	Bobby Holley (:bholley)
	Comment 9 • 9 years ago

So. It all really depends on what we really want and how much work we want to do here.

If the addon wants to create a custom element accessible from content _and_ implemented in content (i.e. no security boundary), it should just use window.eval('{...}') to create the custom prototype. In this case, the custom prototype is not going to be visible at all to the addon (and the XrayWrapper will cause the prototype to go directly to HTMLCustomElement.prototype, I believe).

If the addon wants to create a custom element accessible from content _but_ implemented in the addon scope (i.e. with a security boundary), then it should create an content-sideAPI object using exportFunction (or, more conveniently, cloneInto(apiObj, window, {cloneFunctions: true}). If |apiObj| only has accessors and methods (and any underlying data is stored on a different object), then cloning it into content will create a content-side object with a bunch of content-side functions that forward to the addon-side functions (make sure that you use a closure to access your data and not |this|, since |this| can't be trusted). This should work for the content, but still won't make the custom prototype visible to code running in the addon scope.

If we want to make custom elements with prototypes that are usable from the addon (but not from content), then we need:
(1) Code to deal with the fact that the prototype might not be same-compartment with the document (i.e. something along the lines of wchen's patch above)
(2) Xray support to report the proper prototype to XrayWrappers.

If we want to make custom elements with prototypes that are usable from both the addon _and_ content, things get tricky. The basic issue is that there isn't an easy way to make an arbitrary user-create JS object (the prototype) work naturally for both foo.com and nsEP([foo.com]) simultaneously. There's not really any way to Xray to these objects (since there's nothing to Xray _to_, they're just regular Objects). There are various complicated ways we could try to kinda-sorta support this, but I don't want to spend time brainstorming it unless we actually want this badly enough to spend a week or two of someone's time making it work.

	Bobby Holley (:bholley)
	Comment 10 • 9 years ago

Comment on attachment 8571460 [details] [diff] [review]
Possible fix

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

::: dom/base/nsDocument.cpp
@@ +6207,5 @@
>      } else {
>        protoObject = aOptions.mPrototype;
>  
> +      {
> +        JSAutoCompartment ac(aCx, protoObject);

Before doing this, you _need_ to make sure that the caller subsumes the underlying object (after unwrapping it). Otherwise, the caller could pass a cross-origin object, it would get re-wrapped into the content compartment, and then the content document has no way of knowing whether the caller was allowed to access the underlying object or not.

@@ +6221,5 @@
>  
> +        JS::Rooted<JSPropertyDescriptor> descRoot(aCx);
> +        JS::MutableHandle<JSPropertyDescriptor> desc(&descRoot);
> +        // This check will go through a wrapper, but as we checked above
> +        // it should be transparent or an xray. This should be fine for now,

Where does this check happen?

	William Chen [:wchen] Assignee
	Comment 11 • 9 years ago

In this particular case we want the custom element to work in the add-on.

bholley: Can you review this? or perhaps reassign to mrbkap.

	Bobby Holley (:bholley)
	Comment 12 • 9 years ago

Comment on attachment 8572281 [details] [diff] [review]
Set registered prototype in compartment of caller of registerElement

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

This needs tests. Write a mochitest-chrome with a content iframe and a Sandbox with nsEP(iframePrincipal), and try doing web components stuff on it.

::: dom/base/Element.cpp
@@ +420,5 @@
>      nsDocument* document = static_cast<nsDocument*>(OwnerDoc());
>      JS::Rooted<JSObject*> prototype(aCx);
>      document->GetCustomPrototype(NodeInfo()->NamespaceID(), data->mType, &prototype);
>      if (prototype) {
> +      // We want to set custom prototype in the compartment where the prototype

"We want to set the custom prototype in the compartment where it was registered".

@@ +424,5 @@
> +      // We want to set custom prototype in the compartment where the prototype
> +      // was registered so that the custom prototype is visible to custom
> +      // element callbacks in the compartment where registration occurred.
> +      JSAutoCompartment ac(aCx, prototype);
> +      if (!JS_WrapObject(aCx, &obj) || !JS_SetPrototype(aCx, obj, prototype)) {

So what this is doing is setting a custom "expando" prototype on the XrayWrapper that is only visible from the origin that set it. This isn't what I had in mind, but actually might work, assuming that it's really ok for the prototype to only exist from the XrayWrapper's perspective and not that of the underlying element (or XrayWrappers from any other origins).

This seems just crazy enough to work, but I'm worried that it might break underlying C++ invariants in the web components code. Blake?

::: dom/base/nsDocument.cpp
@@ +6235,5 @@
>          return;
>        }
>  
> +      // Enter the compartment of the protoObject just for some checks.
> +      JSAutoCompartment ac(aCx, protoObject);

You're still in the original compartment of protoObject, right? I don't see us unwrapping it anywhere, so this ac seems like a no-op.

@@ +6342,5 @@
>    // Associate the definition with the custom element.
>    CustomElementHashKey key(namespaceID, typeAtom);
>    LifecycleCallbacks* callbacks = callbacksHolder.forget();
>    CustomElementDefinition* definition =
> +    new CustomElementDefinition(wrappedProto,

Here it looks like we're storing the prototype wrapped into the document's compartment, which contradicts the comment in nsDocument.h. What's the story?

@@ +6378,1 @@
>          if (!JS_SetPrototype(aCx, wrapper, protoObject)) {

This needs a comment explaining that we may be only setting the prototype on an XrayWrapper for the element (here and in the code in Element.cpp).

	Bobby Holley (:bholley)
	Comment 13 • 9 years ago

Oh, also - how are createdCallback etc going to work? I don't see any code to handle crossing compartment boundaries in that case.

	William Chen [:wchen] Assignee
	Comment 14 • 9 years ago

(In reply to Bobby Holley (:bholley) from comment #12)
> Comment on attachment 8572281 [details] [diff] [review]
> Here it looks like we're storing the prototype wrapped into the document's
> compartment, which contradicts the comment in nsDocument.h. What's the story?
> 
I think we're in the caller's compartment when we call JS_WrapObject(aCx, &wrappedProto)

> Oh, also - how are createdCallback etc going to work? I don't see any code
> to handle crossing compartment boundaries in that case.
The callbacks are WebIDL CallbackFunction so I think the bindings should handle that when we call |Call| on them.

	Doug Sherk (:drs) (inactive)
	Comment 16 • 9 years ago

Comment on attachment 8572930 [details] [diff] [review]
v1 diff v2

With both attachment 8572930 [details] [diff] [review] and attachment 8571460 [details] [diff] [review], I'm getting this error under the same circumstances:
W/GeckoConsole(13025): [JavaScript Error: "NotSupportedError: Operation is not supported" {file: "app://ac1b484a-843c-fd4f-8244-8348a4479ce4/js/addon-concat.js" line: 1545}]

This is that line:
```js
module.exports = document.registerElement('gaia-dialog', { prototype: proto });
```

	William Chen [:wchen] Assignee
	Comment 17 • 9 years ago

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #16)
> Comment on attachment 8572930 [details] [diff] [review]
> v1 diff v2
> 
> With both attachment 8572930 [details] [diff] [review] and attachment
> 8571460 [details] [diff] [review], I'm getting this error under the same
> circumstances:
> W/GeckoConsole(13025): [JavaScript Error: "NotSupportedError: Operation is
> not supported" {file:
> "app://ac1b484a-843c-fd4f-8244-8348a4479ce4/js/addon-concat.js" line: 1545}]

That's a different error likely caused by a different issue (my guess is that gaia-dialog was already registered earlier by something else?). I can take a look if you upload the add-on.

	Doug Sherk (:drs) (inactive)
	Comment 18 • 9 years ago

(In reply to William Chen [:wchen] from comment #17)
> That's a different error likely caused by a different issue (my guess is
> that gaia-dialog was already registered earlier by something else?). I can
> take a look if you upload the add-on.

The only way to get past this was to comment out every `document.registerElement()` call, so I don't think any elements are being registered more than once.

Here's the app:
http://people.mozilla.org/~dsherk/customizer.zip

You can unzip this and then install it using WebIDE. Make sure that you go into Settings > Addons and enable this add-on. The errors should pop up right away in |adb logcat| once enabled.

	Kevin Grandon :kgrandon Reporter
	Comment 19 • 9 years ago

When I tested the patch it was working for me... It seems to be a different issue. In order to not conflate things, should we track this in a different bug?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 20 • 9 years ago

Comment on attachment 8572929 [details] [diff] [review]
Set registered prototype in compartment of caller of registerElement. v2

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

r=me with an answer to the question of what to do in the case of failures during the element upgrade algorithm.

::: dom/base/Element.cpp
@@ +425,5 @@
> +      // registered. In the case that |obj| and |prototype| are in different
> +      // compartments, this will set the prototype on the |obj|'s wrapper and
> +      // thus only visible in the wrapper's compartment.
> +      JSAutoCompartment ac(aCx, prototype);
> +      if (!JS_WrapObject(aCx, &obj) || !JS_SetPrototype(aCx, obj, prototype)) {

Overwriting obj here means that we'll return the wrapper instead of the object itself. I'd prefer to use a temporary to do this SetPrototype.

::: dom/base/nsDocument.cpp
@@ +6376,3 @@
>        JS::RootedObject wrapper(aCx);
> +      if ((wrapper = cache->GetWrapper()) && JS_WrapObject(aCx, &wrapper)) {
> +        if (!JS_SetPrototype(aCx, wrapper, wrappedProto)) {

Nit: You can combine the two if statements with && here.

Non-nit: if JS_WrapObject or JS_SetPrototype fail, then we need to propagate the error back out to the caller.

The spec doesn't deal with the case of failure during this bit of the algorithm, but we probably need to do something if we throw an exception during either the JS_WrapObject or JS_SetPrototype calls. Right now, we risk leaving an exception on aCx. At the very least, we should probably call JS_ClearPendingException or JS_ReportPendingException in the case of failure.

	Bobby Holley (:bholley)
	Comment 21 • 9 years ago

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #20)
> The spec doesn't deal with the case of failure during this bit of the
> algorithm, but we probably need to do something if we throw an exception
> during either the JS_WrapObject or JS_SetPrototype calls. Right now, we risk
> leaving an exception on aCx. At the very least, we should probably call
> JS_ClearPendingException or JS_ReportPendingException in the case of failure.

For any new error handling stuff, please use the API on AutoJSAPI by invoking TakeOwnershipOfErrorReporting and then either clearing the exception, stealing it, or letting the AutoJSAPI destructor report it.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 22 • 9 years ago

Ah-ha. I guess that in that case we'll need to add an AutoJSAPI and use it here.

	Bobby Holley (:bholley)
	Comment 23 • 9 years ago

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #22)
> Ah-ha. I guess that in that case we'll need to add an AutoJSAPI and use it
> here.

Where is the cx coming from? If it's being passed in from DOM bindings, then you don't need to do anything - just propagate the exception.

	Bobby Holley (:bholley)
	Comment 24 • 9 years ago

The basic idea is that you instantiate an AutoJSAPI when you want to enter JS with a clean slate. Stuff like registerElement inherits the JS state from the JS caller, and thus doesn't need to do anything fancy with exceptions.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 25 • 9 years ago

I think that the trickiness is that we don't abort the algorithm at the first exception. Right now, we'll happily call into the JSAPI with an exception pending from a previous operation, which (AFAIK) is not allowed. I think that means that we need to explicitly clear the exception if one is thrown.

	Bobby Holley (:bholley)
	Comment 26 • 9 years ago

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #25)
> I think that the trickiness is that we don't abort the algorithm at the
> first exception. Right now, we'll happily call into the JSAPI with an
> exception pending from a previous operation, which (AFAIK) is not allowed. I
> think that means that we need to explicitly clear the exception if one is
> thrown.

Oh I see. Yes, JS_ClearPendingException is an OK thing to do I guess. Long-term we'll be passing around AutoJSAPI& / AutoEntryScript& instead of JSContext* (or accessing them ambiently), so we then we can just invoke ClearPendingException on the JSContext itself.

	Bobby Holley (:bholley)
	Comment 27 • 9 years ago

(In reply to Bobby Holley (:bholley) from comment #26)
> so we then we can just invoke
> ClearPendingException on the JSContext itself.

Er, on the AutoJSAPI itself.

	Candice Serran (:cserran)
	Comment 28 • 9 years ago

Hey William,

Any update on when this can land?

	William Chen [:wchen] Assignee
	Comment 29 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ceda5e90d080

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #20)
> At the very least, we should probably call
> JS_ClearPendingException or JS_ReportPendingException in the case of failure.

Addressed review comments and used JS_ReportPendingException.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 9 years ago

Doesn't this patch change the return value of Element::WrapObject to be in the custom proto compartment?  That's not what the contract for this function is...

I guess I'll fix that up in bug 1145017.

	Carsten Book [:Tomcat]
	Comment 31 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/d8c689def44e

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 9 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #32)
> https://hg.mozilla.org/integration/b2g-inbound/rev/d8c689def44e

This was from bug 1135293, which had the wrong bug number in the commit message.