1.74 KB, application/java-archive
2.23 KB, patch
|Details | Diff | Splinter Review|
5.26 KB, patch
|Details | Diff | Splinter Review|
4.41 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 mozIJSSubScriptLoader.loadSubScript’s behavior with XPCNativeWrapper scope parameters seems to have changed from 3.6.* to 4. Specifically in 4 if scope is a XPCNativeWrapper then it is unwrapping it. I have attached a simple HTML and JS file that demonstrates this, but the basic lines are pretty clear: The calling code is: Components.classes["@mozilla.org/moz/jssubscript-loader;1"] .getService(Components.interfaces.mozIJSSubScriptLoader).loadSubScript("chrome://exampleext/content/firefox4loadsubscriptchange.min.js",xpcnWrappedWindow); And the subscript code is simply: var windowTS = window.toString(); In 3.6.16 windowTS will be “[object XPCNativeWrapper [object Window]]”. In 4.0 it will be “Window”. I also have a slightly more elaborate case that verifies in the calling code that anything that is done to the window object is added to the wrapper in 3.6, but added directly to the unwrapped window in 4.0. The seems like a regression to me since loadSubScript is privileged itself so I don't see a reason for hiding the wrapped object from it. Also mentioned in this thread (but no replies): http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/6ac127f89432fb97/094bb532b6ca8fd7#094bb532b6ca8fd7 Reproducible: Always
I'm not sure, but this might be a security issue also. While before the underlying code was handed a wrapper, its now handed the underlying Window allowing that Window's code to possibly catch things like expando sets and then inject its own code into the privileged call.
Blake, Andreas, what's going on here?
And in particular, this seems like a worry security-wise to me.
Marking as security-sensitive until it's evaluated by the security team. If they think it's an issue, they will mark it to track for Firefox 5.
Blake: do XRayWrappers save us here? Looks like the most likely attack scenario would be a web page hacking an extension doing something with it. moz_bug_r_a4: any fun exploits hiding in here?
moz_bug_r_a4 has answered the question -- this is a potential security pitfall.
mrbkap: progress report?
FYI, I mentioned this issue in a mozillazine thread at http://forums.mozillazine.org/viewtopic.php?f=19&t=2105087&p=10752563#p10752563 before it was marked security-sensitive. I have now deleted that content from the thread, but its probably still cached in a few places.
Blake, can you write up the patch that you have planned here, we'd want this for 6 if possible, and that's already in beta...
Here's the deal. Before compartments, XrayWrappers (then called XPCNativeWrappers) were native objects. If you passed one of these objects into JS_EvaluateUCScriptForPrincipals, things would mostly "just work" (though any local variables or the like would be set on the wrapper instead of the underlying object). When compartments were introduced, we moved XrayWrappers to being proxies. This means that they can no longer be the scope object for any calls to Evaluate. In order to get around this restriction, we started unwrapping XrayWrappers. I can't find any clean fixes for this bug. One option would be to create yet another native wrapper object to wrap the Xray wrapper in. However, that would lead to yet more inconsistent behavior, as variables/property sets would end up on that object (and not even on the Xray wrapper). So, this patch makes us not compile chrome code if we unwrapped a security wrapper. This has the potential to break some extensions, as the subscript will no longer be chrome. David, if you want the old behavior, I'd suggest using a chrome sandbox object (which is essentially option 1 here).
Created attachment 545794 [details] [diff] [review] Proposed fix
We won't be taking this for 6 given the extension compat issue, but we should get this in on m-c asap, and after that land it in aurora once we feel comfortable doing that.
Created attachment 546651 [details] [diff] [review] Proposed fix v2 I had to merge the patch to trunk and the merge wasn't trivial. The main change is making sure we don't use the cache if we're not compiling as chrome.
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/188ec2f3c847
Comment on attachment 546651 [details] [diff] [review] Proposed fix v2 Nominating for 7 aurora. This is a critical security bug, which was filed as a public bug with a testcase and only closed later on. There's a small change in functionality here that might trigger some extension compat issues, but it's a pretty narrow change, so I'd like to see this in aurora, but not in beta any more...
Comment on attachment 546651 [details] [diff] [review] Proposed fix v2 Approved for releases/mozilla-aurora. Please land asap
(marking fixed per comment 17.)
Note that this caused bug 675757. Do we still want to take it on Aurora?
Hmmm, not sure, probably yes. Can you reply the add-on author's comment in bug 675757? Can we get the security group to weigh in on this vs potential add-on compat? Can we have the add-on team scan for this?
The developer has already produced a version of the add-on that he claims is not vulnerable to the exploit. Since it appears that this only affects add-ons using unorthodox practices when communicating between chrome and content, I am in favor of pushing this to Aurora. I wouldn't expect more than a handful of add-ons to be affected. I don't know what I could look for in the add-ons MXR to detect possibly affected add-ons. Just looking for loadSubScript would produce too many results.
Comment on attachment 546651 [details] [diff] [review] Proposed fix v2 Minusing for aurora since this is already fixed on current aurora, but the patch didn't land before 7 was uplifted to beta. Given that the extension is fixed, I think we should take this fix on beta.
(In reply to Christian Legnitto [:LegNeato] from comment #23) > Can we get the security group to weigh in on this vs potential > add-on compat? Can we have the add-on team scan for this? We need to take this fix and damn any compat worries. It really should be a rare strange edge case anyway.
Even on 1.9.2 branch, using a content object as the scope object is potentially unsafe. See bug 680880.
Comment on attachment 546651 [details] [diff] [review] Proposed fix v2 We're worried about add-on compat but think we should take this. Approved for beta
Comment on attachment 545794 [details] [diff] [review] Proposed fix As a note: this is the patch that's going to land on beta. The only reason this patch didn't land was because it conflicted with bug 648125, which never landed on beta. Hopefully this shouldn't be too controversial.
qa-: fix verification unnecessary
For 1.9.2 we'll need an updated patch, I made a quick attempt at just applying the trunk fix here to 1.9.2 but even after getting it to compile and use what I though was the branch equivalent of the stuff that's different on trunk, the problem remained. Blake says he can work on porting this tomorrow morning.
Created attachment 570350 [details] [diff] [review] For 1.9.2 This patch applies to the 1.9.2 branch. It fixes the testcase in bug 680880.
Comment on attachment 570350 [details] [diff] [review] For 1.9.2 a=Legneato for 18.104.22.168. Please land asap on releases/mozilla-1.9.2 default.
philor says this broke the 1.9.2 tree.