More reachable objects not cleaned

[GoogleCodeExporter]

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsic-o
bjects lists the intrinsic objects of ES6, not all of which are reachable by 
own property traversal from roots. We've already encountered and fixed the 
issue with %ThrowTypeError% (nee [[ThrowTypeError]]) escaping the whitelist 
cleaning. Since it was also (in violation of the spec) not frozen, this caused 
a loss of isolation. In ES5, this was the only object that could cause this 
problem.

Now that we are whitelisting some ES6-only objects, we need to be more vigilant 
about providing access to other intrinsics that we may not have cleaned. The 
specific case I just found is that the named typed array constructors, such as 
Float32Array, are speced to inherit from the %TypedArray% intrinsic, which is 
not otherwise reachable. It was thus escaping our whitelist-based cleaning. At 
the time of this writing, Chrome and Safari do not yet implement this part of 
the spec, and no breach seems to occur there. Unfortunately, FF35 and FF 
Nightly 38 do, so we LOSE ISOLATION there due to this hole. 

Thus, this is high priority / security / private -- needing to go through the 
responsible disclosure process.

Original issue reported on code.google.com by erights on 15 Feb 2015 at 5:15

[GoogleCodeExporter]

The only reason we caught this is because of another anomaly that occurs on 
FF35 only (of the browsers I tested): The objects that in ES6 will have a 
symbol-named property named by Symbol.iterator, on FF35, instead have a 
string-named property named '@@iterator'. The name '@@iterator' is absent from 
our whitelist and is present on in FF35 on %TypedArray%.prototype. The same 
self-test that noticed this should also test if there are non-frozen objects 
reachable from known primordials by prototype traversal.

At some point we'll need to extend SES to know about Symbols and symbol-named 
properties, as well as much more of ES6. However, at the moment, even with 
these bugs, AFAIK, our whitelist does not provide access to anything that would 
enable access to a Symbol, and so still provides a safe ES5-ish view in this 
regard.

Original comment by erights on 15 Feb 2015 at 5:15

[GoogleCodeExporter]

Original comment by erights on 15 Feb 2015 at 6:45

• Added labels: Priority-Critical
• Removed labels: Priority-High

[GoogleCodeExporter]

Original comment by erights on 15 Feb 2015 at 6:56

• Added labels: Component-SES
• Removed labels: Component-Other

[GoogleCodeExporter]

Terrible news. I just filed undisclosed v8 bug 
https://code.google.com/p/v8/issues/detail?id=3902 , which says:

Title: URGENT! Unsafe %GeneratorFuntion% intrinsic cannot be denied

The short form:

(function*(){}).constructor('yield window;')().next().value; // the window

Please cc kpreid@google.com on this bug.

As shown at http://people.mozilla.org/~jorendorff/figure-2.png , the 
%GeneratorFunction% intrinsic and the %Generator% intrinsic initially point at 
each other. %Generator%.constructor is initially %GeneratorFunction% and 
%GeneratorFunction%.prototype is initially %Generator%.

Section 25.2.3.1 of 
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-generatorfunction.prot
otype.constructor says
> The initial value of GeneratorFunction.prototype.constructor is the intrinsic 
object %GeneratorFunction%.
> This property has the attributes { [[Writable]]: false, [[Enumerable]]: 
false, [[Configurable]]: true }.

I checked the latest draft at 
http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_drafts&c
ache=cache&media=harmony:working_draft_ecma-262_edition_6_02-12-15-rev33markup.p
df and this section is unchanged. This means that an 

Object.defineProperty(%Generator%, 'constructor', { value: somethingElse })

should change %Generator%.constructor to be somethingElse. Unfortunately, on v8 
(checked both Chrome 41 beta and Chrome 42 canary with the "experimental 
javascript" flag turned on), this property has attributes { [[Writable]]: 
false, [[Enumerable]]: false, [[Configurable]]: false }; and indeed, as that 
implies, its value cannot be changed.

Changing the value of this property is absolutely essentially to the security 
of Caja and SES, and of those Google properties that rely on it. Currently, 
these Google properties are fully vulnerable, and knowledge of this attack 
should be treated according to responsible disclosure principles. The 
undisclosed Caja bug for tracking this is 
https://code.google.com/p/google-caja/issues/detail?id=1953 . Please let me 
know if you'd like access.

Why is this fatal?

The real %GeneratorFunction%, like the initial Function constructor or the 
initial eval function, provides unconditional access to the global object of 
that realm. The security of Caja and SES depends on being able to hide these 
objects from confined code. (It depends on much else as well, but these are 
necessary conditions.)

Some of the intrinsics are what we call "undeniables". For example, the 
original Function.prototype is undeniable, since any "function(){}" expression 
evaluates to a new function which inherits from it, independent of any other 
consideration. However, SES still denied access to the original Function 
constructor by setting Function.prototype.constructor to point at FakeFunction. 
See 
https://code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/se
s/startSES.js#1018

