1125389 - (CVE-2015-0820) ChangeObjectFixedSlotCount can make a non-extensible object extensible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Jan de Mooij [:jandem]]

Locking for now in case this is sec-sensitive.

Comment 3

[Mark S. Miller]

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.

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Till Schneidereit [:till]]

(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.

Comment 6

[Till Schneidereit [:till]]

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

[Mark S. Miller]

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?

Comment 8

[Till Schneidereit [:till]]

(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. :(

Comment 9

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 10

[Brian Hackett [Laid off!]]

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

Comment 11

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

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."

Comment 12

[Brian Hackett [Laid off!]]

(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.

Comment 13

[Till Schneidereit [:till]]

(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

Comment 14

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

Any thoughts on a rating for this issue, Dan?

Comment 15

[Andrew McCreight [:mccr8]]

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)".

Comment 16

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

Comment on attachment 8554594 [details] [diff] [review]
patch

Clearing the sec-approval? flag as sec-moderates don't need approval to go into trunk.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8361462f2a02

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79c310fc659

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5984197&repo=mozilla-inbound

Comment 19

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51ac953371fe

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51ac953371fe

Comment 21

[Till Schneidereit [:till]]

Should this be uplifted to Aurora and Beta? That'd make the window smaller in which sites can be exploited.

Comment 22

[Mark S. Miller]

Please! That would be wonderful!

Comment 23

[Till Schneidereit [:till]]

(And now I see that bhackett already set the flags for that in his initial sec-approval request.)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b2c29669f13a

Comment 26

[Mark S. Miller]

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?

Comment 27

[Till Schneidereit [:till]]

Brian, is there anything that content script could do to prevent this bug from occurring?

Comment 28

[Brian Hackett [Laid off!]]

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

Comment 29

[Mark S. Miller]

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

Thanks!

Comment 30

[Mark S. Miller]

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

[Mark S. Miller]

> 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.

Comment 32

[Till Schneidereit [:till]]

Brian, I think you'd be most qualified by far to do the review Mark requested in comment 30.

Comment 33

[Mark S. Miller]

Hi Brian, I've added [e-mail] . Please let me know if you can see it. Thanks!

Comment 34

[Brian Hackett [Laid off!]]

(In reply to Mark S. Miller from comment #33)
> Hi Brian, I've added [e-mail] . 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.

Comment 35

[Mark S. Miller]

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

[Mark S. Miller]

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

Can this issue now be made public?

Comment 37

[Andrew McCreight [:mccr8]]

Till, if when you get back you could land one of your test cases now that this is fixed, that would be great.  Thanks.