Fixes issue 1623.

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[kpreid2, 2013-01-17 02:53:33]

https://codereview.appspot.com/7134050/diff/1/src/com/google/caja/ses/repairE...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/7134050/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:431: var cleanNewAccessor =
opt_cleanNewAccessor || function(f) {
unnecessary function-expression allocation, hoist it out? But this may go away,
see later comments.

https://codereview.appspot.com/7134050/diff/1/src/com/google/caja/ses/startSE...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7134050/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:822: // for(;;) loop seems better on FF 12
Nightly.
Regarding optimization (please do *not* do this in this CL, but I want to write
down the idea), how about pulling the function(p){...} outside of recur() so it
does not need to be reallocated? The only thing it closes over is val, and
thisArg could be abused for that.

https://codereview.appspot.com/7134050/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:834: tamperProof(val, cleanNewAccessor);
Isn't freezing before recursing valuable for robustness, to ensure that the set
of things iterated over is unchanging in the case of bizarre-but-conformant
object behavior (or, say, Proxies)?

How about, instead of the cleanNewAccessor hook, have a 'cleanHiddenValue' hook
which is called by tamperProof for the value of the accessor, and is bound here
to recur?

In the non-recursive cases before, cleanHiddenValue would be a no-op, and
tamperProof should itself be made responsible for freezing the accessor getter
and setter.

[MarkM, 2013-01-17 05:56:57]

https://codereview.appspot.com/7134050/diff/1/src/com/google/caja/ses/startSE...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7134050/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:834: tamperProof(val, cleanNewAccessor);
On 2013/01/17 02:53:33, kpreid2 wrote:
> Isn't freezing before recursing valuable for robustness, to ensure that the
set
> of things iterated over is unchanging in the case of bizarre-but-conformant
> object behavior (or, say, Proxies)?
> 
> How about, instead of the cleanNewAccessor hook, have a 'cleanHiddenValue'
hook
> which is called by tamperProof for the value of the accessor, and is bound
here
> to recur?

If we call recur as we're iterating over the properties, then we'd still be
calling recur prior to freezing, in opposition to your first point. I think both
your points make sense, and I'll code a variation of this which accumulates the
values in an array, and then after freezing, recurs over the elements of the
array. Costs extra allocation, but anything that deal with your first point
probably has to pay that cost.


> 
> In the non-recursive cases before, cleanHiddenValue would be a no-op, and
> tamperProof should itself be made responsible for freezing the accessor getter
> and setter.

Even in the non-recursive case, startSES should clean these before freezing,
because of the whitelist issue (that I don't yet deal with in this CL). Thus, it
should probably be up to that cleaning function to do the freezing as well.

I'll revisit all this tomorrow after I've slept on it.

[MarkM, 2013-01-20 19:12:48]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[MarkM, 2013-01-21 20:05:12]

new snapshot

[kpreid2, 2013-01-21 20:49:27]

https://codereview.appspot.com/7134050/diff/5001/src/com/google/caja/ses/repa...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/7134050/diff/5001/src/com/google/caja/ses/repa...
src/com/google/caja/ses/repairES5.js:436: var i, j, name; // crockford rule
two spaces before //

https://codereview.appspot.com/7134050/diff/5001/src/com/google/caja/ses/star...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7134050/diff/5001/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:804: if (t === 'number' || t === 'string' ||
t === 'boolean') { return; }
Missing case: t === 'undefined'

How about a hard failure if t is not 'function' or 'object', for
future/(wacky-host-object)-proofing?

If there is any similar type test already in SES, can we extract it to a reused
'isValueWithProperties' function?

https://codereview.appspot.com/7134050/diff/5001/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:819: while (defendingStack.length >= 1) {
I'd rather see > 0. Reason for writing it this way?

https://codereview.appspot.com/7134050/diff/5001/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:948: tamperProof(ArrayLike,
cleanNewAccessor);
Don't these need to be changed? I don't see a definition of cleanNewAccessor any
more. (3 occurrences)

[MarkM, 2013-01-21 21:05:46]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[MarkM, 2013-01-21 21:06:35]

new snapshot

[kpreid2, 2013-01-21 22:46:19]

LGTM

[MarkM, 2013-01-22 03:44:20]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[MarkM, 2013-01-23 02:14:25]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[MarkM, 2013-01-23 02:18:26]

New snapshot. Kevin, please apply your freezeness test


On Mon, Jan 21, 2013 at 2:46 PM, <kpreid.switchb.org@gmail.com> wrote:

> LGTM
>
>
https://codereview.appspot.**com/7134050/<https://codereview.appspot.com/7134...
>



-- 
Text by me above is hereby placed in the public domain

  Cheers,
  --MarkM

[kpreid2, 2013-01-23 19:38:46]

On 2013/01/23 02:18:26, MarkM wrote:
> New snapshot. Kevin, please apply your freezeness test

