1161748 - [Add-ons] Unable to pass objects in MozActivity from add-on code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Justin D'Arcangelo [:justindarc]]

In Bug 1159376, we are trying to update the Customizer add-on to use an 'import-app' activity provided by the System app to handle the installation of add-on blobs. However, when we invoke the MozActivity from the Customizer, we are getting a 'NO_PROVIDER' DOMError because the 'import-app' activity defines this blob argument as 'required':

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/manifest.webapp#L79

Right before this DOMError, we get a JavaScript warning about XrayWrapper that I believe is pointing to the cause of this issue:

W/Studio  (11672): [JavaScript Warning: "XrayWrapper denied access to property data (reason: value not same-origin with target). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported." {file: "jar:file:///system/b2g/omni.ja!/components/ActivityProxy.js" line: 96}]

It seems like the add-on sandboxing is blocking us from passing Blobs through activities.

Comment 1

[Justin D'Arcangelo [:justindarc]]

Also, the code I'm attempting to call from the Customizer add-on looks like this:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/import_app_test.js#L66-L74

Comment 2

[Bobby Holley (:bholley)]

That error means that the code passing aOptions is defining a cross-origin object "data" on it, and we're refusing the read that as chrome. What are the origins of these things, exactly?

Comment 3

[Justin D'Arcangelo [:justindarc]]

(In reply to Bobby Holley (:bholley) from comment #2)
> That error means that the code passing aOptions is defining a cross-origin
> object "data" on it, and we're refusing the read that as chrome. What are
> the origins of these things, exactly?

The Blob I'm passing is originating from the Customizer add-on which is sandboxed. The 'import-app' activity is supposed to be handled by the System app.

Comment 4

[Justin D'Arcangelo [:justindarc]]

I thought it might be specific to Blobs, but I just tried putting an empty string in place of it in the object being passed for the `data` argument and it also failed:

    var activity = new MozActivity({
      name: 'import-app',
      data: { blob: '' }
    });

Comment 5

[Bobby Holley (:bholley)]

This sounds mostly related to gaia architecture. Let me know if there are any questions on how the underlying machinery is supposed to work.

Comment 6

[[:fabrice] Fabrice Desré]

In a "normal" script, loaded directly from an app, everything works as expected.

In add-on scripts loaded in a sandbox using the window as a global, we get the cross-origin error. I guess that the principal from the add-on script has the add-on origin indeed. I'll check that tomorrow (we end up http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp#43 and it's easy to print each principal's origin).

Comment 7

[Justin D'Arcangelo [:justindarc]]

Renaming since this has nothing to do with Blobs in particular. It seems that we are unable to pass any object via MozActivity from add-on code.

Comment 8

[Doug Sherk (:drs) (inactive)]

Fabrice, any updates on this?

Comment 9

[Justin D'Arcangelo [:justindarc]]

Attachment: TestAddonActivities.zip

fabrice: Here's a simple test add-on that adds a button to the Homescreen that launches an activity to take you to the Add-on Manager in the Settings app.

Comment 10

[[:fabrice] Fabrice Desré]

alright, so the error is that "app://verticalhome.gaiamobile.org doesn't subsume [Expanded Principal [app://verticalhome.gaiamobile.org]]".

The root cause is that nsPrincipal::Equals() fails because nsScriptSecurityManager::AppAttributesEqual() fails. The expanded principal has an appId of 0, which is usually what we get from the system principal.

Bobby, any idea why the expanded principal doesn't inherit the appId from the app://verticalhome.gaiamobile.org origin?

Comment 11

[[:fabrice] Fabrice Desré]

nvm, it's explicit in http://mxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp#929

Comment 12

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> alright, so the error is that "app://verticalhome.gaiamobile.org doesn't
> subsume [Expanded Principal [app://verticalhome.gaiamobile.org]]".
> 
> The root cause is that nsPrincipal::Equals() fails because
> nsScriptSecurityManager::AppAttributesEqual() fails.

Well, that's the first check that fails, but that's not really the point. nsExpandedPrincipal has an asymmetric security relationship with the principals in its whitelist. nsEP([foo]) subsumes foo, but not the reverse. That's the whole reason why we're using it for addons.

Comment 13

[[:fabrice] Fabrice Desré]

Right, that's what I understood by reading the code. So what can we do? This use case is totally safe, it's calling code that is accessible to any web page without any special privilege level.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #13)
> Right, that's what I understood by reading the code. So what can we do? This
> use case is totally safe, it's calling code that is accessible to any web
> page without any special privilege level.

Can you explain the use-case a little bit more? Are you trying to have the addon (with a semi-privileged principal) call arbitrary unprivileged code? That's exactly what Xrays are supposed to prevent you from doing. You can certainly waive Xrays, but if you need to do that it probably indicates that something isn't being designed right.

Comment 15

[[:fabrice] Fabrice Desré]

The setup is:
- We load a web page (from an app or not, that doesn't matter).
- We inject a script from an add-on in this page, using the window as the sandbox prototype (https://mxr.mozilla.org/mozilla-central/source/dom/apps/UserCustomizations.jsm#227)

The injected script then calls: var activity = new window.MozActivity({
      name: 'configure',
      data: {
        target: 'device',
        section: 'addons'
      }
    });
We create the activity object properly at https://mxr.mozilla.org/mozilla-central/source/dom/activities/Activity.cpp#36, create the JS proxy at https://mxr.mozilla.org/mozilla-central/source/dom/activities/ActivityProxy.js#39

If I replace `aOptions` at https://mxr.mozilla.org/mozilla-central/source/dom/activities/ActivityProxy.js#96 by a Cu.waiveXrays(aOptions) object, everything works. We are in chrome code, sending things over IPC so I feel this is not a problem, but I may be totally wrong. Note that if `data` is a primitive type, everything works as-is. Only objects/blobs/file have issues. For sure we don't want people to workaround with json or b64 encoded strings...

Comment 16

[Bobby Holley (:bholley)]

Ok, I see what's going on here.

The addon code (running with nsEP[contentOrigin]) invokes the MozActivity constructor with the object literal in comment 15. This constructor runs over Xrays, and the binding code converts the literal to an ActivityOptions dictionary (in the nsEP compartment). But since |data| is of type |any|, it remains an nsEP-privileged object.

The constructor lands us in Activity::Initialize: https://hg.mozilla.org/mozilla-central/annotate/4fb7ff694bf5/dom/activities/Activity.cpp#l36

The semantics of Xray calls over constructors mean that we coerce the argument to an ActivityOptions dictionary in the caller's compartment, but enter the content compartment before invoking the C++ constructor code - so we're on the content compartment at this point. Then, we rehydrate the dictionary back into a JSValue:

https://hg.mozilla.org/mozilla-central/annotate/4fb7ff694bf5/dom/activities/Activity.cpp#l71

So the dictionary ends up as a content object, but with a |data| property that to an object in the more-privileged nsEP compartment. The security wrappers protecting the chrome code in actProxy_startActivity rightly find this to be rather suspicious, since it looks like content is trying to pass an object that it can't touch into chrome code, and convince chrome to touch it. So the security wrappers here are very much protecting us from security bugs.

I think the most sensible thing to do is to enter the PrivilegedJunkScope before converting aOptions back to a JSVal. I'll attach a patch.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Rehydrate ActivityOptions in a system scope. v1

Justin, can you check whether this fixes the problem?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #17)
> Created attachment 8607736 [details] [diff] [review]
> Rehydrate ActivityOptions in a system scope. v1
> 
> Justin, can you check whether this fixes the problem?

(Or Fabrice, whoever gets to it first).

Comment 19

[[:fabrice] Fabrice Desré]

Great, that works! r=me

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8607736 [details] [diff] [review]
Rehydrate ActivityOptions in a system scope. v1

Ok, I'm going to run it by gabor as well, just to be safe.

Comment 21

[Pulsebot]

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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