Closed Bug 636145 Opened 11 years ago Closed 9 years ago

XPCOM "Components" object should not be available by default in module scopes

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: irakli, Assigned: ochameau)

References

Details

Attachments

(4 files, 4 obsolete files)

We try to restrict raw access to the XPCOM using pre-regexping module source. This is approach does not really restrict access to XPCOM and `Components` can be accessed reg-exper will not catch it (for example Ss = eval("Components")).

Also in reality what we need is to have a "chrome" module that will contain all the capabilities, which can be easily achieved by creating module sandboxes with non-system privileges. My proposal would be to do this.
This demonstrate a module that doesn't require("chrome") but still has access to components.*
This patch simply add a new method securityPolicy.getPrincipal in cuddlefish that check for packaging options "needsChrome" and return either "system" or "resource://jetpack-$(jetpackID)".

Then I just added a call to this function from securable-module to setup this custom principal during Sanbox creation.

The main issue with this patch is that needsChrome attribute is not correctly set so that some module use require("chrome") but needsChrome is still set to false (for ex: timer module). So it break a lot of stuff "as is".
Attachment #516273 - Flags: feedback?
Attachment #516273 - Flags: feedback? → feedback?(warner-bugzilla)
Alex I think your patch adds unnecessary complexity what I was suggesting is more along this lines:

Bootstrap-er creates "chrome" module that may be loaded by any / needsChrome modules.
https://github.com/Gozala/dolphin/blob/master/app/extensions/dolphin%40mozilla.com/bootstrap.js#L96-104

All other modules use non privileged sandboxes:
https://github.com/Gozala/dolphin/blob/master/app/extensions/dolphin%40mozilla.com/bootstrap.js#L196-198

This way Components is accessible only to the modules that can require "chrome".
This solution seems nice without taking XPConnect layer into account.
This layer block access to any xpcom access from any sandbox that has not "privileged" principal, like system.

Here is a minimal example:
var sandbox = Components.utils.Sandbox("module:foo");
sandbox.Cc = Components.classes;
sandbox.Ci = Components.interfaces;
Components.utils.evalInSandbox('Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService)', sandbox, '1.8', 'file', 1);

That ends up with the following error:
Permission denied to <module:foo> to call method UnnamedClass.getService

So this is a false good idea and I think that current solution is fine because as soon as a module use require("chrome") he has superpower so it doesn't make much sense to block at all cost any super user access.
I see require("chrome") as a "su" equivalent. But we know that a middle way exists, called "sudo". With fine super power access setup, but it's clearly another feature :)

Finally, I've updated the patch in order to use "module:$(module-name)" URI as principal, like Irakli is doing in Dolphin project.
Attachment #516273 - Attachment is obsolete: true
Attachment #516273 - Flags: feedback?(warner-bugzilla)
Attachment #516693 - Flags: feedback?(warner-bugzilla)
Alex is right, I appeared to be too optimistic so please ignore my last comment.
Duplicate of this bug: 561882
Comment for bug triage: I would love to see it for 1.0. Also assigning this to alex as he submitted a patch already, but feel free to reassign.
Assignee: nobody → poirot.alex
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 1.0b5
We should postpone this work after Brian branch has landed,
as I need to fix require detection in cfx or may be Brian already fixed that!
Hm, yeah. In my bug 627607 patch, I got rid of the "Security Manager" altogether. There are already hard-coded checks for use of "chrome" in the code which decides when to allow a runtime search, and we could just add the system-principal to that same if() statement.

I don't know enough about how principals work here: if a module that does require("chrome") is built in a sandbox that is configured with the system principal, can code in other modules (built with a non-system principal sandbox) still invoke it? The goal is for modules to be able to invoke each other but not see inside their private attributes (which requires some conventions to explain what is private and what is part of the API, e.g. require("self").data should work).
Target Milestone: 1.0b5 → 1.0
This unit test tends to confirm that we can block modules, that doesn't require chrome, to access Components.classes (i.e. xpcoms) without blocking them to call privileaged sandboxes.

That being said I try to reimplement such feature but `requirements` doesn't seems to be computed during unit test execution ?
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/securable-module.js#L229
`reqs` is always empty, so I can't do ("chrome" in reqs) in order to change sandbox principal.
Alex: have you been able to make any progress with this bug?
Whiteboard: [triage:followup]
Oops I forgot to update bug item. I warned during the last meeting that this enhancement may be missed for 1.0 as it may depend on linker work.

I'm now able to apply my patch again, linker work has been improved so I'm able to get necessary information in tests. But I still have some issues:
  Error: Permission denied to access property 'compose'

I may have missed some privileage usecases in attachment 531356 [details].
I'm lowering priority of this bug as it is uncertain that we can actually have  non-prvileaged principals and it will not break addons if we manage to implement it later. (It is going to break only malicious codes)
Retargeting per comment 12 and discussion in yesterday's weekly project status meeting that this is not going to make 1.0 and doesn't have to.
Whiteboard: [triage:followup]
Target Milestone: 1.0 → Future
I took some time to dig this subject and there is multiple issues due to automatic wrapping between two different principals.

