Last Comment Bug 813901 - (CVE-2013-0757) Chrome Object Wrapper can be bypassed using Object.prototype.__proto__
(CVE-2013-0757)
: Chrome Object Wrapper can be bypassed using Object.prototype.__proto__
Status: VERIFIED FIXED
[js:t][adv-main18+][adv-esr17+]
: regression, sec-high, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 17 Branch
: All All
: -- critical (vote)
: mozilla20
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-21 01:54 PST by Mariusz Mlynski
Modified: 2014-07-24 14:38 PDT (History)
14 users (show)
abillings: sec‑bounty+
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
unaffected
18+
verified


Attachments
Proof of concept (587 bytes, text/html)
2012-11-21 01:54 PST, Mariusz Mlynski
no flags Details
Tests. v1 (1.87 KB, patch)
2012-11-21 18:36 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 1 - Throw COW exceptions in the wrapper's scope. v1 (3.56 KB, patch)
2012-11-21 18:37 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Validate the origin of __exposedProps__. v1 (2.59 KB, patch)
2012-11-21 18:37 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
abillings: sec‑approval+
Details | Diff | Splinter Review
Tests. v2 r=mrbkap (1.88 KB, patch)
2012-12-06 14:59 PST, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Part 2 - Validate __exposedProps__. v2 r=mrbkap (3.54 KB, patch)
2012-12-07 14:48 PST, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr17+
Details | Diff | Splinter Review

Description Mariusz Mlynski 2012-11-21 01:54:56 PST
Created attachment 683930 [details]
Proof of concept

It is possible to change the prototype of an object wrapped in a cross-compartment wrapper if the setter of Object.prototype.__proto__ is called in the context of the wrapper. This can be used against chrome objects that don't explicitly define __exposedProps__ to gain access to privileged methods.

This bug appears to have been introduced in Firefox 17, the earlier versions don't have accessors on __proto__.
Comment 1 David Bolter [:davidb] 2012-11-21 10:09:33 PST
(Aside: gave Mariusz canconfirm bug privilege.)
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-11-21 10:49:41 PST
Very clever, Mariusz. Glad to have you on our side. :-)

So the testcase here is changing the prototype on the wrapped chrome object and relying on the fact that __exposedProps__ is allowed to be inherited via the prototype chain. So essentially, the content is defining its own ACL and tricking XPConnect into using it. Classic.

So there are two bugs here. First, people definitely should not be able to do mutable proto on a security wrapper. This is happening via nativeCall right now, and we're finally in a position to fix that, with bug 809652. Please don't set the dep on that bug, because it's a public bug billed as defense-in-depth, and I'd like to keep it that way.

Regardless, the more general problem here is that we don't ensure that the ACL is same-compartment with the wrappee. We should do a compartment check here and throw if that happens.

So I think a two-pronged approach is warranted here. Either will fix the bug, and both are good to have. The nativeCall thing has the advantage that it's more general and far less indicative of the problem here, but the machinery necessary to do it was introduced in 19, so it doesn't exist on beta or esr17.
Comment 3 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2012-11-21 17:45:09 PST
The plan sounds good to me. I feel like we used to deny setting __proto__ on security wrappers but stopped doing so because it was considered "overly paranoid." Oh well.

I second bholley's compliment: this bug is extremely clever!
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-11-21 18:36:49 PST
Created attachment 684262 [details] [diff] [review]
Tests. v1
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-11-21 18:37:08 PST
Created attachment 684263 [details] [diff] [review]
Part 1 - Throw COW exceptions in the wrapper's scope. v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-11-21 18:37:34 PST
Created attachment 684264 [details] [diff] [review]
Part 2 - Validate the origin of __exposedProps__. v1
Comment 7 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2012-12-03 15:48:39 PST
Comment on attachment 684262 [details] [diff] [review]
Tests. v1

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

