Last Comment Bug 653926 - (CVE-2011-3004) loadSubScript is unwrapping XPCNativeWrapper scope parameter
: loadSubScript is unwrapping XPCNativeWrapper scope parameter
: regression, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Windows XP
-- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Andrew Overholt [:overholt]
Depends on: 675757 703017
Blocks: 657351 678924 CVE-2011-3647
  Show dependency treegraph
Reported: 2011-04-30 08:47 PDT by David Rees
Modified: 2014-06-25 13:02 PDT (History)
23 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

simple html/js showing issue (1.74 KB, application/java-archive)
2011-04-30 08:49 PDT, David Rees
no flags Details
Proposed fix (2.23 KB, patch)
2011-07-13 17:54 PDT, Blake Kaplan (:mrbkap)
jst: review+
Details | Diff | Splinter Review
Proposed fix v2 (5.26 KB, patch)
2011-07-18 15:41 PDT, Blake Kaplan (:mrbkap)
jst: review+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review
For 1.9.2 (4.41 KB, patch)
2011-10-28 13:49 PDT, Blake Kaplan (:mrbkap)
jst: review+
christian: approval1.9.2.24+
Details | Diff | Splinter Review

Description User image David Rees 2011-04-30 08:47:17 PDT
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[";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):

Reproducible: Always
Comment 1 User image David Rees 2011-04-30 08:49:54 PDT
Created attachment 529295 [details]
simple html/js showing issue
Comment 2 User image David Rees 2011-04-30 09:05:44 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2011-05-03 21:41:35 PDT
Blake, Andreas, what's going on here?
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2011-05-05 19:33:26 PDT
And in particular, this seems like a worry security-wise to me.
Comment 5 User image Christopher Blizzard (:blizzard) 2011-05-10 15:03:28 PDT
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 User image Daniel Veditz [:dveditz] 2011-05-12 13:26:07 PDT
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 User image Daniel Veditz [:dveditz] 2011-05-25 11:56:36 PDT
moz_bug_r_a4 has answered the question -- this is a potential security pitfall.
Comment 8 User image Daniel Veditz [:dveditz] 2011-06-02 13:46:48 PDT
mrbkap: progress report?
Comment 9 User image David Rees 2011-06-07 13:26:10 PDT
FYI, I mentioned this issue in a mozillazine thread at 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 User image Johnny Stenback (:jst, 2011-07-07 13:38:29 PDT
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 User image Blake Kaplan (:mrbkap) 2011-07-13 17:54:05 PDT
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 User image Blake Kaplan (:mrbkap) 2011-07-13 17:54:39 PDT
Created attachment 545794 [details] [diff] [review]
Proposed fix
Comment 14 User image Johnny Stenback (:jst, 2011-07-14 13:31:24 PDT
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 User image Blake Kaplan (:mrbkap) 2011-07-18 15:41:18 PDT
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.
Comment 17 User image Johnny Stenback (:jst, 2011-07-25 09:23:18 PDT
Merged to m-c:
Comment 18 User image Johnny Stenback (:jst, 2011-07-25 09:26:20 PDT
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 User image christian 2011-07-26 14:41:55 PDT
Comment on attachment 546651 [details] [diff] [review]
Proposed fix v2

Approved for releases/mozilla-aurora. Please land asap
Comment 20 User image Blake Kaplan (:mrbkap) 2011-07-26 17:01:02 PDT
(marking fixed per comment 17.)
Comment 21 User image Blake Kaplan (:mrbkap) 2011-08-11 14:48:20 PDT
Note that this caused bug 675757. Do we still want to take it on Aurora?
Comment 22 User image moz_bug_r_a4 2011-08-14 18:54:51 PDT
Without the fix, the addon in bug 675757 is exploitable. (bug 678924)
Comment 23 User image christian 2011-08-16 00:17:16 PDT
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 User image Jorge Villalobos [:jorgev] 2011-08-18 08:31:35 PDT
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 User image Johnny Stenback (:jst, 2011-08-18 13:27:50 PDT
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 User image Daniel Veditz [:dveditz] 2011-08-18 13:28:23 PDT
(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 User image moz_bug_r_a4 2011-08-22 06:27:45 PDT
Even on 1.9.2 branch, using a content object as the scope object is potentially
unsafe.  See bug 680880.
Comment 28 User image christian 2011-08-22 14:32:46 PDT
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 User image Blake Kaplan (:mrbkap) 2011-08-22 15:03:23 PDT
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 User image Blake Kaplan (:mrbkap) 2011-08-22 19:04:42 PDT
Comment 31 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 13:37:15 PDT
qa-: fix verification unnecessary
Comment 32 User image Johnny Stenback (:jst, 2011-10-27 21:55:55 PDT
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 User image Blake Kaplan (:mrbkap) 2011-10-28 13:49:01 PDT
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 34 User image christian 2011-10-28 15:46:54 PDT
Comment on attachment 570350 [details] [diff] [review]
For 1.9.2

a=Legneato for Please land asap on releases/mozilla-1.9.2 default.
Comment 35 User image Mats Palmgren (:mats) 2011-10-29 20:10:42 PDT
philor says this broke the 1.9.2 tree.
Comment 37 User image Raymond Forbes[:rforbes] 2013-07-19 18:39:18 PDT

Note You need to log in before you can comment on or make changes to this bug.