653926 - (CVE-2011-3004) loadSubScript is unwrapping XPCNativeWrapper scope parameter

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[David Rees]

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

Comment 1

[David Rees]

Attachment: simple html/js showing issue

Comment 2

[David Rees]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Blake, Andreas, what's going on here?

Comment 4

[Boris Zbarsky [:bzbarsky]]

And in particular, this seems like a worry security-wise to me.

Comment 5

[Christopher Blizzard (:blizzard)]

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.

Comment 6

[Daniel Veditz [:dveditz]]

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?

Comment 7

[Daniel Veditz [:dveditz]]

moz_bug_r_a4 has answered the question -- this is a potential security pitfall.

Comment 8

[Daniel Veditz [:dveditz]]

mrbkap: progress report?

Comment 9

[David Rees]

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.

Comment 11

[Johnny Stenback (:jst)]

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...

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

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).

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Comment 14

[Johnny Stenback (:jst)]

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.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/188ec2f3c847

Comment 17

[Johnny Stenback (:jst)]

Merged to m-c:

http://hg.mozilla.org/mozilla-central/rev/188ec2f3c847

Comment 18

[Johnny Stenback (:jst)]

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 19

[LegNeato]

Comment on attachment 546651 [details] [diff] [review]
Proposed fix v2

Approved for releases/mozilla-aurora. Please land asap

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(marking fixed per comment 17.)

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Note that this caused bug 675757. Do we still want to take it on Aurora?

Comment 22

[moz_bug_r_a4]

Without the fix, the addon in bug 675757 is exploitable. (bug 678924)

Comment 23

[LegNeato]

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?

Comment 24

[Jorge Villalobos [:jorgev] (he/him)]

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 25

[Johnny Stenback (:jst)]

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.

Comment 26

[Daniel Veditz [:dveditz]]

(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.

Comment 27

[moz_bug_r_a4]

Even on 1.9.2 branch, using a content object as the scope object is potentially
unsafe.  See bug 680880.

Comment 28

[LegNeato]

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 29

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-beta/rev/7b2999dd88b9

Comment 31

[u279076]

qa-: fix verification unnecessary

Comment 32

[Johnny Stenback (:jst)]

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.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For 1.9.2

This patch applies to the 1.9.2 branch. It fixes the testcase in bug 680880.

Comment 34

[LegNeato]

Comment on attachment 570350 [details] [diff] [review]
For 1.9.2

a=Legneato for 1.9.2.24. Please land asap on releases/mozilla-1.9.2 default.

Comment 35

[Mats Palmgren (inactive)]

philor says this broke the 1.9.2 tree.

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/releases/mozilla-1.9.2/rev/2d3d72d5868d
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/b5d60f395ce5

Comment 37

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719