1/ Functions from system principals given to lower privilege scripts 
doesn't have apply/call methods:
Here the simpliest test you may run from a JS console:
var doc = top.opener.gBrowser.contentDocument;
doc.wrappedJSObject.t = top.opener.openFeedbackPage;
doc.wrappedJSObject.defaultView.eval("document.t.apply(null, [])");

Whereas following script works perfectly:
var doc = top.opener.gBrowser.contentDocument;
doc.wrappedJSObject.t  = top.opener.openFeedbackPage;
doc.wrappedJSObject.defaultView.eval("document.t()");

So the issues is not "not enough privileges to call chrome function",
but `apply` method just do not exists on a function!

In jetpack, api-utils::publicConstructor:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/api-utils.js#L69
`privateCtor.apply` is null when we call `publicConstructor` from module that require("chrome")


2/ instanceof is broken over sandboxes. This is a well known issue, but we are not facing the usual errors. Instead we have some wrapping issues.

a) when you create an object with constructor being defined on another sandbox
and you send this instance back the the sandbox that contain the constructor
definition, instanceof will return false!
Test case:
function MyClass(){};
var doc = top.opener.gBrowser.contentDocument;
doc.wrappedJSObject.inst = new MyClass();
[doc.wrappedJSObject.defaultView.eval("document.inst") instanceof MyClass,
 new MyClass() instanceof MyClass];

b) from the other sandbox, constructor.prototype is undefined.
Test case:
function MyClass(){};
var doc = top.opener.gBrowser.contentDocument;
doc.wrappedJSObject.inst = new MyClass();
[doc.wrappedJSObject.defaultView.eval("document.inst.constructor.prototype"),
 new MyClass().constructor.prototype]

In jetpack, api-utils::hiddenframe use instanceof and fails on it:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/hidden-frame.js#L133
Based on my level of understanding, when you pass an object from a chrome sandbox to a none chrome sandbox, the object will be wrapped into a cross origin wrapper and an xraywrapper. And when you try to call a method on the object, pretty much only the original native methods will work (this protects against overwriting methods in a dangerous way in js) 

From what I understood here about the original problem is the availability of the components in the module, and the wrappers here are not really needed, so my question is have you guys tried to turn off xraywrappers for this sandbox (wantXrays flag)? 

I assume the solution won't be this simple. And I heard a couple of time that for addons a special sandbox implementation would be needed. It is not realyl clear for me why yet, so what I think would be the best, that first let's collect the requested features we need from that sandbox (or addonbox). Here or into a new bug.
Assuming we can get a platform fix for this in for Fx9, opportunistically targeting SDK 1.4.
Depends on: 677294
Summary: By default chrome capabilities in modules. → XPCOM "Components" object should not be available by default in module scopes
Whiteboard: [milestone:1.4]
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> From what I understood here about the original problem is the availability
> of the components in the module, and the wrappers here are not really
> needed, so my question is have you guys tried to turn off xraywrappers for
> this sandbox (wantXrays flag)?

I updated Alex's patch to do this (and apply on tip, although one of the changes I needed to make is a kludge that may want fixing in any final patch), but I still see the kinds of problems Alex describes:

