Last Comment Bug 858101 - (CVE-2013-1697) [[DefaultValue]] on XrayWrapper can call content-defined toString/valueOf methods
(CVE-2013-1697)
: [[DefaultValue]] on XrayWrapper can call content-defined toString/valueOf met...
Status: VERIFIED FIXED
[adv-main22+][adv-esr1707+]
: sec-high
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla24
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-04 08:50 PDT by moz_bug_r_a4
Modified: 2014-11-19 19:46 PST (History)
16 users (show)
cbook: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
verified
verified
verified
22+
verified
22+
fixed
wontfix
affected
fixed


Attachments
testcase (1002 bytes, text/html)
2013-04-04 08:53 PDT, moz_bug_r_a4
no flags Details
Run the DefaultValue algorithm directly on the wrapper for Xrays. v1 (4.82 KB, patch)
2013-04-16 13:58 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Tests. v1 (3.60 KB, patch)
2013-04-16 13:59 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Fix tests. WIP (6.09 KB, patch)
2013-05-03 18:10 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Run the DefaultValue algorithm directly on the wrapper for Xrays. v2 r=mrbkap (5.12 KB, patch)
2013-05-03 18:12 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details | Diff | Splinter Review
test fixes for the web console (3.97 KB, patch)
2013-05-04 04:03 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Fix other tests. v1 (5.69 KB, patch)
2013-05-04 09:19 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Run the DefaultValue algorithm directly on the wrapper for Xrays (on b2g18). v3 r=mrbkap (5.00 KB, patch)
2013-06-04 15:00 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Fix other tests (on b2g18). v1 r=mrbkap (2.71 KB, patch)
2013-06-04 15:00 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Run the DefaultValue algorithm directly on the wrapper for Xrays (on esr17). v3 r=mrbkap (5.06 KB, patch)
2013-06-04 15:12 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Fix other tests (on esr17). v1 r=mrbkap (2.71 KB, patch)
2013-06-04 15:12 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Followup fix for the lack of Cu.isXrayWrapper on esr17/b2g18. v1 r=me (1.59 KB, patch)
2013-06-05 10:45 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2013-04-04 08:50:58 PDT
This is a regression between Firefox 6.0.2 and Firefox 7.  See also bug 720619.
Comment 1 moz_bug_r_a4 2013-04-04 08:53:07 PDT
Created attachment 733366 [details]
testcase

Content-defined toString/valueOf methods are called on window and document.body, but are not called on document.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2013-04-09 11:39:13 PDT
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?
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2013-04-09 12:01:16 PDT
Once we figure out what Xray behavior we want here, I can write the patch.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2013-04-09 18:50:20 PDT
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.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2013-04-16 13:58:48 PDT
Created attachment 738177 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays. v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2013-04-16 13:59:10 PDT
Created attachment 738178 [details] [diff] [review]
Tests. v1
Comment 7 Blake Kaplan (:mrbkap) 2013-04-16 16:20:21 PDT
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.
Comment 8 Blake Kaplan (:mrbkap) 2013-04-16 16:22:55 PDT
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.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2013-04-17 08:05:40 PDT
(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. :-)
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2013-04-17 08:08:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=290412914b10
Comment 11 Jesse Ruderman 2013-04-17 14:10:57 PDT
Based on bug 860494 comment 17, I'm bumping this to sec-high.
Comment 12 Jesse Ruderman 2013-04-17 14:14:56 PDT
(Was something wrong about the sec-moderate analysis in comment 2?)
Comment 13 David Bolter [:davidb] 2013-04-26 11:35:58 PDT
Can we lower the sec rating now that bug 860494 is fixed?
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2013-04-26 11:47:07 PDT
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.
Comment 15 moz_bug_r_a4 2013-04-27 04:48:59 PDT
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.
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2013-05-03 18:10:25 PDT
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.
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2013-05-03 18:12:04 PDT
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.
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2013-05-03 18:12:50 PDT
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.
Comment 19 Mihai Sucan [:msucan] 2013-05-04 04:03:58 PDT
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
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2013-05-04 09:19:27 PDT
Created attachment 745557 [details] [diff] [review]
Fix other tests. v1