I find no problems, testing against (r5227 & Patch Set 5 & my exception leak
fix).

[MarkM, 2013-01-23 20:21:57]

Great, thanks.

Felix, as far as I'm concerned, fire when ready ;)


On Wed, Jan 23, 2013 at 11:38 AM, <kpreid.switchb.org@gmail.com> wrote:

> On 2013/01/23 02:18:26, MarkM wrote:
>
>> New snapshot. Kevin, please apply your freezeness test
>>
>
> I find no problems, testing against (r5227 & Patch Set 5 & my exception
> leak fix).
>
>
https://codereview.appspot.**com/7134050/<https://codereview.appspot.com/7134...
>



-- 
Text by me above is hereby placed in the public domain

  Cheers,
  --MarkM

[MarkM, 2013-02-02 23:31:19]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[MarkM, 2013-02-03 06:16:01]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[MarkM, 2013-02-03 06:20:54]

New snapshot.

The getter installed by tamperProof now simply does "return value;". This
makes
http://5238.caja.appspot.com/?es5=true#http://www.thinkfu.com/jq-minimal.html...
faster. And as far as I can tell, does not weaken any
security property. In retrospect, the more complex getter seems pointless.
Does anyone see any problem with this?


On Wed, Jan 23, 2013 at 12:21 PM, Mark Miller <erights@gmail.com> wrote:

> Great, thanks.
>
> Felix, as far as I'm concerned, fire when ready ;)
>
>
> On Wed, Jan 23, 2013 at 11:38 AM, <kpreid.switchb.org@gmail.com> wrote:
>
>> On 2013/01/23 02:18:26, MarkM wrote:
>>
>>> New snapshot. Kevin, please apply your freezeness test
>>>
>>
>> I find no problems, testing against (r5227 & Patch Set 5 & my exception
>> leak fix).
>>
>>
https://codereview.appspot.**com/7134050/<https://codereview.appspot.com/7134...
>>
>
>
>
> --
> Text by me above is hereby placed in the public domain
>
>   Cheers,
>   --MarkM
>



-- 
Text by me above is hereby placed in the public domain

  Cheers,
  --MarkM

[MarkM, 2013-02-03 06:23:29]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[kpreid2, 2013-02-04 18:02:49]

https://codereview.appspot.com/7134050/diff/27002/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7134050/diff/27002/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:829: defendingStack = [];
missing rethrow

[kpreid2, 2013-02-04 18:17:07]

On 2013/02/03 06:20:54, MarkM wrote:
> New snapshot.
> 
> The getter installed by tamperProof now simply does "return value;". This
> makes
> http://5238.caja.appspot.com/?es5=true#http://www.thinkfu.com/jq-minimal.html
> significantly faster. And as far as I can tell, does not weaken any
> security property. In retrospect, the more complex getter seems pointless.
> Does anyone see any problem with this?

I agree that this is OK and the more complex getter is pointless and perhaps
even wrong. For example, consider this 'OO-overriding' getter:

var super = getPropertyDescriptor(Object.getPrototypeOf(foo), 'bar');
Object.defineProperty(foo, 'bar', {
  get: function() {
    var orig = 'value' in super ? super.value : super.get.call(this);
    return orig * 10;
  }
});

[MarkM, 2013-02-04 21:44:02]

In def, we call tamperProof on val before recuring on its property
descriptor. But tamper-proof, due to
http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake
, replaces configurable own data properties on prototypical objects
with getters/setters. The traversal which happens *afterwards*
correctly does not invoke the getter, but rather recurs on the getter
and setter functions themselves.

The fix is to recur before calling tamperProof. However, this would
leave the new getters and setters themselves unfrozen. This fix also
enhances tamperProof to take an optional function argument for
cleaning these new accessor functions. This CL provides freeze as the
cleaning function for now, but really we want to first remove all own
properties from these function instances not allowed by the whitelist.

[MarkM, 2013-02-04 21:45:31]

https://codereview.appspot.com/7134050/diff/27002/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7134050/diff/27002/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:829: defendingStack = [];
On 2013/02/04 18:02:49, kpreid2 wrote:
> missing rethrow

Good, err, catch ;). (Sorry, couldn't resist.)

Done.

[kpreid2, 2013-02-05 00:42:50]

LGTM

[Mark S. Miller, 2013-02-05 02:00:46]

Thanks for the LGTM. Since this is in responsible disclosure, I'm leaving
it to Felix to do the actual commit.


On Mon, Feb 4, 2013 at 4:42 PM, <kpreid.switchb.org@gmail.com> wrote:

> LGTM
>
>
>
https://codereview.appspot.**com/7134050/<https://codereview.appspot.com/7134...
>
> --
> --
> ---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<caja-discuss-undisclo...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>


-- 
    Cheers,
    --MarkM