813901 - (CVE-2013-0757) Chrome Object Wrapper can be bypassed using Object.prototype.__proto__

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mariusz Mlynski]

Attachment: 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] (NeedInfo me for attention)]

(Aside: gave Mariusz canconfirm bug privilege.)

Comment 2

[Bobby Holley (:bholley)]

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

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

Attachment: Tests. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Throw COW exceptions in the wrapper's scope. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Validate the origin of __exposedProps__. v1

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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

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

Can you suggest a security rating for this, Bobby?

Comment 10

[Al Billings [:abillings - ex-MoCo]]

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

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

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

Attachment: Tests. v2 r=mrbkap

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Bobby Holley (:bholley)]

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

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/86137785c2ab
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34384723ccfc

Comment 17

[Bobby Holley (:bholley)]

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 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/34384723ccfc
https://hg.mozilla.org/mozilla-central/rev/86137785c2ab

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d99039171189
https://hg.mozilla.org/releases/mozilla-aurora/rev/a8694744c763

https://hg.mozilla.org/releases/mozilla-beta/rev/b7449fe8cf06
https://hg.mozilla.org/releases/mozilla-beta/rev/051b3d8f915d

https://hg.mozilla.org/releases/mozilla-esr17/rev/9f4823f294d5
https://hg.mozilla.org/releases/mozilla-esr17/rev/ed2e7e4c6699

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

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 22

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b6a5fcd7f54b
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/ef8021ce1492

Comment 23

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/aa7b46b95085
remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/02329ba80ec6

Comment 24

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

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

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

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

Pushed the test:

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

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d67b2524bd3f