Closed Bug 1161748 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+, firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S13 (29may)
blocking-b2g 2.5+
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- fixed
b2g-master --- fixed

People

(Reporter: justindarc, Assigned: bholley)

References

Details

(Whiteboard: [spark])

Attachments

(2 files)

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.
Blocks: 1159376
Priority: -- → P1
Whiteboard: [spark]
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
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?
(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.
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: '' }
    });
This sounds mostly related to gaia architecture. Let me know if there are any questions on how the underlying machinery is supposed to work.
Flags: needinfo?(fabrice)
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).
Flags: needinfo?(fabrice)
Blocks: 1144843
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.
Summary: [Add-ons] Unable to pass Blob in MozActivity from add-on code → [Add-ons] Unable to pass objects in MozActivity from add-on code
Blocks: spark-addons
blocking-b2g: --- → spark+
Fabrice, any updates on this?
Flags: needinfo?(fabrice)
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.
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?
Flags: needinfo?(fabrice)
(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.
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.
(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.
Flags: needinfo?(fabrice)
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...
Flags: needinfo?(fabrice)
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.
Justin, can you check whether this fixes the problem?
Attachment #8607736 - Flags: feedback?(jdarcangelo)
(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).
Great, that works! r=me
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.
Attachment #8607736 - Flags: feedback?(jdarcangelo) → review?(gkrizsanits)
Attachment #8607736 - Flags: review?(gkrizsanits) → review+
https://hg.mozilla.org/mozilla-central/rev/b79c6a84b645
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S13 (29may)
blocking-b2g: spark+ → 2.5+
You need to log in before you can comment on or make changes to this bug.