Closed Bug 813901 (CVE-2013-0757) Opened 7 years ago Closed 7 years ago

Chrome Object Wrapper can be bypassed using Object.prototype.__proto__

Categories

(Core :: JavaScript Engine, defect, critical)

17 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 + verified
firefox19 + verified
firefox20 + verified
firefox-esr10 --- unaffected
firefox-esr17 18+ verified

People

(Reporter: marius.mlynski, Assigned: bholley)

Details

(Keywords: regression, sec-high, testcase, Whiteboard: [js:t][adv-main18+][adv-esr17+])

Attachments

(4 files, 2 obsolete files)

Attached file 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__.
Attachment #683930 - Attachment mime type: text/plain → text/html
(Aside: gave Mariusz canconfirm bug privilege.)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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!
Attached patch Tests. v1 (obsolete) — Splinter Review
Attachment #684262 - Flags: review?(mrbkap)
Attachment #684264 - Flags: review?(mrbkap)
Flags: in-testsuite?
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.
Attachment #684262 - Flags: review?(mrbkap) → review+
Attachment #684263 - Flags: review?(mrbkap) → review+
Attachment #684264 - Flags: review?(mrbkap) → review+
Whiteboard: [js:t]
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.
Attachment #684264 - Flags: sec-approval?
Can you suggest a security rating for this, Bobby?
Comment on attachment 684264 [details] [diff] [review]
Part 2 - Validate the origin of __exposedProps__. v1

Sec-approval+. Please prepare branch patches as well.
Attachment #684264 - Flags: sec-approval? → sec-approval+
(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?
Keywords: sec-high
(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.
Attachment #684262 - Attachment is obsolete: true
Attachment #689407 - Flags: review+
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.
Attachment #684264 - Attachment is obsolete: true
Attachment #689952 - Flags: review+
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
Attachment #689952 - Flags: approval-mozilla-esr17?
Attachment #689952 - Flags: approval-mozilla-beta?
Attachment #689952 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/34384723ccfc
https://hg.mozilla.org/mozilla-central/rev/86137785c2ab
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Attachment #689952 - Flags: approval-mozilla-esr17?
Attachment #689952 - Flags: approval-mozilla-esr17+
Attachment #689952 - Flags: approval-mozilla-beta?
Attachment #689952 - Flags: approval-mozilla-beta+
Attachment #689952 - Flags: approval-mozilla-aurora?
Attachment #689952 - Flags: approval-mozilla-aurora+
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
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.
Whiteboard: [js:t] → [js:t][adv-main18+][adv-esr17+]
Alias: CVE-2013-0757
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
Flags: sec-bounty? → sec-bounty+
Group: core-security
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!
Flags: needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(bobbyholley+bmo)
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.