Closed
Bug 636145
Opened 13 years ago
Closed 12 years ago
XPCOM "Components" object should not be available by default in module scopes
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: irakli, Assigned: ochameau)
References
Details
Attachments
(4 files, 4 obsolete files)
1.24 KB,
patch
|
Details | Diff | Splinter Review | |
1.10 KB,
text/javascript
|
Details | |
1.29 KB,
patch
|
Details | Diff | Splinter Review | |
1.50 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
This demonstrate a module that doesn't require("chrome") but still has access to components.*
Assignee | ||
Comment 2•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
Attachment #516273 -
Flags: feedback? → feedback?(warner-bugzilla)
Reporter | ||
Comment 3•13 years ago
|
||
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".
Assignee | ||
Comment 4•13 years ago
|
||
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)
Reporter | ||
Comment 5•13 years ago
|
||
Alex is right, I appeared to be too optimistic so please ignore my last comment.
Reporter | ||
Comment 7•13 years ago
|
||
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
Updated•13 years ago
|
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 1.0b5
Assignee | ||
Comment 8•13 years ago
|
||
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!
Comment 9•13 years ago
|
||
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).
Updated•13 years ago
|
Target Milestone: 1.0b5 → 1.0
Assignee | ||
Comment 10•13 years ago
|
||
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.
Comment 11•13 years ago
|
||
Alex: have you been able to make any progress with this bug?
Updated•13 years ago
|
Whiteboard: [triage:followup]
Assignee | ||
Comment 12•13 years ago
|
||
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)
Comment 13•13 years ago
|
||
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
Assignee | ||
Comment 14•13 years ago
|
||
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
Comment 15•13 years ago
|
||
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]
Comment 17•13 years ago
|
||
(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
Comment 18•13 years ago
|
||
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)
Comment 19•13 years ago
|
||
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?
Assignee | ||
Comment 20•13 years ago
|
||
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)
Comment 21•13 years ago
|
||
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).
Assignee | ||
Comment 22•13 years ago
|
||
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).
Assignee | ||
Comment 23•13 years ago
|
||
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)
Comment 24•13 years ago
|
||
(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 26•13 years ago
|
||
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)
Updated•13 years ago
|
Assignee | ||
Comment 27•12 years ago
|
||
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.
Assignee | ||
Comment 28•12 years ago
|
||
As agreed in bug 774636, we will keep the fix mentioned in comment 27.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•