Likewise, in v8 or any other browser that has implemented ES6 generators, 
%Generator% is undeniable, because any "function*(){}" expression inherits from 
it. In order for SES to still deny access to %GeneratorFunction%, it needs to 
change %Generator%.constructor to point at a FakeGeneratorFunction. We have an 
undisclosed CL https://codereview.appspot.com/202030043/ that does exactly 
this, and works on all the other browsers we tested on, except Opera of course. 
Please let me know if you'd access to this CL.

Fortunately, the v8 bug can be quietly fixed apparently just to conform to the 
spec, without the fix itself calling attention to the existence of this 
vulnerability. Unfortunately, the same cannot be said of the Caja CL. There's 
no way to upgrade Caja to seal off %GeneratorFunction% without making clear 
that it is currently exposed.

Assuming no one else discovers this attack and we obtain no evidence it is 
being exploited, we will need to keep all of these private until the v8 fix 
rolls out in a Chrome release and the fix to Caja then gets rolled out in those 
Google properties that rely on it.

My apologies to all for not having noticed this vulnerability earlier. Thanks.

Original comment by erights@google.com on 19 Feb 2015 at 3:33

[GoogleCodeExporter]

Filed undisclosed https://bugs.webkit.org/show_bug.cgi?id=141865 which says

Title: Object.prototype.__proto__ getter and setter act as sloppy functions, 
coercing to global window

> (0,Object.getOwnPropertyDescriptor(Object.prototype, '__proto__').get)();
TypeError // according to spec, and on all other browsers
WindowPrototype // On Safari

> (0,Object.getOwnPropertyDescriptor(Object.prototype, 
'__proto__').set)(['foo']);
TypeError // according to spec, and on all other browsers
// success on Safari

> window.__proto__
["foo"]

The expressions above invoke the Object.prototype.__proto__ getter and setter 
as functions, i.e., with an undefined this-binding. Sloppy functions 
(non-strict function coded in JavaScript), on entry, coerce an undefined 
this-binding into the global object (window), which they bind to their "this". 
Built-in functions MUST not do this. For non-undefined, non-null primitive 
values, some coerce these using ToObject to a wrapper, i.e.,

> Object.prototype.valueOf.call(3);
Number

but, unlike ES3 or sloppy functions, they must not coerce a null or undefined 
to the global object, i.e.,

> Object.prototype.valueOf.call(void 0);
TypeError

> Object.prototype.valueOf.call(null);
TypeError

As a consequence of this, Caja and SES are vulnerable. Code that is supposed to 
be confined, and therefore must not be able to access the global object, can 
nevertheless manipulate it through these getters and setters. Direct access to 
the WindowPrototype is in any case adequate to completely compromise all the 
security Caja and SES are built to provide, as it is easy to get direct access 
to the DOM from there.

With followup:

Downgrading the importance of this one to Normal, since we have an adequate 
workaround. Replacing the __proto__ getter and setter with wrappers that check 
their "this" works. Currently in undisclosed 
https://codereview.appspot.com/202030043/

Original comment by erights on 22 Feb 2015 at 10:23

[GoogleCodeExporter]

Filed public https://bugs.webkit.org/show_bug.cgi?id=141871 with undisclosed 
followup at https://bugs.webkit.org/show_bug.cgi?id=141878 which says:

Title: Throwing a frozen object opens a capability leak

This is an update on https://bugs.webkit.org/show_bug.cgi?id=141871 , which I 
unfortunately reported publicly as I did not yet understand the buggy behavior, 
nor the danger it creates. During this responsible disclosure period, please 
follow up in this bug rather than that one, so as not to draw attention to the 
actual danger.

Test case:

var o = Object.freeze([]), leak = {}; try { throw o; } catch (_) {}; o.stack = 
leak; console.log(o.stack === leak);

It must return false, and does on all other browsers tested.

It returns true on at least Safari 7.1.3 (9537.85.12.18) and Safari 7.0.5 
(9537.77.4). Please let me know what behavior you see on the latest Safari. 
Thanks.

On Safari's tested, throwing a frozen object adds 'line', 'column', 'sourceURL' 
(not always), and 'stack' properties to it. Although this is a violation of the 
freeze contract, the nature of the added 'line', 'column', and 'sourceURL' 
properties looks like it might not be a severe security problem -- they are 
added as non-writable, non-configurable data properties whose values are 
primitives, and therefore immutable from here forward. This is a bit of an 
information leak, but does not leak capabilities.

But the 'stack' property is added as a writable, configurable data property. 
Though its initial value is a string, one can assign an object to it, as shown 
by the test case above. This allows arbitrary capabilities to be passed between 
compartments that should be isolated from each other, by virtue of sharing only 
transitively frozen objects.

Original comment by erights on 22 Feb 2015 at 10:24

[GoogleCodeExporter]

Original comment by erights@google.com on 24 Feb 2015 at 1:11

• Changed state: Pending

[GoogleCodeExporter]

Fixed in Caja r5717 by
https://codereview.appspot.com/202040043/
https://codereview.appspot.com/202030043/
https://codereview.appspot.com/214110043/

Original comment by erights on 13 Mar 2015 at 7:59

• Changed state: Fixed
• Removed labels: Private