(addon-sdk)mykbook:api-utils myk$ cfx test -f hidden-frame --binary /Applications/Nightly.app/
Using binary at '/Applications/Nightly.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/bN/bN4Z5w6UEdC60cFgLX32NU+++TI/-Tmp-/tmp4WmfIp.mozrunner'.
Running tests on Firefox 8.0a1/Gecko 8.0a1 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under Darwin/x86_64-gcc3.
error: TEST FAILED: test-hidden-frame.testFrame (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://api-utils-api-utils-lib/api-utils.js", line 71, in PublicCtor
TypeError: privateCtor.apply is not a function
console: [JavaScript Error: "TEST FAILED: test-hidden-frame.testFrame (exception)
"]
console: [JavaScript Error: "An exception occurred.
Traceback (most recent call last):
  File "resource://api-utils-api-utils-lib/api-utils.js", line 71, in PublicCtor
TypeError: privateCtor.apply is not a function
"]
^CTraceback (most recent call last):
  File "/Users/myk/addon-sdk/bin/cfx", line 29, in <module>
    cuddlefish.run()
  File "/Users/myk/addon-sdk/python-lib/cuddlefish/__init__.py", line 790, in run
    norun=options.no_run)
  File "/Users/myk/addon-sdk/python-lib/cuddlefish/runner.py", line 383, in run_app
    time.sleep(0.05)

> I assume the solution won't be this simple. And I heard a couple of time
> that for addons a special sandbox implementation would be needed. It is not
> realyl clear for me why yet, so what I think would be the best, that first
> let's collect the requested features we need from that sandbox (or
> addonbox). Here or into a new bug.

The only two issues I know of are this one about removing the Components global (or at least access to it) and bug 672443 about reducing the number of compartments that addons use (for which we filed platform bug 677294 about creating JavaScript scopes that share compartments).

So let's focus on just these two issues.  We can always tackle additional issues as we run across them. (However, if you know of other current issues, then let us know!  If there are three or more of them, then we might create a meta-bug to track them, especially if it looks like the work to resolve them all will take place in a single patch.)
Whiteboard: [milestone:1.4]
Target Milestone: Future → 1.4
In the AccessCheck.cpp ExposedPropertiesOnly::check function, I think we can follow the same practice with Function.apply / call than for Array.length. This seem to fix Alex's example.
Attachment #554355 - Flags: review?(mrbkap)
I'm not sure if this fix can be exploited or not, to inject a function into a chrome context and execute there or not, I will write some test for this later.

Also is the prototype / instanceof bugs important to fix for jetpack? And are those security restrictions or just not supported features?
Prototype/instanceof bugs are as important as apply/call bug.
If we want to solve this issue by using non-system principal, we have to fix all these bugs. And if you have a quick'n dirty patch, it would allow us to check if there is other kind of issues that would disallow us to use non-system principal.

Having said that, if we figure out that these errors can't be solved without exposing new security issues, we may wait for you to be more confident with wrappers. In the meantime, we can just remove `Components` variable from sandboxes that doesn't require chrome. (We still may have access to it if the module get a reference of a priviledge object, like require("window-utils").activeBrowserWindow.Components)
Instanceof problem is a consequence of the prototype problem. It can be solved the same way as the call/apply, the question is again, if it's security issue or not... So I wait some response for this from some experts in this area, why these things were disabled in the first place. For prototype I'm afraid it's too easy to fetch a reference to the global Object this way, and that's why it is blocked. If so, maybe we could use __exposedProps__ from javascript, or set some special flag on scriptSecurityManager for addons (sandboxes with name).
I runned tests again with a patched build provided by Gabor, with quick fixes that address issues mentioned in comment 14.
The result is that we are hitting another issue.
With two modules with the same principal, each module can access properties of objects shared between them.

Here is a scratchpad test script:

  let Cu = Components.utils;

  let s1 = Cu.Sandbox("module:foo", {wantXrays: false});
  Cu.evalInSandbox("var o = {};", s1);
  let s2 = Cu.Sandbox("module:foo", {wantXrays: false});
  Cu.evalInSandbox("function test(o) {o.foo();};", s2);

  s2.test(s1.o);
  // -> Error: Permission denied to access property 'foo'

It fails whatever the `wantXrays` value is, and it fails if we set the same `sandboxName` too (that would allow to share same compartment between modules).
Gabor was able to figure out the issue behind comment 22 and filled bug 681648.
So here is a new patch that:
- use "http://foo.com" as principal of all module that doesn't use require(chrome)
- set wantXrays to true

Then, if I use the custom firefox build with platform fixes from gabor, 
I'm able to pass all api-utils/addon-kit tests.
I'm still hitting two issues:
- crashes on reading-data and reddit-panel tests, but I think that gabor release build contain some known crashes
- exception: "TypeError: property.value.bind is not a function". I suppose that this is the same than `apply`.
Attachment #516693 - Attachment is obsolete: true
Attachment #536069 - Attachment is obsolete: true
Attachment #553952 - Attachment is obsolete: true
Attachment #516693 - Flags: feedback?(warner-bugzilla)
(In reply to Alexandre Poirot (:alex) from comment #23)
> - use "http://foo.com" 

There were suggestions in the mentioned bug that we should use principals instead of URIs, see the bug for more details.

> - set wantXrays to true

It's true by default so you can just remove it.

> - crashes on reading-data and reddit-panel tests, but I think that gabor
> release build contain some known crashes

The binary I gave to you should crash when the second sandbox is crated with the same sandboxName. If there is any other crash please let me, know (on irc or here)

> - exception: "TypeError: property.value.bind is not a function". I suppose
> that this is the same than `apply`.

Yes, it's exactly the same story.
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.4 → ---
Comment on attachment 554355 [details] [diff] [review]
fix for the apply undefined issue

I really don't like this approach. We need to do something better here.
Attachment #554355 - Flags: review?(mrbkap)
Depends on: 697775
Blocks: 697775
No longer depends on: 697775
Fixed by following revision:
https://github.com/mozilla/addon-sdk/commit/b167ac2bf0b777bcf5c13ca5735e92d3d8bb666c
From Pull request 448:
https://github.com/mozilla/addon-sdk/pull/448
Related to bug 756548.

But this change is subject to discussion through bug 774636.
Depends on: 747434
As agreed in bug 774636, we will keep the fix mentioned in comment 27.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.