Bug 858101 (CVE-2013-1697)

[[DefaultValue]] on XrayWrapper can call content-defined toString/valueOf methods

VERIFIED FIXED in Firefox 22, Firefox OS v1.1hd

Status

()

Core
Security
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

Tracking

({sec-high})

unspecified
mozilla24
x86
Windows XP
sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 wontfix, firefox22 verified, firefox23 verified, firefox24 verified, firefox-esr1722+ verified, b2g1822+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected, b2g-v1.1hd fixed)

Details

(Whiteboard: [adv-main22+][adv-esr1707+])

Attachments

(10 attachments, 2 obsolete attachments)

1002 bytes, text/html
Details
3.60 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.12 KB, patch
bholley
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
3.97 KB, patch
Details | Diff | Splinter Review
5.69 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.00 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.71 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.06 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.71 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.59 KB, patch
bholley
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
This is a regression between Firefox 6.0.2 and Firefox 7.  See also bug 720619.
(Reporter)

Comment 1

4 years ago
Created attachment 733366 [details]
testcase

Content-defined toString/valueOf methods are called on window and document.body, but are not called on document.
So, this only works for chrome accessing content (that is to say, non-SecurityWrappers), is that right?

We should fix this, but I don't think it's a huge deal. AFAICT it's just the result of the fact that XrayWrapper doesn't override defaultValue, so we end up in the defaultValue hook of the base wrapper. But that does a security check (which, incidentally, should probably be moved into SecurityWrapper), so it should only call the toString() of the underlying object if the wrapper subsumes the wrappee. There's a potential for chrome to get confused, but all in all it's probably not too dangerous. Marking sec-moderate.

Boris, what kind of semantics do we want for [[DefaultValue]] on DOM Xrays? Should we just run the DefaultValue algorithm on the wrapper and let the generics take care of it?
Keywords: sec-moderate
Flags: needinfo?(bzbarsky)
Once we figure out what Xray behavior we want here, I can write the patch.
Assignee: nobody → bobbyholley+bmo
I'm not sure what we want out of [[DefaultValue]] honestly, other than feeling that calling a content-defined defaultValue is not a good idea...  Ideally, it would do whatever [[DefaultValue]] on a vanilla DOM object does.
Flags: needinfo?(bzbarsky)
Created attachment 738177 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays. v1
Attachment #738177 - Flags: review?(mrbkap)
Created attachment 738178 [details] [diff] [review]
Tests. v1
Attachment #738178 - Flags: review?(mrbkap)
Comment on attachment 738177 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays. v1

