Bug 1125389 (CVE-2015-0820)

ChangeObjectFixedSlotCount can make a non-extensible object extensible

RESOLVED FIXED in Firefox 36, Firefox OS v2.2

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jandem, Assigned: bhackett)

Tracking

({regression, sec-moderate})

unspecified
mozilla38
regression, sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ fixed, firefox37+ fixed, firefox38 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main36+]sec-high for Caja)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Test below fails with --ion-eager --no-threads. I think the problem is that we reshape in ChangeObjectFixedSlotCount and then lose the not-extensible flag.

This was reduced from a jstest failure (ecma_6/Array/from_errors.js) with --ion-eager --no-threads

function Obj() {
    this.x = 0;
    Object.preventExtensions(this);
}
function test() {
    var A = new Obj();
    while (true) {
	assertEq(Object.isExtensible(A), false);
        return;
    }
    A.length1 = X;
}
test();
(Reporter)

Updated

3 years ago
Flags: needinfo?(bhackett1024)
(Reporter)

Comment 1

3 years ago
With the patches in bug 1125505 I get a jit-test failure, I think it's the same problem but with the is-delegate object flag.
Blocks: 1125505
(Reporter)

Comment 2

3 years ago
Locking for now in case this is sec-sensitive.
Group: core-security

Comment 3

3 years ago
I must be missing something obvious. In the code 

function test() {
    var A = new Obj();
    while (true) {
	assertEq(Object.isExtensible(A), false);
        return;
    }
    A.length1 = X;
}

isn't the "A.length1 = X;" line unreachable? I don't get it.
(Reporter)

Comment 4

