Issue 202030043: Protect against hidden intrinsics (Closed)
|
Created:
10 years, 2 months ago by MarkM Modified:
10 years, 2 months ago CC:
caja-discuss-undisclosed_googlegroups.com, felix8a, ihab.awad, Jasvir, kpreid2, metaweta, MikeSamuel, pennymac, MarkM Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
Descriptionhttps://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsic-objects
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.
The above summary of the problem is from
https://code.google.com/p/google-caja/issues/detail?id=1953 , which
this CL attempts to fix by generalizing the mechanism we were already
using for %ThrowTypeError%. We add to cajaVM an intrinsics object,
move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to
cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if
it seems to be present, and list all not-otherwise-reachable ES6
instrinsics in the whitelist.
Patch Set 1 #
Total comments: 21
Patch Set 2 : Protect against hidden intrinsics #Patch Set 3 : Protect against hidden intrinsics #
Total comments: 2
Patch Set 4 : Protect against hidden intrinsics #Patch Set 5 : Protect against hidden intrinsics #Patch Set 6 : Protect against hidden intrinsics #Patch Set 7 : Protect against hidden intrinsics #Patch Set 8 : Protect against hidden intrinsics #Patch Set 9 : Protect against hidden intrinsics #Patch Set 10 : Protect against hidden intrinsics #
Total comments: 41
Patch Set 11 : Protect against hidden intrinsics #Patch Set 12 : Protect against hidden intrinsics #Patch Set 13 : Protect against hidden intrinsics #Patch Set 14 : Protect against hidden intrinsics #Patch Set 15 : Protect against hidden intrinsics #Patch Set 16 : Protect against hidden intrinsics #Patch Set 17 : Protect against hidden intrinsics #Patch Set 18 : Protect against hidden intrinsics #
Total comments: 16
Patch Set 19 : Protect against hidden intrinsics #
Total comments: 6
Patch Set 20 : Protect against hidden intrinsics #Patch Set 21 : Protect against hidden intrinsics #
MessagesTotal messages: 45
On 2015/02/15 05:25:46, MarkM wrote:
Kevin, test-scan-guest is still failing due to new coverage gaps. I looked and
don't really understand this code, so I'll need your help on that part.
The new errors:
Coverage gap: Need custom obtainInstance for ctor; threw Error: %TypedArray%
calling/constructing not implemented yet
| Path: cajaVM.intrinsics.TypedArray
| Program: window.cajaVM.intrinsics.TypedArray
| toString: function TypedArray() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.TypedArray.prototype.get buffer
| Program:
Object.getOwnPropertyDescriptor(window.cajaVM.intrinsics.TypedArray.prototype,
"buffer").get
| toString: function buffer() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.TypedArray.prototype.get byteLength
| Program:
Object.getOwnPropertyDescriptor(window.cajaVM.intrinsics.TypedArray.prototype,
"byteLength").get
| toString: function byteLength() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.TypedArray.prototype.get byteOffset
| Program:
Object.getOwnPropertyDescriptor(window.cajaVM.intrinsics.TypedArray.prototype,
"byteOffset").get
| toString: function byteOffset() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.TypedArray.prototype.get length
| Program:
Object.getOwnPropertyDescriptor(window.cajaVM.intrinsics.TypedArray.prototype,
"length").get
| toString: function length() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.TypedArray.prototype.set
| Program: window.cajaVM.intrinsics.TypedArray.prototype.set
| toString: function set() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.TypedArray.prototype.subarray
| Program: window.cajaVM.intrinsics.TypedArray.prototype.subarray
| toString: function subarray() {
[native code]
}
Sign in to reply to this message.
Regarding the scan problems, the following should be mostly sufficient for the new gaps. Please put these lines after the nine setupTypedArray calls: obtainInstance.define(cajaVM.intrinsics.TypedArray, new Int8Array(3)); argsByIdentity(cajaVM.intrinsics.TypedArray, genAllCall()); expectedAlwaysThrow.setByIdentity(cajaVM.intrinsics.TypedArray, true); (The use of Int8Array in particular is arbitrary.) https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:378: * Obtain the ES5 singleton %ThrowTypeError% intrinsic. Words "ES5 singleton" are pointless now https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1375: intrinsics: intrinsics, Add a comment that this is not part of the public API, as was previously present. Unless you mean to make it so. In any case, add a comment explaining what this is for. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1693: * Removes finds all properties to be removed starting at value by Grammar/coherence: "Removes finds all…to be removed…, removing…". https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1707: 'unexpected intrinsic', prefix + '.__proto__'); I note that in principle, we could make this non-fatal by “deleting the property”, i.e. setPrototypeOf(value, null). But that would be fairly useless. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1770: // TODO(kpreid): Is this list complete? Do you happen to know if any new ES6 constructs need this list expanded? https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/white... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:114: // rest start at ES6. s/start at/were introduced in/ https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:128: TypedArray: TypedArrayWhitelist = { // Typed Arrays spec "Typed Arrays spec" is an obsolete comment since ES6 includes typed arrays. https://codereview.appspot.com/202030043/diff/1/tests/com/google/caja/plugin/... File tests/com/google/caja/plugin/test-scan-guest.js (right): https://codereview.appspot.com/202030043/diff/1/tests/com/google/caja/plugin/... tests/com/google/caja/plugin/test-scan-guest.js:558: // TODO test other intrinsics like TypedArray For clarity, please write "%TypedArray%" in this comment, because _typed arrays_ and their immediate ctors are already tested.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
> Regarding the scan problems, the following should be mostly sufficient for the > new gaps. Please put these lines after the nine setupTypedArray calls: > obtainInstance.define(cajaVM.intrinsics.TypedArray, new Int8Array(3)); > argsByIdentity(cajaVM.intrinsics.TypedArray, genAllCall()); > expectedAlwaysThrow.setByIdentity(cajaVM.intrinsics.TypedArray, true); > (The use of Int8Array in particular is arbitrary.) Done, but I haven't yet tested this part. Will test before submitting. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:378: * Obtain the ES5 singleton %ThrowTypeError% intrinsic. On 2015/02/15 21:59:47, kpreid2 wrote: > Words "ES5 singleton" are pointless now Done. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1375: intrinsics: intrinsics, On 2015/02/15 21:59:47, kpreid2 wrote: > Add a comment that this is not part of the public API, as was previously > present. Unless you mean to make it so. I do mean to make it so. > In any case, add a comment explaining what this is for. Done. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1693: * Removes finds all properties to be removed starting at value by On 2015/02/15 21:59:47, kpreid2 wrote: > Grammar/coherence: "Removes finds all…to be removed…, removing…". Done. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1707: 'unexpected intrinsic', prefix + '.__proto__'); On 2015/02/15 21:59:47, kpreid2 wrote: > I note that in principle, we could make this non-fatal by “deleting the > property”, i.e. setPrototypeOf(value, null). But that would be fairly useless. Acknowledged. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1770: // TODO(kpreid): Is this list complete? On 2015/02/15 21:59:47, kpreid2 wrote: > Do you happen to know if any new ES6 constructs need this list expanded? I'm not sure I understand your criteria, but perhaps you mean to include all intrinsics that don't have a global name? In any case, should I add TypedArray here, if it exists? https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/white... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:114: // rest start at ES6. On 2015/02/15 21:59:47, kpreid2 wrote: > s/start at/were introduced in/ Done. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:128: TypedArray: TypedArrayWhitelist = { // Typed Arrays spec On 2015/02/15 21:59:47, kpreid2 wrote: > "Typed Arrays spec" is an obsolete comment since ES6 includes typed arrays. Done. https://codereview.appspot.com/202030043/diff/1/tests/com/google/caja/plugin/... File tests/com/google/caja/plugin/test-scan-guest.js (right): https://codereview.appspot.com/202030043/diff/1/tests/com/google/caja/plugin/... tests/com/google/caja/plugin/test-scan-guest.js:558: // TODO test other intrinsics like TypedArray On 2015/02/15 21:59:47, kpreid2 wrote: > For clarity, please write "%TypedArray%" in this comment, because _typed arrays_ > and their immediate ctors are already tested. Done.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
>> Regarding the scan problems, the following should be mostly sufficient for the >> new gaps. Please put these lines after the nine setupTypedArray calls: >> obtainInstance.define(cajaVM.intrinsics.TypedArray, new Int8Array(3)); >> argsByIdentity(cajaVM.intrinsics.TypedArray, genAllCall()); >> expectedAlwaysThrow.setByIdentity(cajaVM.intrinsics.TypedArray, true); >> (The use of Int8Array in particular is arbitrary.) > Done, but I haven't yet tested this part. Will test before submitting. Tested. Works fine. Thanks.
Sign in to reply to this message.
https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1693: * Removes finds all properties to be removed starting at value by On 2015/02/16 00:32:08, MarkM wrote: > Done. Still says "_Finds_ all properties _to be removed_ starting at …, _removing_ all those...". The beginning sounds like it's purely a search, and then later it says it removes them. How about: Removes all non-whitelisted properties found by recursively and reflectively walking own property chains. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1770: // TODO(kpreid): Is this list complete? On 2015/02/16 00:32:07, MarkM wrote: > On 2015/02/15 21:59:47, kpreid2 wrote: > > Do you happen to know if any new ES6 constructs need this list expanded? > > I'm not sure I understand your criteria, but perhaps you mean to include all > intrinsics that don't have a global name? Restating the criterion: This list contains all objects which cannot be made inaccessible by modifying the reference graph. > In any case, should I add TypedArray > here, if it exists? No, because TypedArray is not accessible by any special syntax: there is no algorithm in the spec which has a step equivalent to "Return %TypedArray%." or "Set the [[Prototype]] of _foo_ to %TypedArray%.". If there were typed array literals, %TypedArray% would not qualify (because it is accessible only via the subclass constructors), but the subclass constructors would. I just checked ES6-draft, and there is at least one object that should be on this list and is not: %Generator%. Just like Function.prototype is accessible from Object.getPrototypeOf(function() {}), this is accessible from Object.getPrototypeOf(function*() {}). Presumably there are more. Any one of these, if not processed by the whitelist, is a NOT_ISOLATED — which is why this self-test exists. Too bad ES6 doesn't have a way to ask for a list of all intrinsics — as it is, SES cannot be safe against future spec additions like these.
Sign in to reply to this message.
https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1693: * Removes finds all properties to be removed starting at value by On 2015/02/16 01:04:55, kpreid2 wrote: > On 2015/02/16 00:32:08, MarkM wrote: > > Done. > > Still says "_Finds_ all properties _to be removed_ starting at …, _removing_ all > those...". The beginning sounds like it's purely a search, and then later it > says it removes them. How about: > > Removes all non-whitelisted properties found by recursively and reflectively > walking own property chains. Done. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1770: // TODO(kpreid): Is this list complete? On 2015/02/16 01:04:55, kpreid2 wrote: > On 2015/02/16 00:32:07, MarkM wrote: > > On 2015/02/15 21:59:47, kpreid2 wrote: > > > Do you happen to know if any new ES6 constructs need this list expanded? > > > > I'm not sure I understand your criteria, but perhaps you mean to include all > > intrinsics that don't have a global name? > > Restating the criterion: This list contains all objects which cannot be made > inaccessible by modifying the reference graph. > > > In any case, should I add TypedArray > > here, if it exists? > > No, because TypedArray is not accessible by any special syntax: there is no > algorithm in the spec which has a step equivalent to "Return %TypedArray%." or > "Set the [[Prototype]] of _foo_ to %TypedArray%.". If there were typed array > literals, %TypedArray% would not qualify (because it is accessible only via the > subclass constructors), but the subclass constructors would. > > > I just checked ES6-draft, and there is at least one object that should be on > this list and is not: %Generator%. Just like Function.prototype is accessible > from Object.getPrototypeOf(function() {}), this is accessible from > Object.getPrototypeOf(function*() {}). OMG, you're right. This is a loss of isolation not fixed by this CL in its present state. Not ready for review until I think I've dealt successfully with this issue. > Presumably there are more. Any one of these, if not processed by the whitelist, > is a NOT_ISOLATED — which is why this self-test exists. > > Too bad ES6 doesn't have a way to ask for a list of all intrinsics — as it is, > SES cannot be safe against future spec additions like these. Indeed. We should propose such a mechanism for ES7.
Sign in to reply to this message.
Forgot one critical thing... https://codereview.appspot.com/202030043/diff/40001/src/com/google/caja/ses/w... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/202030043/diff/40001/src/com/google/caja/ses/w... src/com/google/caja/ses/whitelist.js:117: IteratorPrototype: {}, This part of the whitelist has no effect, because the only properties you put on cajaVM.intrinsics are ThrowTypeError and TypedArray; none of the rest of the intrinsics are present.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
I added all the not-otherwise-named intrinsics except for %MapIteratorPrototype% and %SetIteratorPrototype%. These are clearly not yet an issue since Map and Set are not yet whitelisted. Because the additional ES6 intrinsics are currently in different partial implementation states across browsers, I added them in a way tolerant of such plausible missing steps in the intrinsic graph, but not for finding things I didn't expect. Tested to work on Chrome 41.0.2272.53 beta Chrome 42.0.2305.2 canary FF 35.0.1 FF Nightly 38.0a1 Safari 7.0.5 (9537.77.4) Opera 27.0.1689.69 Not yet tested on IE. Will test there before submitting. https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/202030043/diff/1/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1770: // TODO(kpreid): Is this list complete? On 2015/02/16 01:04:55, kpreid2 wrote: > I just checked ES6-draft, and there is at least one object that should be on > this list and is not: %Generator%. Just like Function.prototype is accessible > from Object.getPrototypeOf(function() {}), this is accessible from > Object.getPrototypeOf(function*() {}). > > Presumably there are more. Any one of these, if not processed by the whitelist, > is a NOT_ISOLATED — which is why this self-test exists. OMG, this was a huge deal. Not only was %GeneratorFunction% not frozen nor processed by the whitelist, as you anticipated. %GeneratorFunction%.__proto__ was the original Function object (!!!) leading to a full arbitrary-code-execution breach of all of SES. Wow. This is now ready for review. https://codereview.appspot.com/202030043/diff/40001/src/com/google/caja/ses/w... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/202030043/diff/40001/src/com/google/caja/ses/w... src/com/google/caja/ses/whitelist.js:117: IteratorPrototype: {}, On 2015/02/16 01:58:14, kpreid2 wrote: > This part of the whitelist has no effect, because the only properties you put on > cajaVM.intrinsics are ThrowTypeError and TypedArray; none of the rest of the > intrinsics are present. Done.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
ant runtests reveals problems, both coverage gaps and some more serious. Thus,
this is again not yet a candidate for submission. Again, would appreciate help
on the coverage gaps. I'll investigate the other problems below.
Coverage gap: No argument generator for function
| Path: cajaVM.intrinsics.GeneratorFunction
| Program: window.cajaVM.intrinsics.GeneratorFunction
| toString: function GeneratorFunction() {
[native code]
}
Problem: Object is extensible
| Path: cajaVM.intrinsics.GeneratorFunction<PLAIN>()
| Program: (0,window.cajaVM.intrinsics.GeneratorFunction)()
| toString: function* anonymous() {
}
Coverage gap: No argument generator for function
| Path: cajaVM.intrinsics.GeneratorFunction<PLAIN>()
| Program: (0,window.cajaVM.intrinsics.GeneratorFunction)()
| toString: function* anonymous() {
}
Problem: Properties are writable: prototype
| Path: cajaVM.intrinsics.GeneratorFunction<PLAIN>()
| Program: (0,window.cajaVM.intrinsics.GeneratorFunction)()
| toString: function* anonymous() {
}
Problem: Object is extensible
| Path: cajaVM.intrinsics.GeneratorFunction<PLAIN>().prototype
| Program: (0,window.cajaVM.intrinsics.GeneratorFunction)().prototype
| toString: [object Object]
Coverage gap: No argument generator for function
| Path: cajaVM.intrinsics.GeneratorFunction.prototype.prototype.next
| Program: window.cajaVM.intrinsics.GeneratorFunction.prototype.prototype.next
| toString: function next() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.GeneratorFunction.prototype.prototype.next
| Program: window.cajaVM.intrinsics.GeneratorFunction.prototype.prototype.next
| toString: function next() {
[native code]
}
Coverage gap: No argument generator for function
| Path: cajaVM.intrinsics.GeneratorFunction.prototype.prototype.throw
| Program: window.cajaVM.intrinsics.GeneratorFunction.prototype.prototype.throw
| toString: function throw() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.GeneratorFunction.prototype.prototype.throw
| Program: window.cajaVM.intrinsics.GeneratorFunction.prototype.prototype.throw
| toString: function throw() {
[native code]
}
Coverage gap: No argument generator for function
| Path: cajaVM.intrinsics.IteratorPrototype.constructor
| Program: window.cajaVM.intrinsics.IteratorPrototype.constructor
| toString: function Iterator() {
[native code]
}
Coverage gap: No non-throwing call
| Path: cajaVM.intrinsics.IteratorPrototype.constructor
| Program: window.cajaVM.intrinsics.IteratorPrototype.constructor
| toString: function Iterator() {
[native code]
}
Sign in to reply to this message.
On Feb 15, 2015, at 21:39, erights@gmail.com wrote: > ant runtests reveals problems, both coverage gaps and some more serious. > Thus, this is again not yet a candidate for submission. Again, would > appreciate help on the coverage gaps. I'll investigate the other > problems below. > > > Coverage gap: No argument generator for function > | Path: cajaVM.intrinsics.GeneratorFunction > | Program: window.cajaVM.intrinsics.GeneratorFunction > | toString: function GeneratorFunction() { > [native code] > } These are not straightforward like the typed array ones -- we actually need to write rules for the scanner to know how to deal with generators. This will be non-trivial. I'll tackle it tomorrow after the holiday. > Problem: Object is extensible > | Path: cajaVM.intrinsics.GeneratorFunction<PLAIN>() > | Program: (0,window.cajaVM.intrinsics.GeneratorFunction)() > | toString: function* anonymous() { > > } This is not actually a serious problem, but the scanner doesn't know that this call returns a _fresh object_ so it assumes the worst. (It assumes this of 'new' invocations, but not regular function calls.) The fix is the argument-generator (oops, terminology collision!) for this call (which currently isn't defined, as warned about above, and we're getting the default) should be wrapped in freshResult(...) to note this. -- Kevin Reid <http://switchb.org/kpreid/>
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
New snapshot. Everything ready for review except the coverage tester. Please do review -- the changes are substantial. Kevin, I'd appreciate it we could pair program the needed coverage tester changes together, with you taking the lead. Thanks.
Sign in to reply to this message.
Do we care about SES on Rhino? It supports E4X, so there's an undeniable XML.prototype. On Thu, Feb 19, 2015 at 7:13 PM, <erights@gmail.com> wrote: > New snapshot. Everything ready for review except the coverage tester. > Please do review -- the changes are substantial. > > Kevin, I'd appreciate it we could pair program the needed coverage > tester changes together, with you taking the lead. Thanks. > > https://codereview.appspot.com/202030043/ > > -- > -- > ---You received this message because you are subscribed to the Google Groups > "caja-discuss-undisclosed" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email to caja-discuss-undisclosed+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. -- Mike Stay - metaweta@gmail.com http://www.cs.auckland.ac.nz/~mike http://reperiendi.wordpress.com
Sign in to reply to this message.
We actually test for e4x (STRICT_E4X_LITERALS_ALLOWED) and refuse to run if it is supported, so we should fail safe there. (Would be good to test of course) Other than that, I don't care about Rhino until someone wants to use SES there. I haven't heard lately about anyone using Rhino for anything. On Thu, Feb 19, 2015 at 7:46 PM, Mike Stay <metaweta@gmail.com> wrote: > Do we care about SES on Rhino? It supports E4X, so there's an > undeniable XML.prototype. > > On Thu, Feb 19, 2015 at 7:13 PM, <erights@gmail.com> wrote: > > New snapshot. Everything ready for review except the coverage tester. > > Please do review -- the changes are substantial. > > > > Kevin, I'd appreciate it we could pair program the needed coverage > > tester changes together, with you taking the lead. Thanks. > > > > https://codereview.appspot.com/202030043/ > > > > -- > > -- > > ---You received this message because you are subscribed to the Google > Groups > > "caja-discuss-undisclosed" group. > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email to caja-discuss-undisclosed+unsubscribe@googlegroups.com. > > For more options, visit https://groups.google.com/d/optout. > > > > -- > Mike Stay - metaweta@gmail.com > http://www.cs.auckland.ac.nz/~mike > http://reperiendi.wordpress.com > > -- > -- > --- > You received this message because you are subscribed to the Google Groups > "caja-discuss-undisclosed" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to caja-discuss-undisclosed+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > -- Cheers, --MarkM
Sign in to reply to this message.
https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:650: // ignore Please add a stronger verification here that the error is occurring because it is bad syntax, not because we've made a typo in refactoring this code. We don't want to accidentally ignore generator intrinsics. Also add a TODO to remove the try once generators are fully deployed in supported browsers. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:656: * The undeniable are the primordial objects are ambiently reachable The undeniable_s_ are the primordial objects _which_ are ambiently… https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:669: var undeniableTuples = [ Add a comment here explaining what the third element of these tuples should be, suitable for someone who is adding a new entry to the list to know whether they're doing it right. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:684: // See http://people.mozilla.org/~jorendorff/figure-2.png This seems like a link likely to break. Give some description so people might find it again, like "See <url> for a diagram of the intrinsic objects related to generators and functions, which may also be found in the specification ..." https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:685: if (!strictArgumentsGenerator) { return; } If there's some less fragile condition we could use here, it'd be nice. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:709: // processing and monkey patching, we call getUndeniables again and …we _will_ call… https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:738: // TODO %MapIteratorPrototype% TODOs to do before submitting https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:743: if ([][iteratorSym] === void 0) { return; } This will exit too early if for some reason [][iteratorSym] does not exist but ''[iteratorSym] does. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:800: return result; How about a self-test that none of the values are undefined? https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3062: var triples = [ call this something less generic like "sourcesOfTTE" https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3383: * property is stray line break https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3400: function test_GENERATORFUNCTION_CANNOT_BE_DENIED() { Why does this need to be a test? Won't clean() fail when it fails to overwrite the property? https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3407: if (!Generator || I think this would be better written as !(... && ...) to make it more like "If the following pattern does not hold" https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:5249: urls: ['https://code.google.com/p/google-caja/issues/detail?id=1953', We don't generally put our own bug URLs in here. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1012: (function() { A comment above here just as a header for this block would be good. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1054: if (!Generator || would rather see !(... && ...) https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1078: // report This a TODO? https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1398: anonIntrinsics: ses.getAnonIntrinsics(), Suggested paranoia: Object.freeze(ses.getAnonIntrinsics()). This will cause whitelisting to fail on attempts to delete properties from anonIntrinsics, as it should. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1779: // The following objects are ambiently available via language Say "These objects" not "The following objects" because we're not listing them explicitly here. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1788: 'Number of undeniables changed'); This will catch what happens if something is in getUndeniables() but is not on the whitelist, correct? I suggest mentioning that.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
Not yet ready for review. Soon. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:650: // ignore On 2015/02/20 22:02:37, kpreid2 wrote: > Please add a stronger verification here that the error is occurring because it > is bad syntax, not because we've made a typo in refactoring this code. We don't > want to accidentally ignore generator intrinsics. > > Also add a TODO to remove the try once generators are fully deployed in > supported browsers. Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:656: * The undeniable are the primordial objects are ambiently reachable On 2015/02/20 22:02:37, kpreid2 wrote: > The undeniable_s_ are the primordial objects _which_ are ambiently… Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:669: var undeniableTuples = [ On 2015/02/20 22:02:37, kpreid2 wrote: > Add a comment here explaining what the third element of these tuples should be, > suitable for someone who is adding a new entry to the list to know whether > they're doing it right. Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:684: // See http://people.mozilla.org/~jorendorff/figure-2.png On 2015/02/20 22:02:37, kpreid2 wrote: > This seems like a link likely to break. Give some description so people might > find it again, like "See <url> for a diagram of the intrinsic objects related to > generators and functions, which may also be found in the specification ..." Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:685: if (!strictArgumentsGenerator) { return; } On 2015/02/20 22:02:37, kpreid2 wrote: > If there's some less fragile condition we could use here, it'd be nice. None comes to mind. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:709: // processing and monkey patching, we call getUndeniables again and On 2015/02/20 22:02:37, kpreid2 wrote: > …we _will_ call… Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:738: // TODO %MapIteratorPrototype% On 2015/02/20 22:02:37, kpreid2 wrote: > TODOs to do before submitting Added explanation https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:743: if ([][iteratorSym] === void 0) { return; } On 2015/02/20 22:02:37, kpreid2 wrote: > This will exit too early if for some reason [][iteratorSym] does not exist but > ''[iteratorSym] does. Trickier than I expected, but done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:800: return result; On 2015/02/20 22:02:37, kpreid2 wrote: > How about a self-test that none of the values are undefined? Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3062: var triples = [ On 2015/02/20 22:02:37, kpreid2 wrote: > call this something less generic like "sourcesOfTTE" Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3383: * property is On 2015/02/20 22:02:37, kpreid2 wrote: > stray line break Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3400: function test_GENERATORFUNCTION_CANNOT_BE_DENIED() { On 2015/02/20 22:02:37, kpreid2 wrote: > Why does this need to be a test? Won't clean() fail when it fails to overwrite > the property? This way, the failure, which we know to expect on some versions of some browsers, gets reported from our test and repair framework, which gives a better diagnostic experience. But yes, currently it also fails during clean, which I don't really like. I'm inclined to leave that as is for this CL though. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3407: if (!Generator || On 2015/02/20 22:02:37, kpreid2 wrote: > I think this would be better written as !(... && ...) to make it more like "If > the following pattern does not hold" Done. Amazing how much better that is. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:5249: urls: ['https://code.google.com/p/google-caja/issues/detail?id=1953', On 2015/02/20 22:02:37, kpreid2 wrote: > We don't generally put our own bug URLs in here. We do, and I don't see any reason not to. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1012: (function() { On 2015/02/20 22:02:38, kpreid2 wrote: > A comment above here just as a header for this block would be good. Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1054: if (!Generator || On 2015/02/20 22:02:38, kpreid2 wrote: > would rather see !(... && ...) Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1078: // report On 2015/02/20 22:02:38, kpreid2 wrote: > This a TODO? Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1078: // report On 2015/02/20 22:02:38, kpreid2 wrote: > This a TODO? Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1398: anonIntrinsics: ses.getAnonIntrinsics(), On 2015/02/20 22:02:38, kpreid2 wrote: > Suggested paranoia: Object.freeze(ses.getAnonIntrinsics()). This will cause > whitelisting to fail on attempts to delete properties from anonIntrinsics, as it > should. Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1779: // The following objects are ambiently available via language On 2015/02/20 22:02:38, kpreid2 wrote: > Say "These objects" not "The following objects" because we're not listing them > explicitly here. Done. https://codereview.appspot.com/202030043/diff/180001/src/com/google/caja/ses/... src/com/google/caja/ses/startSES.js:1788: 'Number of undeniables changed'); On 2015/02/20 22:02:38, kpreid2 wrote: > This will catch what happens if something is in getUndeniables() but is not on > the whitelist, correct? I suggest mentioning that. The point was actually a bit different. Explained.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
New snapshot with repair for https://bugs.webkit.org/show_bug.cgi?id=141865 , where Object.prototype.__proto__ getter and setter act as sloppy functions, coercing an undefined this-binding to global window. On browsers suffering from https://code.google.com/p/v8/issues/detail?id=3902 (Chrome 40 and 41.0.2272.64), changed startSES.js so that it proceeds despite this error after insuring that severity has been raised to at least NOT_ISOLATED, so that further testing can occur. As of this snapshot, Chrome 40 and 41.0.2272.64 both fail test-scan-guest with Coverage gap: No non-throwing call | Path: Object<CONSTRUCT>(dummyFunction).get arguments | Program: Object.getOwnPropertyDescriptor(new (window.Object)(.../* dummyFunction */), "arguments").get | toString: function ThrowTypeError() { [native code] } Problem: Object is extensible | Path: Object<CONSTRUCT>(dummyFunction).get arguments.prototype | Program: Object.getOwnPropertyDescriptor(new (window.Object)(.../* dummyFunction */), "arguments").get.prototype | toString: [object Object] Problem: Properties are writable and/or configurable: constructor | Path: Object<CONSTRUCT>(dummyFunction).get arguments.prototype | Program: Object.getOwnPropertyDescriptor(new (window.Object)(.../* dummyFunction */), "arguments").get.prototype | toString: [object Object] As of this snapshot, Safari 7.0.5 (9537.77.4) fails test-scan-guest with Problem: Object is not frozen | Path: cajaVM.GuardT.coerce<THIS>(dummyFunction,function (v) { throw Object.freeze([ 'ejectorArg', v ]); }) thrown | Program: (function(){try{ window.cajaVM.GuardT.coerce.call(.../* [object Object] */, .../* dummyFunction */,.../* function (v) { throw Object.freeze([ 'ejectorArg', v ]); } */) }catch(e){return e}}()) | toString: ejectorArg,Specimen does not have the "Guard" trademark Problem: Properties are writable and/or configurable: stack | Path: cajaVM.GuardT.coerce<THIS>(dummyFunction,function (v) { throw Object.freeze([ 'ejectorArg', v ]); }) thrown | Program: (function(){try{ window.cajaVM.GuardT.coerce.call(.../* [object Object] */, .../* dummyFunction */,.../* function (v) { throw Object.freeze([ 'ejectorArg', v ]); } */) }catch(e){return e}}()) | toString: ejectorArg,Specimen does not have the "Guard" trademark ....and more similar problems, ending in | Program: (function(){try{ new (window.cajaVM.Trademark)("NaN").guard.coerce.call(.../* [object Object] */, .../* [object Object] */,.../* function (v) { throw Object.freeze([ 'ejectorArg', v ]); } */) }catch(e){return e}}()) | toString: ejectorArg,Specimen does not have the "NaN" trademark
Sign in to reply to this message.
As of this snapshot, FF Nightly 38.0a1 (2015-02-20) fails test-scan-guest with
Coverage gap: No non-throwing call
| Path: cajaVM.anonIntrinsics.GeneratorFunction.prototype.prototype.throw
| Program:
window.cajaVM.anonIntrinsics.GeneratorFunction.prototype.prototype.throw
| toString: function throw() {
[native code]
}
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:642: * uncondition in the test of the code. unconditional https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:677: put // on these blank lines https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3509: * Detects https://bugs.webkit.org/show_bug.cgi?id=141865 Change this paragraph and the one for the below function to make it clearer that this is for the _getter_ and that is for the _setter_ only. I initially read this as two copies of the same function, missing the one letter difference. https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3573: * mutations. It makes the object claim to be unfrozen _and extends it_ with new properties named 'line', 'column', and 'stack'. https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:4240: * Repairs both getter and setter. If either are vulnerable, I don't Please rename the function, like maybe repair_UNDERBAR_PROTO_accessors_USES_GLOBAL (the lowercase to mark the irregularity). https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:5507: preSeverity: severities.SAFE_SPEC_VIOLATION, This is not a SAFE_SPEC_VIOLATION, because it allows extending non-extensibles; but it is only an UNSAFE_SPEC_VIOLATION because the added properties are pure data. Maybe we could put it on the acceptable problems (NO_KNOWN_EXPLOIT_SPEC_VIOLATION) list, but we'd need to think about whether we expose any table-like objects whose extension could be dangerous. https://codereview.appspot.com/202030043/diff/340001/tests/com/google/caja/pl... File tests/com/google/caja/plugin/test-scan-guest.js (right): https://codereview.appspot.com/202030043/diff/340001/tests/com/google/caja/pl... tests/com/google/caja/plugin/test-scan-guest.js:577: obtainInstance.define(cajaVM.anonIntrinsics.GeneratorFunction, Please apply this patch. @@ -574,10 +574,12 @@ var aGeneratorFunction = directAccess.evalInGuestFrame( '(function*() { "use strict"; ' + 'while (true) { try { yield 1; } catch (e) {} } })'); + var aGenerator = aGeneratorFunction(); + aGenerator.next(); // so .throw() will be caught obtainInstance.define(cajaVM.anonIntrinsics.GeneratorFunction, aGeneratorFunction); obtainInstance.define(cajaVM.anonIntrinsics.GeneratorFunction.prototype, - aGeneratorFunction()); + aGenerator); var generatorIteratorPrototype = cajaVM.anonIntrinsics.GeneratorFunction.prototype.prototype; @@ -591,7 +593,7 @@ Ref.is(generatorIteratorPrototype['throw'])), // fresh because it returns { value: ..., done: ... } freshResult(G.any( - genAllCall(), genMethod(), genMethod(genSmallInteger)))); + genMethod(G.value('foo')), genMethod()))); } argsByIdentity(cajaVM.anonIntrinsics.ThrowTypeError, genAllCall());
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
New snapshot. I am not including a mitigation for the new severe undisclosed Safari problem https://bugs.webkit.org/show_bug.cgi?id=141878 (See THROWING_THAWS_FROZEN_OBJECT in this CL), pending two pieces of info: * Does Safari 8 still suffer from this? * If SES fails to initialize on Safari 7, will our current users still safely fall back to ES5/3 mode? Can we afford to have them do so? Regarding the test-scan-guest failure in Chrome 41, I will still investigate, but my current hypothesis is that this will go away once GENERATORFUNCTION_CANNOT_BE_DENIED is fixed on Chrome 41. https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:642: * uncondition in the test of the code. On 2015/02/22 01:21:51, kpreid2 wrote: > unconditional Done. https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:677: On 2015/02/22 01:21:51, kpreid2 wrote: > put // on these blank lines Done. https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3509: * Detects https://bugs.webkit.org/show_bug.cgi?id=141865 On 2015/02/22 01:21:51, kpreid2 wrote: > Change this paragraph and the one for the below function to make it clearer that > this is for the _getter_ and that is for the _setter_ only. I initially read > this as two copies of the same function, missing the one letter difference. Done. https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3573: * mutations. On 2015/02/22 01:21:51, kpreid2 wrote: > It makes the object claim to be unfrozen _and extends it_ with new properties > named 'line', 'column', and 'stack'. Somehow I misunderstood didn't realize that the object actually is modified. Turns out this is worse than an information leak -- the added 'stack' property is writable/configurable, and can be assigned an actual object. This goes back to NOT_ISOLATED. PTAL -- very much. https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:4240: * Repairs both getter and setter. If either are vulnerable, I don't On 2015/02/22 01:21:51, kpreid2 wrote: > Please rename the function, like maybe > repair_UNDERBAR_PROTO_accessors_USES_GLOBAL (the lowercase to mark the > irregularity). Done. Unfortunately, the plural made "USES" go to "USE". https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:5507: preSeverity: severities.SAFE_SPEC_VIOLATION, On 2015/02/22 01:21:51, kpreid2 wrote: > This is not a SAFE_SPEC_VIOLATION, because it allows extending non-extensibles; > but it is only an UNSAFE_SPEC_VIOLATION because the added properties are pure > data. > > Maybe we could put it on the acceptable problems > (NO_KNOWN_EXPLOIT_SPEC_VIOLATION) list, but we'd need to think about whether we > expose any table-like objects whose extension could be dangerous. The problem turned out to be much worse, so this is not NOT_ISOLATED. Reported at https://bugs.webkit.org/show_bug.cgi?id=141878 https://codereview.appspot.com/202030043/diff/340001/tests/com/google/caja/pl... File tests/com/google/caja/plugin/test-scan-guest.js (right): https://codereview.appspot.com/202030043/diff/340001/tests/com/google/caja/pl... tests/com/google/caja/plugin/test-scan-guest.js:577: obtainInstance.define(cajaVM.anonIntrinsics.GeneratorFunction, Done. But on FF Nightly I still get: Coverage gap: No non-throwing call | Path: cajaVM.anonIntrinsics.GeneratorFunction.prototype.prototype.throw | Program: window.cajaVM.anonIntrinsics.GeneratorFunction.prototype.prototype.throw | toString: function throw() { [native code] } Is this the case you expected that patch to fix?
Sign in to reply to this message.
https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/202030043/diff/340001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3509: * Detects https://bugs.webkit.org/show_bug.cgi?id=141865 On 2015/02/22 21:55:21, MarkM wrote: > Done. I meant the "detects" paragraph. Absent qualifications, it implies that it is solely responsible for detecting the bug. Something like "Detects half of" would be better. https://codereview.appspot.com/202030043/diff/340001/tests/com/google/caja/pl... File tests/com/google/caja/plugin/test-scan-guest.js (right): https://codereview.appspot.com/202030043/diff/340001/tests/com/google/caja/pl... tests/com/google/caja/plugin/test-scan-guest.js:577: obtainInstance.define(cajaVM.anonIntrinsics.GeneratorFunction, On 2015/02/22 21:55:21, MarkM wrote: > Done. But on FF Nightly I still get: > > Coverage gap: No non-throwing call > | Path: cajaVM.anonIntrinsics.GeneratorFunction.prototype.prototype.throw > Is this the case you expected that patch to fix? No, this is something I already saw which is strange and I do not understand. The scanner is not acting as it ought to. I will need to investigate further, but I do not think it indicates a Caja bug. https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3571: * in the object incomplete https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3588: return 'Throwing changed properties: ' + newNames; Aren't we expecting the length to increase, since o did not start with 'stack' etc.? This test makes the below for loop code moot. https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3591: if (oldNames.indexOf(newNames[i]) === -1) { This is testing a property was gained, not lost.
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3571: * in the object On 2015/02/23 22:26:57, kpreid2 wrote: > incomplete Done. https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3588: return 'Throwing changed properties: ' + newNames; On 2015/02/23 22:26:57, kpreid2 wrote: > Aren't we expecting the length to increase, since o did not start with 'stack' > etc.? This test makes the below for loop code moot. No, this is intended, but I can see why it is confusing. Added a comment to clarify. https://codereview.appspot.com/202030043/diff/360001/src/com/google/caja/ses/... src/com/google/caja/ses/repairES5.js:3591: if (oldNames.indexOf(newNames[i]) === -1) { On 2015/02/23 22:26:57, kpreid2 wrote: > This is testing a property was gained, not lost. What I was checking for was actually quite silly: If the lengths are the same, it doesn't necessarily mean that the property names are the same. If not, it means that some property was lost while another was gained. Without other evidence, this is sufficiently unlikely that it is silly to add code for this. The loop is gone.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-intrinsi... 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. The above summary of the problem is from https://code.google.com/p/google-caja/issues/detail?id=1953 , which this CL attempts to fix by generalizing the mechanism we were already using for %ThrowTypeError%. We add to cajaVM an intrinsics object, move %ThrowTypeError% from cajaVM['[[ThrowTypeError]]'] to cajaVM.instrinsics.ThrowTypeError, add cajaVM.intrinsics.TypedArray if it seems to be present, and list all not-otherwise-reachable ES6 instrinsics in the whitelist.
Sign in to reply to this message.
New snapshot in response to https://code.google.com/p/google-caja/issues/detail?id=1956 PTAL
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
