Closed Bug 858101 (CVE-2013-1697) Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Security, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 --- verified
firefox23 --- verified
firefox24 --- verified
firefox-esr17 22+ verified
b2g18 22+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected
b2g-v1.1hd --- fixed

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

Details

(Keywords: sec-high, Whiteboard: [adv-main22+][adv-esr1707+])

Attachments

(10 files, 2 obsolete files)

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
This is a regression between Firefox 6.0.2 and Firefox 7.  See also bug 720619.
Attached file 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)
Attached patch Tests. v1Splinter Review
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. :-)
Based on bug 860494 comment 17, I'm bumping this to sec-high.
Keywords: sec-moderatesec-high
(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.
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.
Attached patch Fix tests. WIP (obsolete) — Splinter Review
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)
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+
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)
Awesome, thanks Mihai! Flagging mrbkap for review on the other test changes.
Attachment #745450 - Attachment is obsolete: true
Attachment #745557 - Flags: review?(mrbkap)
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+
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?
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.
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).
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.
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)
Flags: needinfo?(bobbyholley+bmo)
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.
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.
Whiteboard: [adv-main22+][adv-esr1707+]
Alias: CVE-2013-1697
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.