Review of attachment 738177 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1809,5 @@
> +                                        JSType hint, MutableHandleValue vp)
> +{
> +    // Even if this isn't a security wrapper, Xray semantics dictate that we
> +    // run the DefaultValue algorithm directly on the Xray wrapper.
> +    return js::DefaultValue(cx, wrapper, hint, vp);

I'd also note there that we don't have to worry about the special defaultValue cases (such as Date) because we're guaranteed to only have "normally" behaving objects here.
Attachment #738177 - Flags: review?(mrbkap) → review+
Comment on attachment 738178 [details] [diff] [review]
Tests. v1

Review of attachment 738178 [details] [diff] [review]:
-----------------------------------------------------------------

Is it worth throwing Location into these tests in some way or another?

::: js/xpconnect/tests/chrome/test_bug858101.xul
@@ +40,5 @@
> +    // because it's still on the old XPConnect bindings, meaning it ends up
> +    // with an XPC_WN_Shared_Convert in its jsclass which gets invoked in
> +    // JSObject::defaultValue. This should be flipped when Document ends up
> +    // on the new bindings.
> +    ok(!!/hah/.exec(iwin.wrappedJSObject + ""), "Waivers should get content-defined window stringification");

I'd prefer to see the !! be != null.
Attachment #738178 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #8)
> Is it worth throwing Location into these tests in some way or another?

Well, valueOf and toString are both non-writable non-configurable |own| properties on the Location object, so the defaultValue behavior can't be changed, even in the content compartment (without Xrays). I was tempted to just verify that for godo measure, but it would the test unnecessarily complicated given the eval injection. So I'd say it's probably best to skip it, unless you have other ideas.

> ::: js/xpconnect/tests/chrome/test_bug858101.xul
> > +    ok(!!/hah/.exec(iwin.wrappedJSObject + ""), "Waivers should get content-defined window stringification");
> 
> I'd prefer to see the !! be != null.

Can I convince you otherwise? This is the pattern I've been using for the past year and and a half, so it's in dozens of tests already. I'd like to stay consistent if possible. :-)
https://tbpl.mozilla.org/?tree=Try&rev=290412914b10

Comment 11

4 years ago
Based on bug 860494 comment 17, I'm bumping this to sec-high.
Keywords: sec-moderate → sec-high

Comment 12

4 years ago
(Was something wrong about the sec-moderate analysis in comment 2?)
Can we lower the sec rating now that bug 860494 is fixed?
Probably? It would take me some time to grok the testcase in comment  11. And I'll hopefully get back to this soon, investigate the test failures, and just get it landed.
(Reporter)

Comment 15

4 years ago
I forgot to mention that the testcase in question abuses the session store code.  The XSS testcase in bug 860494 fools the session store code into injecting the attacker's page's content into the target page.
Created attachment 745450 [details] [diff] [review]
Fix tests. WIP

This is a WIP patch to fix the test failures caused by this patch. The issue
here is that implicit conversion of an XrayWrapper now runs the [[DefaultValue]]
algorith on the wrapper, rather than the underlying object, which means that we
wrap the toString in "[XrayWrapper ]" for historical reasons.

These fixes were relatively straightforward, but the two remaining failures:
devtools/webconsole/test/browser_console_variables_view.js
browser_console_variables_view_while_debugging.js

Are complex enough that I'm going to need some help by someone who understands
the devtools stuff.
Mihai, can you apply these patches and dig into the two tests in comment 16? This is a security bug, so it would be good to tackle this on the soon side.
Flags: needinfo?(mihai.sucan)
Created attachment 745451 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays. v2 r=mrbkap

Updated the patch to address review comments and rebase forward. Mihai, this
should be all you need.
Attachment #738177 - Attachment is obsolete: true
Attachment #745451 - Flags: review+
Created attachment 745522 [details] [diff] [review]
test fixes for the web console

The two failing tests check that objects and properties are correctly displayed in the web console variables view. They failed simply because the expected string changed.

This patch makes all tests pass with attachment 745451 [details] [diff] [review].

r=me
Flags: needinfo?(mihai.sucan)
Created attachment 745557 [details] [diff] [review]
Fix other tests. v1

Awesome, thanks Mihai! Flagging mrbkap for review on the other test changes.
Attachment #745450 - Attachment is obsolete: true
Attachment #745557 - Flags: review?(mrbkap)
https://tbpl.mozilla.org/?tree=Try&rev=0226c754e57f
This looks green, and can land pending sec-approval and mrbkap's review. I'm going to file for sec-approval now.
Comment on attachment 745451 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays. v2 r=mrbkap

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It's fairly obvious that XrayWrappers end up invoking defaultValue on the underlying JSObject instead of doing anything Xray-y. But this only affects chrome->content Xrays, so constructing an exploit would involve finding a place where chrome can be confused by implicitly converting a content object to a string in a way that it could be confused to do something bad. Finding something like that might be very hard.

FWIW, it's still not totally clear to me whether this is sec-high or sec-moderate.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No more than the test itself.

Which older supported branches are affected by this flaw?

All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backports should be trivial.

How likely is this patch to cause regressions; how much testing does it need?

It breaks some tests, and there's a reasonable possibility that it might break subtly break frontend code. Testing would be good.
Attachment #745451 - Flags: sec-approval?
This is getting sec-approval but only for checkin into m-c after 5/28, two weeks into the next cycle. At that point, please also make branch patches and nominate it for branches.
Whiteboard: [checkin after 5/28]
Attachment #745451 - Flags: sec-approval? → sec-approval+

Updated

4 years ago
Attachment #745557 - Flags: review?(mrbkap) → review+
(In reply to Mihai Sucan [:msucan] from comment #19)
> Created attachment 745522 [details] [diff] [review]
> test fixes for the web console
> 
> The two failing tests check that objects and properties are correctly
> displayed in the web console variables view. They failed simply because the
> expected string changed.

Why do these tests need to use .wrappedJSObject to access the content document off of the window now?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #25)
> (In reply to Mihai Sucan [:msucan] from comment #19)
> > Created attachment 745522 [details] [diff] [review]
> > test fixes for the web console
> > 
> > The two failing tests check that objects and properties are correctly
> > displayed in the web console variables view. They failed simply because the
> > expected string changed.
> 
> Why do these tests need to use .wrappedJSObject to access the content
> document off of the window now?

Because Xray toString behaves differently (it inserts an [object XrayWrapper ] around the underlying toString). Previously this code worked because DefaultValue (implicit string conversion) didn't go through Xray, but now it does.
I've made a note to land this at the appropriate date.

Note to self - the git branch name for these patches is |defaultvalue_xray|.
Flags: in-testsuite?
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/266b669dd70b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/12653403df73
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9e1b954e7f
Comment on attachment 745451 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays. v2 r=mrbkap

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Firefox 6 timeframe
User impact if declined: sec bugs
Testing completed: just landed to m-i
Risk to taking this patch (and alternatives if risky):  Medium risk. I had to change some browser-chrome tests to adapt to this change, so it could break extensions.
String or UUID changes made by this patch: None.
Attachment #745451 - Flags: approval-mozilla-esr17?
Attachment #745451 - Flags: approval-mozilla-beta?
Attachment #745451 - Flags: approval-mozilla-b2g18?
Attachment #745451 - Flags: approval-mozilla-aurora?
(In reply to Bobby Holley (:bholley) from comment #29)
> Risk to taking this patch (and alternatives if risky):  Medium risk. I had
> to change some browser-chrome tests to adapt to this change, so it could
> break extensions.

Can you go into more detail here? comment 23 didn't mention add-on risk, which makes this more concerning.

Updated

4 years ago
Attachment #745451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Alex Keybl [:akeybl] from comment #30)
> Can you go into more detail here? comment 23 didn't mention add-on risk,
> which makes this more concerning.

It did, IMO. Given that our legacy addon model is to inject addon code directly into the frontend, there's no meaningful distinction between frontend code and and addon code. Anything that might break frontend code might also break addons.

Anyway - the issue here is that we're changing the implict-toString behavior of Xray Wrappers. Previously it'd give you the toString of the underlying JS object, which is the security bug. Some of our tests were depending on this, and started failing.

However, the problems were always in the tests, not the frontend code itself. So that's a positive sign. I'd say we should probably land this, but I don't have a full picture of the other risks we're dealing with.
The tests were doing something very specific in relying on the toString output - I doubt this will have much impact for "real code" in add-ons (just like it didn't affect the front-end code itself).
https://hg.mozilla.org/mozilla-central/rev/266b669dd70b
https://hg.mozilla.org/mozilla-central/rev/12653403df73
https://hg.mozilla.org/mozilla-central/rev/7d9e1b954e7f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox24: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
https://hg.mozilla.org/releases/mozilla-aurora/rev/568a6e1cc1a8
https://hg.mozilla.org/releases/mozilla-aurora/rev/e63d4899efc0
https://hg.mozilla.org/releases/mozilla-aurora/rev/de4fa126dd9f
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
status-b2g-v1.1hd: --- → affected
status-firefox21: --- → wontfix
status-firefox22: --- → affected
status-firefox23: --- → fixed
status-firefox-esr17: --- → affected
Comment on attachment 745451 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays. v2 r=mrbkap

comment 31 and comment 32 lead us to believe this is manageable risk for b4, and a worthwhile fix for FF22.
Attachment #745451 - Flags: approval-mozilla-esr17?
Attachment #745451 - Flags: approval-mozilla-esr17+
Attachment #745451 - Flags: approval-mozilla-beta?
Attachment #745451 - Flags: approval-mozilla-beta+
Attachment #745451 - Flags: approval-mozilla-b2g18?
Attachment #745451 - Flags: approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-beta/rev/e3d25c40d9a0
https://hg.mozilla.org/releases/mozilla-beta/rev/eb782cc8015d

The web console tests fixed in patch 2 aren't on <Fx23, so that was skipped.
status-firefox22: affected → fixed
Whiteboard: [checkin after 5/28]
This (unsurprisingly) gets hairy for backporting to b2g18 and esr17. I would prefer Bobby to generate those patches rather than having me guess on the right things to do :)
Flags: needinfo?(bobbyholley+bmo)
Keywords: branch-patch-needed
Created attachment 758225 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays (on b2g18). v3 r=mrbkap
Attachment #758225 - Flags: review+
Created attachment 758227 [details] [diff] [review]
Fix other tests (on b2g18). v1 r=mrbkap
Attachment #758227 - Flags: review+
Created attachment 758246 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays (on esr17). v3 r=mrbkap
Attachment #758246 - Flags: review+
Created attachment 758247 [details] [diff] [review]
Fix other tests (on esr17). v1 r=mrbkap
Attachment #758247 - Flags: review+
Flags: needinfo?(bobbyholley+bmo)
Keywords: branch-patch-needed
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d1eafa349050
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b6a83f3f6215

https://hg.mozilla.org/releases/mozilla-esr17/rev/3290a0450c8e
https://hg.mozilla.org/releases/mozilla-esr17/rev/e8884fea6e4f
status-b2g18: affected → fixed
status-firefox-esr17: affected → fixed
And backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/67c807727762
https://hg.mozilla.org/releases/mozilla-esr17/rev/724fa6b27e3f
status-b2g18: fixed → affected
status-firefox-esr17: fixed → affected
Would have helped if I'd included the log links:
https://tbpl.mozilla.org/php/getParsedLog.php?id=23810531&tree=Mozilla-B2g18
https://tbpl.mozilla.org/php/getParsedLog.php?id=23810408&tree=Mozilla-Esr17
Oh, I see. On b2g18/esr17 the definition of DefaultValue ends up in a |namespace js {}| block, whereas it doesn't on trunk. And while clang (my local compiler) is fine with that, it looks like g++ doesn't like explicitly qualifying a namespace within that namespace.

So the solution should be to just strip the js:: prefix from the js::DefaultValue declaration in jsobj.cpp.
Created attachment 758673 [details] [diff] [review]
Followup fix for the lack of Cu.isXrayWrapper on esr17/b2g18. v1 r=me
Attachment #758673 - Flags: review+
Looks like that did the trick, thanks!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bdefa0dc9d83
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a9a3b28662b7

https://hg.mozilla.org/releases/mozilla-esr17/rev/9447b18044fc
https://hg.mozilla.org/releases/mozilla-esr17/rev/e2eecb449eeb
https://hg.mozilla.org/releases/mozilla-esr17/rev/00914aec47d1
status-b2g18: affected → fixed
status-firefox-esr17: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bdefa0dc9d83
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bdefa0dc9d83
status-b2g-v1.1hd: affected → fixed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #48)
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bdefa0dc9d83
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bdefa0dc9d83

And of course, that second link should have been:
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a9a3b28662b7
Some questions I had while verifying this bug, to be clear.

1. What exactly is expected behavior before and after the fix? That the page-defined toString output ("w s", "w v") should not be written to the page?

2. In recent builds of FF24 (some time after May 25), we no longer have Error Console but instead, an integrated Browser Console. When I follow this test's instructions to evaluate JS there, I get a JS error that "top.opener is null". What has changed here?
(In reply to Matt Wobensmith from comment #50)
> 2. In recent builds of FF24 (some time after May 25), we no longer have
> Error Console but instead, an integrated Browser Console. When I follow this
> test's instructions to evaluate JS there, I get a JS error that "top.opener
> is null". What has changed here?

Browser Console executes the code in the top level browser chrome window, while the error console executed the code in the error console window.

You can change |top.opener.content| to |content| and retry the eval. That *should* work.

Also, you can still open the Error Console from the devtools menu. Make sure that devtools.errorconsole.enabled is set to true in about:config (browser restart needed).
Thanks Mihai, that explains a lot! :)

I'll wait for more info on question #1 now.

Much appreciated.
(In reply to Matt Wobensmith from comment #50)
> Some questions I had while verifying this bug, to be clear.
> 
> 1. What exactly is expected behavior before and after the fix? That the
> page-defined toString output ("w s", "w v") should not be written to the
> page?

Privileged code should not see any content-defined stringification unless it explicitly waives Xray vision.
Thanks Bobby. 

So, we now expect to see XrayWrapper.toString output only, in this case. Is that correct?
(In reply to Matt Wobensmith from comment #54)
> Thanks Bobby. 
> 
> So, we now expect to see XrayWrapper.toString output only, in this case. Is
> that correct?

Not sure what you mean by XrayWrapper.toString, exactly. We expect to see the DOM behavior, un-modified, as if content hadn't touched it.
OK, thanks, I follow.

Confirmed original issue on several builds - 17.0.6esr and FF22/23/24.
Confirmed fixed on FF22 and FF23, 2013-06-14.
Confirmed fixed on FF24, 2013-06-13.
Confirmed fixed on 17esr 2013-06-18.
Status: RESOLVED → VERIFIED
status-firefox22: fixed → verified
status-firefox23: fixed → verified
status-firefox24: fixed → verified
status-firefox-esr17: fixed → verified
Whiteboard: [adv-main22+][adv-esr1707+]
Alias: CVE-2013-1697
tracking-b2g18: --- → 22+
tracking-firefox-esr17: --- → 22+
landed tests, which are green locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa45cf3568d3
and landed on central https://hg.mozilla.org/mozilla-central/rev/fa45cf3568d3
Flags: in-testsuite? → in-testsuite+
Group: core-security
You need to log in before you can comment on or make changes to this bug.