The default bug view has changed. See this FAQ.
Bug 653926 (CVE-2011-3004)

loadSubScript is unwrapping XPCNativeWrapper scope parameter

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: David Rees, Assigned: mrbkap)

Tracking

({regression, testcase})

unspecified
x86
Windows XP
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox5- wontfix, firefox6- wontfix, firefox7+ fixed, firefox8+ fixed, status2.0 wanted, status1.9.2 .24-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical][qa-])

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 529295 [details]
simple html/js showing issue
(Reporter)

Comment 2

6 years ago
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?
Assignee: general → nobody
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → XPConnect
Ever confirmed: true
Keywords: regression
QA Contact: general → xpconnect
And in particular, this seems like a worry security-wise to me.
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
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.
Group: core-security
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?
Assignee: nobody → mrbkap
Attachment #529295 - Attachment mime type: application/octet-stream → application/java-archive
status-firefox5: --- → affected
tracking-firefox5: ? → -
Whiteboard: [need answer to comment 6 from mrbkap]
Blocks: 657351
moz_bug_r_a4 has answered the question -- this is a potential security pitfall.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: --- → wanted
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox7: --- → ?
Whiteboard: [need answer to comment 6 from mrbkap] → [sg:critical]

Updated

6 years ago
tracking-firefox6: ? → +
tracking-firefox7: ? → +
mrbkap: progress report?
(Reporter)

Comment 9

6 years ago
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.

Updated

6 years ago
status-firefox5: affected → wontfix
Keywords: testcase
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...
(Assignee)

Comment 12

6 years ago
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).
(Assignee)

Comment 13

6 years ago
Created attachment 545794 [details] [diff] [review]
Proposed fix
Attachment #545794 - Flags: review?(jst)

Updated

6 years ago
Attachment #545794 - Flags: review?(jst) → review+
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.
status-firefox6: affected → wontfix
tracking-firefox6: + → -
(Assignee)

Comment 15

6 years ago
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.
Attachment #545794 - Attachment is obsolete: true
Attachment #546651 - Flags: review?(jst)

Updated

6 years ago
Attachment #546651 - Flags: review?(jst) → review+
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/188ec2f3c847
Whiteboard: [sg:critical] → [sg:critical][inbound]
Merged to m-c:

http://hg.mozilla.org/mozilla-central/rev/188ec2f3c847
status-firefox8: --- → fixed
tracking-firefox8: --- → +
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...
Attachment #546651 - Flags: approval-mozilla-aurora?

Comment 19

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

Approved for releases/mozilla-aurora. Please land asap
Attachment #546651 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 20

6 years ago
(marking fixed per comment 17.)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical][inbound] → [sg:critical]
Whiteboard: [sg:critical] → [sg:critical][needs aurora landing ASAP]
(Assignee)

Updated

6 years ago
Depends on: 675757
(Assignee)

Comment 21

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

Comment 22

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

Comment 23

6 years ago
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?
Blocks: 678924
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.
Attachment #546651 - Flags: approval-mozilla-aurora+ → approval-mozilla-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.

Comment 27

6 years ago
Even on 1.9.2 branch, using a content object as the scope object is potentially
unsafe.  See bug 680880.
Whiteboard: [sg:critical][needs aurora landing ASAP] → [sg:critical][needs beta approval ASAP]

Comment 28

6 years ago
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
Attachment #546651 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 29

6 years ago
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.
Attachment #545794 - Attachment is obsolete: false
(Assignee)

Comment 30

6 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/7b2999dd88b9
status-firefox7: affected → fixed
qa-: fix verification unnecessary
Whiteboard: [sg:critical][needs beta approval ASAP] → [sg:critical][needs beta approval ASAP][qa-]
Alias: CVE-2011-3004
Whiteboard: [sg:critical][needs beta approval ASAP][qa-] → [sg:critical][qa-]
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.
(Assignee)

Comment 33

6 years ago
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.
Attachment #570350 - Flags: review?(jst)
(Assignee)

Updated

6 years ago
Attachment #570350 - Flags: approval1.9.2.24?

Updated

6 years ago
Attachment #570350 - Flags: review?(jst) → review+

Comment 34

6 years ago
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.
Attachment #570350 - Flags: approval1.9.2.24? → approval1.9.2.24+
philor says this broke the 1.9.2 tree.
(Assignee)

Comment 36

6 years ago
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/2d3d72d5868d
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/b5d60f395ce5
status1.9.2: unaffected → .24-fixed
Blocks: 680880
Depends on: 703017
Group: core-security
Comment 27 is private: false
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.