3 years ago
(In reply to Mark S. Miller from comment #3)
> isn't the "A.length1 = X;" line unreachable? I don't get it.

It is unreachable, but it must be triggering something in the JIT or something as it didn't repro without that line.
(In reply to Mark S. Miller from comment #3)
> isn't the "A.length1 = X;" line unreachable? I don't get it.

The test case isn't as clear as it could be. Here's a slightly clearer one:

function Obj() {
	this.bar = 0;
    Object.preventExtensions(this);
}
function test() {
    var A = new Obj();
    var i = 0;
    while (!Object.isExtensible(A)) {
    	i++;
    }
    A.foo = 1;
    print(A.foo, i);
}
test(); // Prints `1, [some 4-digit number]` when run in the shell with --ion-eager.


However, this only reproduces in the shell with --ion-eager, so it might not affect the browser.
Hum, here's one that reproduces in the browser:

function Obj() {
    this.x = 0;
    Object.preventExtensions(this);
}
var i = 0;
function test() {
    var A = new Obj();
    while (true) {
    	i++;
    	if (Object.isExtensible(A)) {
    		throw new Error("Shouldn't get here");
    	}
    }
    A.length1 = X;
}
try {
	test();
} catch (e) {
	console.log(e, 'iterations: ' + i);
}

Comment 7

3 years ago
In that case, yes, this is a security vulnerability.

I'm not sure what the right protocol is for sharing knowledge of a non-public vulnerability like this. May I individually inform some other Cajadores who work at Google?
(In reply to Mark S. Miller from comment #7)
> In that case, yes, this is a security vulnerability.
> 
> I'm not sure what the right protocol is for sharing knowledge of a
> non-public vulnerability like this. May I individually inform some other
> Cajadores who work at Google?

Absolutely.

Also note that this bug was open for about 17 hours, so it's not completely non-public by now. I doubt interested parties would have much of a hard time adapting the test case from comment 0 to work similarly to the one in comment 6. :(
(Assignee)

Comment 9

3 years ago
Created attachment 8554594 [details] [diff] [review]
patch
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8554594 - Flags: review?(jdemooij)
(Reporter)

Updated

3 years ago
Attachment #8554594 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 10

3 years ago
Comment on attachment 8554594 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1041688
[User impact if declined]: Some objects marked as non-extensible can be made extensible again.
[Risks and why]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

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

No.

Which older supported branches are affected by this flaw?

35+

If not all supported branches, which bug introduced the flaw?

bug 1041688

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

trivial

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

none
Attachment #8554594 - Flags: sec-approval?
Attachment #8554594 - Flags: approval-mozilla-beta?
Attachment #8554594 - Flags: approval-mozilla-aurora?
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox-esr31: --- → unaffected
We need to figure out a security rating for this before approval. The comments do not make it clear of the implications of the problem: "Some objects marked as non-extensible can be made extensible again."
Flags: needinfo?(bhackett1024)
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
(Assignee)

Comment 12

3 years ago
(In reply to Al Billings [:abillings] from comment #11)
> We need to figure out a security rating for this before approval. The
> comments do not make it clear of the implications of the problem: "Some
> objects marked as non-extensible can be made extensible again."

I don't know myself, I was just going by comment 7.
Flags: needinfo?(bhackett1024)
(In reply to Al Billings [:abillings] from comment #11)
> We need to figure out a security rating for this before approval. The
> comments do not make it clear of the implications of the problem: "Some
> objects marked as non-extensible can be made extensible again."

I think this mostly/only affects sandboxing systems such as Google's Caja. To quote from the Caja documentation[1], "SES, to enforce object-capability rules, must prevent all un-granted communications channels. So in SES, all built-in primordial objects are transitively frozen [...]"

So essentially this bug disables some of the most fundamental security guarantees Caja gives.

[1] https://code.google.com/p/google-caja/wiki/SES#No_Monkey_Patching_Primordials
Any thoughts on a rating for this issue, Dan?
Flags: needinfo?(dveditz)
It kind of sounds like a sec-moderate because we're not really running code cross origin anything, but instead we're failing to provide some kind of security mechanism to the webpage itself.  The security rating page says that sec-moderate includes "Missing Additional Security Controls (x-frame options, SECURE/HTTPOnly flags, etc)".
Keywords: sec-moderate
Comment on attachment 8554594 [details] [diff] [review]
patch

Clearing the sec-approval? flag as sec-moderates don't need approval to go into trunk.
Attachment #8554594 - Flags: sec-approval?
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Blocks: 1041688
Flags: needinfo?(dveditz)
Keywords: regression
Whiteboard: sec-high for Caja
https://hg.mozilla.org/mozilla-central/rev/51ac953371fe
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
status-b2g-master: affected → fixed
Should this be uplifted to Aurora and Beta? That'd make the window smaller in which sites can be exploited.

Comment 22

3 years ago
Please! That would be wonderful!
(And now I see that bhackett already set the flags for that in his initial sec-approval request.)
tracking-firefox36: --- → +
tracking-firefox37: --- → +
Attachment #8554594 - Flags: approval-mozilla-beta?
Attachment #8554594 - Flags: approval-mozilla-beta+
Attachment #8554594 - Flags: approval-mozilla-aurora?
Attachment #8554594 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b2c29669f13a
https://hg.mozilla.org/releases/mozilla-beta/rev/26d78b7b0bfa

For those watching from home, this fix will be in Fx36beta6 going out around Tuesday next week. It can otherwise be tested in a current Fx38 nightly build now or tomorrow's Fx37 DevEdition nightly.
status-firefox36: affected → fixed
status-firefox37: affected → fixed
(Assignee)

Updated

3 years ago
Depends on: 1127987

Comment 26

3 years ago
Google's official policy https://support.google.com/a/answer/33864 is that the present and immediately previous major releases of FF, IE, and Safari are supported. IIUC, the release of FF36 is about a week away, and the release of FF37 about 6 weeks after that.

I have an undisclosed CL at https://codereview.appspot.com/202040043/ that cannot be revealed without disclosing this bug. However, since this bug seems fatal, I don't know the triggering conditions, nor any workarounds or mitigations, once this CL is submitted, Caja and SES will merely detect the existence of the bug and refuse to run, so it will stop working on FF35. Should this bug become public or we see evidence that it is being exploited, we would need to submit this CL immediately.

If we did have a workaround we could add as a repair to https://code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/ses/repairES5.js , then we could avoid an unpleasant situation. Curiously, I do have one weak bit of evidence that this bug may be repairable:

When I run the unsubmitted SES self test from that secret CL, when it runs the test a second time it, the symptoms repeatedly disappear. However, since I don't understand why, or what triggers this apparent disappearance, I have the SES self test conservatively assume that the bug can still be exploited.

Any ideas for a potential mitigation or workaround?
Brian, is there anything that content script could do to prevent this bug from occurring?
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 28

3 years ago
(In reply to Till Schneidereit [:till] from comment #27)
> Brian, is there anything that content script could do to prevent this bug
> from occurring?

Yes, there are several ways.  This bug only affects objects created by calling 'new Foo()' on some script Foo.  If the constructor has been called more than 20 times before preventing extensions then the bug won't impact anything.  If the object which preventExtensions is called on has any properties which are non-configurable or non-writable then the bug won't impact anything (so calling seal() or freeze() on an object with at least one property shouldn't be able to trigger this bug)
Flags: needinfo?(bhackett1024)

Comment 29

3 years ago
Awesome! That's what we needed. I will start work on a repair.

Thanks!
Whiteboard: sec-high for Caja → [adv-main36+]sec-high for Caja

Comment 30

3 years ago
At https://codereview.appspot.com/202040043/ we have a non-disclosed draft CL that we have approved for testing and repairing this issue for Caja and SES. The test is adapted from comment 6. The repair is adapted from comment 28. It seems to work on FF35 and doesn't break on anything else.

However, since we don't really understand why comment 6 triggers the bug nor why comment 28 prevents the bug, it is always possible we screwed something up without noticing. For example, our test uses a TypeError rather than a ReferenceError because it seems to work as well and the code is a bit cleaner.

Would anyone on this bug thread care to be given access to our CL so you can take a look and tell us what we should be concerned about, before we submit? We'd really appreciate it.

Note that in order for me to get codereview.appspot.com to give you access, I'd need a Google account of some sort for you. Thanks.

Comment 31

3 years ago
> Note that in order for me to get codereview.appspot.com to give you access,
> I'd need a Google account of some sort for you. Thanks.

If that's a problem, please let me know and I can just send you a patch file instead, or before-and-after source files if you prefer.
Alias: CVE-2015-0820
Brian, I think you'd be most qualified by far to do the review Mark requested in comment 30.
Flags: needinfo?(bhackett1024)

Comment 33

3 years ago
Hi Brian, I've added bhackett1024@gmail.com . Please let me know if you can see it. Thanks!
(Assignee)

Comment 34

3 years ago
(In reply to Mark S. Miller from comment #33)
> Hi Brian, I've added bhackett1024@gmail.com . Please let me know if you can
> see it. Thanks!

Hi, the patch looks good to me.

(In reply to Mark S. Miller from comment #30)
> However, since we don't really understand why comment 6 triggers the bug nor
> why comment 28 prevents the bug, it is always possible we screwed something
> up without noticing. For example, our test uses a TypeError rather than a
> ReferenceError because it seems to work as well and the code is a bit
> cleaner.

This error crops up due to some transformations we make to objects during an analysis of objects created using 'new' --- we look at the eventual number of properties of the largest created object for a given function and tailor the size of existing and future objects accordingly.  The analysis is only interested in objects that have seen plain property definitions in the constructor like 'function Foo() { this.a = 3; } new Foo()', and bails out on other sorts of properties on the 'new' objects.
Flags: needinfo?(bhackett1024)

Comment 35

3 years ago
https://codereview.appspot.com/202040043/
https://codereview.appspot.com/202030043/
https://codereview.appspot.com/214110043/
Are now committed and public in Caja r5717. Please make this issue public as well. Thanks.

Comment 36

3 years ago
Somewhat explained at https://code.google.com/p/google-caja/wiki/SecurityAdvisory20150313

Can this issue now be made public?
Till, if when you get back you could land one of your test cases now that this is fixed, that would be great.  Thanks.
Group: core-security
Flags: needinfo?(till)
Flags: needinfo?(till)
You need to log in before you can comment on or make changes to this bug.