858101 - (CVE-2013-1697) [[DefaultValue]] on XrayWrapper can call content-defined toString/valueOf methods

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

This is a regression between Firefox 6.0.2 and Firefox 7.  See also bug 720619.

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Content-defined toString/valueOf methods are called on window and document.body, but are not called on document.

Comment 2

[Bobby Holley (:bholley)]

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

Once we figure out what Xray behavior we want here, I can write the patch.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Attachment: Run the DefaultValue algorithm directly on the wrapper for Xrays. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

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

(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)]

https://tbpl.mozilla.org/?tree=Try&rev=290412914b10

Comment 11

[Jesse Ruderman]

Based on bug 860494 comment 17, I'm bumping this to sec-high.

Comment 12

[Jesse Ruderman]

(Was something wrong about the sec-moderate analysis in comment 2?)

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Can we lower the sec rating now that bug 860494 is fixed?

Comment 14

[Bobby Holley (:bholley)]

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]

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

Attachment: 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)]

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

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

Attachment: 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)]

Attachment: Fix other tests. v1

Awesome, thanks Mihai! Flagging mrbkap for review on the other test changes.

Comment 21

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0226c754e57f

Comment 22

[Bobby Holley (:bholley)]

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

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 - ex-MoCo]]

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: [e-mail]]]

(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)]

(In reply to :Gavin Sharp (use [e-mail] 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)]

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 28

[Bobby Holley (:bholley)]

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 29

[Bobby Holley (:bholley)]

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

(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)]

(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: [e-mail]]]

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 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/266b669dd70b
https://hg.mozilla.org/mozilla-central/rev/12653403df73
https://hg.mozilla.org/mozilla-central/rev/7d9e1b954e7f

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Alex Keybl [:akeybl]]

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

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

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

Attachment: Run the DefaultValue algorithm directly on the wrapper for Xrays (on b2g18). v3 r=mrbkap

Comment 39

[Bobby Holley (:bholley)]

Attachment: Fix other tests (on b2g18). v1 r=mrbkap

Comment 40

[Bobby Holley (:bholley)]

Attachment: Run the DefaultValue algorithm directly on the wrapper for Xrays (on esr17). v3 r=mrbkap

Comment 41

[Bobby Holley (:bholley)]

Attachment: Fix other tests (on esr17). v1 r=mrbkap

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

And backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/67c807727762
https://hg.mozilla.org/releases/mozilla-esr17/rev/724fa6b27e3f

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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

Comment 45

[Bobby Holley (:bholley)]

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

Attachment: Followup fix for the lack of Cu.isXrayWrapper on esr17/b2g18. v1 r=me

Comment 47

[Ryan VanderMeulen [:RyanVM]]

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

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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

Comment 49

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 50

[Matt Wobensmith [:mwobensmith][:matt:]]

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

(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:]]

Thanks Mihai, that explains a lot! :)

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

Much appreciated.

Comment 53

[Bobby Holley (:bholley)]

(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:]]

Thanks Bobby. 

So, we now expect to see XrayWrapper.toString output only, in this case. Is that correct?

Comment 55

[Bobby Holley (:bholley)]

(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:]]

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

landed tests, which are green locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa45cf3568d3

Comment 58

[Carsten Book [:Tomcat]]

and landed on central https://hg.mozilla.org/mozilla-central/rev/fa45cf3568d3