Awesome, thanks Mihai! Flagging mrbkap for review on the other test changes.
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2013-05-04 09:20:29 PDT
https://tbpl.mozilla.org/?tree=Try&rev=0226c754e57f
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2013-05-05 15:46:07 PDT
This looks green, and can land pending sec-approval and mrbkap's review. I'm going to file for sec-approval now.
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2013-05-05 15:52:12 PDT
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.
Comment 24 Al Billings [:abillings] 2013-05-06 10:06:03 PDT
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.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-06 16:44:29 PDT
(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?
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2013-05-06 16:56:38 PDT
(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.
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2013-05-07 13:21:35 PDT
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|.
Comment 29 Bobby Holley (:bholley) (busy with Stylo) 2013-05-31 10:38:49 PDT
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.
Comment 30 Alex Keybl [:akeybl] 2013-05-31 14:30:42 PDT
(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.
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2013-05-31 15:20:12 PDT
(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.
Comment 32 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-31 16:00:34 PDT
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).
Comment 35 Alex Keybl [:akeybl] 2013-06-03 12:14:13 PDT
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.
Comment 36 Ryan VanderMeulen [:RyanVM] 2013-06-03 14:07:38 PDT
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.
Comment 37 Ryan VanderMeulen [:RyanVM] 2013-06-04 11:00:02 PDT
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 :)
Comment 38 Bobby Holley (:bholley) (busy with Stylo) 2013-06-04 15:00:21 PDT
Created attachment 758225 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays (on b2g18). v3 r=mrbkap
Comment 39 Bobby Holley (:bholley) (busy with Stylo) 2013-06-04 15:00:37 PDT
Created attachment 758227 [details] [diff] [review]
Fix other tests (on b2g18). v1 r=mrbkap
Comment 40 Bobby Holley (:bholley) (busy with Stylo) 2013-06-04 15:12:10 PDT
Created attachment 758246 [details] [diff] [review]
Run the DefaultValue algorithm directly on the wrapper for Xrays (on esr17). v3 r=mrbkap
Comment 41 Bobby Holley (:bholley) (busy with Stylo) 2013-06-04 15:12:21 PDT
Created attachment 758247 [details] [diff] [review]
Fix other tests (on esr17). v1 r=mrbkap
Comment 45 Bobby Holley (:bholley) (busy with Stylo) 2013-06-05 08:42:22 PDT
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.
Comment 46 Bobby Holley (:bholley) (busy with Stylo) 2013-06-05 10:45:05 PDT
Created attachment 758673 [details] [diff] [review]
Followup fix for the lack of Cu.isXrayWrapper on esr17/b2g18. v1 r=me
Comment 50 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-18 12:39:46 PDT
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?
Comment 51 Mihai Sucan [:msucan] 2013-06-18 12:45:15 PDT
(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).
Comment 52 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-18 12:50:18 PDT
Thanks Mihai, that explains a lot! :)

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

Much appreciated.
Comment 53 Bobby Holley (:bholley) (busy with Stylo) 2013-06-18 12:54:40 PDT
(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.
Comment 54 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-18 13:07:06 PDT
Thanks Bobby. 

So, we now expect to see XrayWrapper.toString output only, in this case. Is that correct?
Comment 55 Bobby Holley (:bholley) (busy with Stylo) 2013-06-18 14:27:30 PDT
(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.
Comment 56 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-18 15:47:53 PDT
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.
Comment 57 Bobby Holley (:bholley) (busy with Stylo) 2013-11-20 16:05:25 PST
landed tests, which are green locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa45cf3568d3
Comment 58 Carsten Book [:Tomcat] 2013-11-21 05:49:56 PST
and landed on central https://hg.mozilla.org/mozilla-central/rev/fa45cf3568d3

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