::: js/xpconnect/tests/unit/test_bug813901.js
@@ +17,5 @@
> +  sb.obj.__proto__ = sb.p;
> +  checkThrows('obj.foo = 4;', sb, /__exposedProps__/);
> +}
> +
> +function checkThrows(expression, sb, regexp) {

Stylistic quibble: I'd prefer checkThrows to come first even though it gets hoisted anyway.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-12-05 13:43:45 PST
Comment on attachment 684264 [details] [diff] [review]
Part 2 - Validate the origin of __exposedProps__. v1

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Fairly easily. It's pretty clear that we're protected against a content-defined __exposedProps__. Now, this doesn't say _how_ to do that. The __proto__ trick is indeed clever, but I'm concerned that, once someone is looking for a way to inject a client-defined __exposedProps__, the __proto__ thing becomes a lot more obvious as an exploit vector.

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

The exception message (argument to JS_ReportError) is probably the most obvious indicator.

Which older supported branches are affected by this flaw?

All of them.

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

I don't have backports, but they should be fairly straightforward.

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

It's relatively low-risk. I think the best tradeoff is probably to land this on all branches one week before the beta codefreeze.

I could also make the exception message here a bit more cryptic ("Invalid __exposedProps__"), which might help.
Comment 9 Al Billings [:abillings] 2012-12-06 10:13:03 PST
Can you suggest a security rating for this, Bobby?
Comment 10 Al Billings [:abillings] 2012-12-06 10:13:37 PST
Comment on attachment 684264 [details] [diff] [review]
Part 2 - Validate the origin of __exposedProps__. v1

Sec-approval+. Please prepare branch patches as well.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-12-06 12:09:21 PST
(In reply to Al Billings [:abillings] from comment #10)
> Sec-approval+. Please prepare branch patches as well.

Does this indicate that I should check this in now as is, or should I do something along the lines of what's mentioned in comment 8?
Comment 12 Al Billings [:abillings] 2012-12-06 13:48:57 PST
(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Al Billings [:abillings] from comment #10)
> > Sec-approval+. Please prepare branch patches as well.
> 
> Does this indicate that I should check this in now as is, or should I do
> something along the lines of what's mentioned in comment 8?

If no date restriction is given, you're good to go for mozilla-central. If by the mention in comment 8, you mean:

> I could also make the exception message here a bit more cryptic ("Invalid __exposedProps__"), which might help.

That would be great but I'll leave it to your discretion here.

Once it is on central, you can attach branch patches and ask for approval to check them in. We do branch triage collectively once a week and release management watches requests daily.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-12-06 14:59:08 PST
Created attachment 689407 [details] [diff] [review]
Tests. v2 r=mrbkap
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-12-06 15:08:27 PST
https://tbpl.mozilla.org/?tree=Try&rev=c867eb2382f1
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-12-07 14:48:01 PST
Created attachment 689952 [details] [diff] [review]
Part 2 - Validate __exposedProps__. v2 r=mrbkap

There was one test failure in test_cows. The fix is to modify test_cows to deep
clone in getCOW, otherwise we end up with content-defined __exposedProps__.

Carrying over review.
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-12-07 14:57:25 PST
Comment on attachment 689952 [details] [diff] [review]
Part 2 - Validate __exposedProps__. v2 r=mrbkap

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown, but apparently versions prior to 17 didn't allow setting __proto__ on security wrappers.
User impact if declined: security bugs
Testing completed (on m-c, etc.): Just landed to inbound
Risk to taking this patch (and alternatives if risky): The only real risk here is that we might break an addon that was defining __exposedProps__ in content (I think the likelihood of this is _extremely_ small). But, well, we don't want to allow that. The alternative would be to try to disable proto setting on security wrappers, which is effectively what bug 809652 is. But that's higher risk.
String or UUID changes made by this patch: None
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-12-10 08:03:40 PST
Looks like this is going to need beta/esr17 specific patches. My attempt to unbitrot and land this there resulted in red.
https://hg.mozilla.org/releases/mozilla-beta/rev/890b55d9284c
https://hg.mozilla.org/releases/mozilla-esr17/rev/e7ae8a290745
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-12-10 11:37:18 PST
Looks like it was just a simple mismerge. The chunk using hallpass needs to come after the definition:

https://hg.mozilla.org/releases/mozilla-beta/rev/051b3d8f915d#l2.35

I just did the merge with git cherry-pick and it seemed to do the right thing. I'll push it now.
Comment 24 Matt Wobensmith [:mwobensmith][:matt:] 2013-01-14 17:40:35 PST
Confirmed issue with nightly, 2012-11-21.
Confirmed fixed 2013-01-14, nightly
Confirmed fixed 2013-01-14, Aurora
Confirmed fixed 2013-01-14, beta
Confirmed fixed 17.0.2esr
Comment 26 Christian Holler (:decoder) 2013-03-11 09:44:33 PDT
I tried adding the test here but a try run failed:

https://tbpl.mozilla.org/php/getParsedLog.php?id=20525220&tree=Try&full=1#error1

bholley, can you take a look and fix the test (and possibly land it then)? Thanks!
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2013-03-11 14:55:07 PDT
(In reply to Christian Holler (:decoder) from comment #26)
> I tried adding the test here but a try run failed:
> 
> https://tbpl.mozilla.org/php/getParsedLog.
> php?id=20525220&tree=Try&full=1#error1
> 
> bholley, can you take a look and fix the test (and possibly land it then)?
> Thanks!

I filed bug 850000 for this.
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2013-03-17 21:45:26 PDT
Pushed the test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d67b2524bd3f
Comment 29 Ed Morley [:emorley] 2013-03-18 13:16:11 PDT
https://hg.mozilla.org/mozilla-central/rev/d67b2524bd3f

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