Nuking misleading properties in `Object.getOwnPropertyDescriptor`

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Nathan Wall
Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from))
            Object.defineProperty(to, name,
                Object.getOwnPropertyDescriptor(from, name));
    }

If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.

    Object.prototype.get = function() { };

    var x = { foo: 1 };
    var y = { };
    copy(x, y); // TypeError: property descriptors must not specify a value or
                // be writable when a getter or setter has been specified

It's misleading that you can't trust `getOwnPropertyDescriptor` to give you a descriptor which you can use to define a property with `defineProperty`.

A script which doesn't use `defineProperty` could very well define a `get` property on `Object.prototype`, unknowingly breaking other scripts which do use `defineProperty`. This is a hazard in the language.

I think one of the following would be an appropriate solution:

(1) "Nuke" the special properties (`get`, `set`, and `value` when any of them is not defined) on a descriptor returned by `getOwnPropertyDescriptor` which shouldn't be inherited through the descriptor's prototype. By "nuke" them, I mean specify that they be defined as `undefined`, much like `callee` and `caller` in strict mode. (I don't think strict mode is required in this case, though, since I highly doubt anyone could be relying on this behavior.)

(2) Use the introduction of `Reflect.defineProperty` as an opportunity to "fix" this function to only inspect own properties on the property descriptor so that the descriptor's prototype doesn't matter. This is similar to the addition of `Number.isNaN` to fix the broken `isNaN`.

Nathan


[1] https://mail.mozilla.org/pipermail/es-discuss/2012-November/026705.html     
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

RE: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Nathan Wall
> (1) "Nuke" the special properties (`get`, `set`, and `value` when any of them is not defined) on a descriptor returned by `getOwnPropertyDescriptor` which shouldn't be inherited through the descriptor's prototype. By "nuke" them, I mean specify that they be defined as `undefined`, much like `callee` and `caller` in strict mode. (I don't think strict mode is required in this case, though, since I highly doubt anyone could be relying on this behavior.)

Naturally, `defineProperty` would also have to be rewritten a little to ignore properties set to `undefined`. If `get`, `set`, and `value` are all set to `undefined`, then `value` would be assumed to be `undefined`. In addition, `writable` would also have to be nuked for accessor descriptors.  I think it's workable?

Nathan    
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Andrea Giammarchi-2
In reply to this post by Nathan Wall
for safe definition and more, have a look into redefine

br


On Sat, Mar 9, 2013 at 7:54 PM, Nathan Wall <[hidden email]> wrote:
Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from))
            Object.defineProperty(to, name,
                Object.getOwnPropertyDescriptor(from, name));
    }

If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.

    Object.prototype.get = function() { };

    var x = { foo: 1 };
    var y = { };
    copy(x, y); // TypeError: property descriptors must not specify a value or
                // be writable when a getter or setter has been specified

It's misleading that you can't trust `getOwnPropertyDescriptor` to give you a descriptor which you can use to define a property with `defineProperty`.

A script which doesn't use `defineProperty` could very well define a `get` property on `Object.prototype`, unknowingly breaking other scripts which do use `defineProperty`. This is a hazard in the language.

I think one of the following would be an appropriate solution:

(1) "Nuke" the special properties (`get`, `set`, and `value` when any of them is not defined) on a descriptor returned by `getOwnPropertyDescriptor` which shouldn't be inherited through the descriptor's prototype. By "nuke" them, I mean specify that they be defined as `undefined`, much like `callee` and `caller` in strict mode. (I don't think strict mode is required in this case, though, since I highly doubt anyone could be relying on this behavior.)

(2) Use the introduction of `Reflect.defineProperty` as an opportunity to "fix" this function to only inspect own properties on the property descriptor so that the descriptor's prototype doesn't matter. This is similar to the addition of `Number.isNaN` to fix the broken `isNaN`.

Nathan


[1] https://mail.mozilla.org/pipermail/es-discuss/2012-November/026705.html
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Herby Vojčík
In reply to this post by Nathan Wall
Yes, this was in this list some three-four months ago. There were
answers like "JS is this way dynamic, playing with Object.prototype hurts".

I proposed that return value from Object.getOwnPropertyDescriptor should
inherit from well well-known frozen object with all the default set.
Back, then no-one seemed to be interested.

Herby

Nathan Wall wrote:

> Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:
>
>      function copy(from, to) {
>          for (let name of Object.getOwnPropertyNames(from))
>              Object.defineProperty(to, name,
>                  Object.getOwnPropertyDescriptor(from, name));
>      }
>
> If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.
>
>      Object.prototype.get = function() { };
>
>      var x = { foo: 1 };
>      var y = { };
>      copy(x, y); // TypeError: property descriptors must not specify a value or
>                  // be writable when a getter or setter has been specified
>
> It's misleading that you can't trust `getOwnPropertyDescriptor` to give you a descriptor which you can use to define a property with `defineProperty`.
>
> A script which doesn't use `defineProperty` could very well define a `get` property on `Object.prototype`, unknowingly breaking other scripts which do use `defineProperty`. This is a hazard in the language.
>
> I think one of the following would be an appropriate solution:
>
> (1) "Nuke" the special properties (`get`, `set`, and `value` when any of them is not defined) on a descriptor returned by `getOwnPropertyDescriptor` which shouldn't be inherited through the descriptor's prototype. By "nuke" them, I mean specify that they be defined as `undefined`, much like `callee` and `caller` in strict mode. (I don't think strict mode is required in this case, though, since I highly doubt anyone could be relying on this behavior.)
>
> (2) Use the introduction of `Reflect.defineProperty` as an opportunity to "fix" this function to only inspect own properties on the property descriptor so that the descriptor's prototype doesn't matter. This is similar to the addition of `Number.isNaN` to fix the broken `isNaN`.
>
> Nathan
>
>
> [1] https://mail.mozilla.org/pipermail/es-discuss/2012-November/026705.html   
> _______________________________________________
> es-discuss mailing list
> [hidden email]
> https://mail.mozilla.org/listinfo/es-discuss
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Andrea Giammarchi-2
there was another discussion here, is not just getOwnPropertyDescriptor problem: https://mail.mozilla.org/pipermail/es-discuss/2012-November/026705.html


On Sun, Mar 10, 2013 at 4:37 AM, Herby Vojčík <[hidden email]> wrote:
Yes, this was in this list some three-four months ago. There were answers like "JS is this way dynamic, playing with Object.prototype hurts".

I proposed that return value from Object.getOwnPropertyDescriptor should inherit from well well-known frozen object with all the default set. Back, then no-one seemed to be interested.

Herby


Nathan Wall wrote:
Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:

     function copy(from, to) {
         for (let name of Object.getOwnPropertyNames(from))
             Object.defineProperty(to, name,
                 Object.getOwnPropertyDescriptor(from, name));
     }

If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.

     Object.prototype.get = function() { };

     var x = { foo: 1 };
     var y = { };
     copy(x, y); // TypeError: property descriptors must not specify a value or
                 // be writable when a getter or setter has been specified

It's misleading that you can't trust `getOwnPropertyDescriptor` to give you a descriptor which you can use to define a property with `defineProperty`.

A script which doesn't use `defineProperty` could very well define a `get` property on `Object.prototype`, unknowingly breaking other scripts which do use `defineProperty`. This is a hazard in the language.

I think one of the following would be an appropriate solution:

(1) "Nuke" the special properties (`get`, `set`, and `value` when any of them is not defined) on a descriptor returned by `getOwnPropertyDescriptor` which shouldn't be inherited through the descriptor's prototype. By "nuke" them, I mean specify that they be defined as `undefined`, much like `callee` and `caller` in strict mode. (I don't think strict mode is required in this case, though, since I highly doubt anyone could be relying on this behavior.)

(2) Use the introduction of `Reflect.defineProperty` as an opportunity to "fix" this function to only inspect own properties on the property descriptor so that the descriptor's prototype doesn't matter. This is similar to the addition of `Number.isNaN` to fix the broken `isNaN`.

Nathan


[1] https://mail.mozilla.org/pipermail/es-discuss/2012-November/026705.html                                    
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Tom Van Cutsem-3
In reply to this post by Nathan Wall
Hi Nathan,

2013/3/10 Nathan Wall <[hidden email]>
Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from))
            Object.defineProperty(to, name,
                Object.getOwnPropertyDescriptor(from, name));
    }

If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.

To my mind, the blame for the breakage lies with `Object.prototype` being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO.

(2) Use the introduction of `Reflect.defineProperty` as an opportunity to "fix" this function to only inspect own properties on the property descriptor so that the descriptor's prototype doesn't matter. This is similar to the addition of `Number.isNaN` to fix the broken `isNaN`.

Apart from the fact that Reflect.defineProperty returns a boolean success value rather than throwing or returning normally, I would not be in favor of changing the semantics of the Reflect.* functions much from the Object.* functions. Your suggestion is reasonable, but I wonder if the inconsistent behavior it introduces isn't going to cause more confusion than the problem it intends to fix in the first place.

Cheers,
Tom

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

David Bruant-5
Le 12/03/2013 16:45, Tom Van Cutsem a écrit :
Hi Nathan,

2013/3/10 Nathan Wall <[hidden email]>
Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from))
            Object.defineProperty(to, name,
                Object.getOwnPropertyDescriptor(from, name));
    }

If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.

To my mind, the blame for the breakage lies with `Object.prototype` being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO.
I agree.

As Object.prototype-jacking threats are discussed more and more recently, I'd like to take a step back and "meta-discuss" JavaScript threats.

Currently, by default, any script that run can mutate the environment it is executed in (it can be fixed by sandboxing with things like Caja [1] and soon the module loader API used with proxies [2], but even then, there could be leaks of native built-ins).
The first (security) decision any JavaScript application should make would be to freeze all built-ins like SES [3][4] does. (In the future, it could even make sense to add a CSP [5] directive for that)
If necessary, the application can first enhance the environment by adding polyfills/libraries and such, but that's pretty much the only thing that's acceptable to run before freezing everything.

Given that freezing all built-ins (after polyfills) is a reasonable thing to do, I think JavaScript threat should be considered serious only if applicable assuming the environment is already frozen.
It naturally rules out threats related to property descriptors inheriting from Object.prototype or anything looking like "what if an attacker switches Array.prototype.push and Array.prototype.pop?"

David

[1] http://code.google.com/p/google-caja/
[2] http://wiki.ecmascript.org/doku.php?id=harmony:module_loaders
[3] http://code.google.com/p/es-lab/wiki/SecureEcmaScript
[4] http://code.google.com/p/es-lab/source/browse/#svn%2Ftrunk%2Fsrc%2Fses
[5] https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

RE: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Nathan Wall
David Bruant wrote:

> Tom Van Cutsem wrote:
> > To my mind, the blame for the breakage lies with `Object.prototype` 
> > being mutated by the third-party script, not with property descriptors 
> > inheriting from Object.prototype. Thus, a fix for the breakage should 
> > address that directly, rather than tweaking the design of property 
> > descriptors, IMHO. 
> I agree. 

> The first (security) decision any JavaScript application should make 
> would be to freeze all built-ins like SES [3][4] does. (In the future, 
> it could even make sense to add a CSP [5] directive for that) 
> If necessary, the application can first enhance the environment by 
> adding polyfills/libraries and such, but that's pretty much the only 
> thing that's acceptable to run before freezing everything. 

Hey David and Tom.  This is good advice for application authors, but I don't work at the application level; I write libraries.  I don't want to freeze everything because I want to leave the environment open to monkey-patching and shimming by other libraries and the application authors. So this isn't an option for me.

> "what if an attacker switches Array.prototype.push and Array.prototype.pop?"

These are issues that are easy to address by using stored late-bound function references rather than methods and array-likes instead of true arrays.

    var push = Function.prototype.call.bind(Array.prototype.push),
        arrayLike = Object.create(null);
    arrayLike.length = 0;
    push(arrayLike, 'item-1');

As long as the environment is correct when my script initializes, I get all methods I need to use stored inside my library's closure. Freezing isn't needed.

It's also possible to write around the `defineProperty` problem by converting the descriptor into a prototype-less object. However, I actually encountered some performance problems with this. I was able to improve the performance by only dropping the prototype when necessary (as long as `get`, `set`, `value` or `writable` haven't been added to `Object.prototype`, it's not necessary). However, as a matter of principle, my argument is that `Object.getOwnPropertyDescriptor` should, at the bare minimum, return a descriptor that can be known to work in `Object.defineProperty`.  If `Object.defineProperty` doesn't accept it, then you `getOwnPropertyDescriptor` didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like `Object.prototype.get`, (2) is a potential stumbling block, (3) has no real benefit -- really, there's not anything positive about this behavior, and (4) forces developers who want to support `Object.prototype.get` to add an extra layer of cleaning before using `defineProperty`.

Nathan    
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Mark S. Miller-2
In reply to this post by David Bruant-5
The one qualification everyone should be aware of is that if they simply freeze these themselves, rather than using the tamper-proofing abstractions defined by SES[1][2], then they will suffer from the override mistake[3] and conventional code such as the following will break:

  function Point(x, y) {
    this.x = x;
    this.y = y;
  }
  Point.prototype.toString = function() {
    return '<' + x + ',' + y + '>';
  };

This normal looking assignment to Point.prototype.toString fails because Point.prototype inherits from Object.prototype, on which toString is a non-writable non-configurable data property.





On Wed, Mar 13, 2013 at 7:18 AM, David Bruant <[hidden email]> wrote:
Le 12/03/2013 16:45, Tom Van Cutsem a écrit :
Hi Nathan,

2013/3/10 Nathan Wall <[hidden email]>
Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from))
            Object.defineProperty(to, name,
                Object.getOwnPropertyDescriptor(from, name));
    }

If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.

To my mind, the blame for the breakage lies with `Object.prototype` being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO.
I agree.

As Object.prototype-jacking threats are discussed more and more recently, I'd like to take a step back and "meta-discuss" JavaScript threats.

Currently, by default, any script that run can mutate the environment it is executed in (it can be fixed by sandboxing with things like Caja [1] and soon the module loader API used with proxies [2], but even then, there could be leaks of native built-ins).
The first (security) decision any JavaScript application should make would be to freeze all built-ins like SES [3][4] does. (In the future, it could even make sense to add a CSP [5] directive for that)
If necessary, the application can first enhance the environment by adding polyfills/libraries and such, but that's pretty much the only thing that's acceptable to run before freezing everything.

Given that freezing all built-ins (after polyfills) is a reasonable thing to do, I think JavaScript threat should be considered serious only if applicable assuming the environment is already frozen.
It naturally rules out threats related to property descriptors inheriting from Object.prototype or anything looking like "what if an attacker switches Array.prototype.push and Array.prototype.pop?"

David

[1] http://code.google.com/p/google-caja/
[2] http://wiki.ecmascript.org/doku.php?id=harmony:module_loaders
[3] http://code.google.com/p/es-lab/wiki/SecureEcmaScript
[4] http://code.google.com/p/es-lab/source/browse/#svn%2Ftrunk%2Fsrc%2Fses
[5] https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html



--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Mark S. Miller-2
In reply to this post by Nathan Wall
On Wed, Mar 13, 2013 at 8:26 AM, Nathan Wall <[hidden email]> wrote:
David Bruant wrote:
> Tom Van Cutsem wrote:
> > To my mind, the blame for the breakage lies with `Object.prototype` 
> > being mutated by the third-party script, not with property descriptors 
> > inheriting from Object.prototype. Thus, a fix for the breakage should 
> > address that directly, rather than tweaking the design of property 
> > descriptors, IMHO. 
> I agree. 

> The first (security) decision any JavaScript application should make 
> would be to freeze all built-ins like SES [3][4] does. (In the future, 
> it could even make sense to add a CSP [5] directive for that) 
> If necessary, the application can first enhance the environment by 
> adding polyfills/libraries and such, but that's pretty much the only 
> thing that's acceptable to run before freezing everything. 

Hey David and Tom.  This is good advice for application authors, but I don't work at the application level; I write libraries.  I don't want to freeze everything because I want to leave the environment open to monkey-patching and shimming by other libraries and the application authors. So this isn't an option for me.

> "what if an attacker switches Array.prototype.push and Array.prototype.pop?"

These are issues that are easy to address by using stored late-bound function references rather than methods and array-likes instead of true arrays.

    var push = Function.prototype.call.bind(Array.prototype.push),
        arrayLike = Object.create(null);
    arrayLike.length = 0;
    push(arrayLike, 'item-1');

As long as the environment is correct when my script initializes, I get all methods I need to use stored inside my library's closure. Freezing isn't needed.

That's correct. I've written a bit of code like that myself. At first, for those used to JS or even just conventional oo, the style needed is very counter-intuitive and awkward -- it goes against the grain of what the language tries to make convenient. But amazingly, it is possible, and one even gets used to it after a while.

We need a name for your use case. It is definitely distinct from the normal JS library writing practice, which implicitly assumes that the primordials haven't been too corrupted but does nothing to detect if this is true. And distinct from the opposite extreme -- the SES library practice, which may rely on the primordials having been made incorruptible. Amusingly, these extremes resemble each other much more than they resemble your middle ground.

For your use case, your analysis of the defineProperty problem is indeed valid. To date, few if any language design decisions have purposely been made to accommodate this use case, although some language changes have helped anyway. I think perhaps the shortest path towards anything like the defineProperty reform you seek is to argue first why this use case is important.


 

It's also possible to write around the `defineProperty` problem by converting the descriptor into a prototype-less object. However, I actually encountered some performance problems with this. I was able to improve the performance by only dropping the prototype when necessary (as long as `get`, `set`, `value` or `writable` haven't been added to `Object.prototype`, it's not necessary). However, as a matter of principle, my argument is that `Object.getOwnPropertyDescriptor` should, at the bare minimum, return a descriptor that can be known to work in `Object.defineProperty`.  If `Object.defineProperty` doesn't accept it, then you `getOwnPropertyDescriptor` didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like `Object.prototype.get`, (2) is a potential stumbling block, (3) has no real benefit -- really, there's not anything positive about this behavior, and (4) forces developers who want to support `Object.prototype.get` to add an extra layer of cleaning before using `defineProperty`.

Nathan
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss



--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

David Bruant-5
In reply to this post by Nathan Wall
Le 13/03/2013 16:26, Nathan Wall a écrit :

> David Bruant wrote:
>> Tom Van Cutsem wrote:
>>> To my mind, the blame for the breakage lies with `Object.prototype`
>>> being mutated by the third-party script, not with property descriptors
>>> inheriting from Object.prototype. Thus, a fix for the breakage should
>>> address that directly, rather than tweaking the design of property
>>> descriptors, IMHO.
>> I agree.
>>  
>> The first (security) decision any JavaScript application should make
>> would be to freeze all built-ins like SES [3][4] does. (In the future,
>> it could even make sense to add a CSP [5] directive for that)
>> If necessary, the application can first enhance the environment by
>> adding polyfills/libraries and such, but that's pretty much the only
>> thing that's acceptable to run before freezing everything.
> Hey David and Tom.  This is good advice for application authors, but I don't work at the application level; I write libraries.  I don't want to freeze everything because I want to leave the environment open to monkey-patching and shimming by other libraries and the application authors. So this isn't an option for me.
Interesting.
As a library author, in theory, you don't know in which environment your
code is going to be executed (that is maybe the environment has been
modified or not), so I think one assumption has to be made:
Either you assume the library runs in a non-changing environment
(whether the client has frozen itself or just decided not to change
anything) or you assume it is a battlefield where anything can happen
and try to capture a reference to all available built-ins at first load
as well as being defensive against potential changes to the environment
(like Object.prototype in your case).

>> "what if an attacker switches Array.prototype.push and Array.prototype.pop?"
> These are issues that are easy to address by using stored late-bound function references rather than methods and array-likes instead of true arrays.
>
>      var push = Function.prototype.call.bind(Array.prototype.push),
>          arrayLike = Object.create(null);
>      arrayLike.length = 0;
>      push(arrayLike, 'item-1');
>
> As long as the environment is correct when my script initializes, I get all methods I need to use stored inside my library's closure. Freezing isn't needed.
>
> It's also possible to write around the `defineProperty` problem by converting the descriptor into a prototype-less object. However, I actually encountered some performance problems with this. I was able to improve the performance by only dropping the prototype when necessary (as long as `get`, `set`, `value` or `writable` haven't been added to `Object.prototype`, it's not necessary). However, as a matter of principle, my argument is that `Object.getOwnPropertyDescriptor` should, at the bare minimum, return a descriptor that can be known to work in `Object.defineProperty`.  If `Object.defineProperty` doesn't accept it, then you `getOwnPropertyDescriptor` didn't really give me a valid descriptor.
>
> I think that this behavior (1) limits the creativity of developers to define properties like `Object.prototype.get`
I don't think we should consider adding a Object.prototype.get property
as creativity. For instance, proxies have an optional "get" trap, so
things can become confusing quickly.
Other properties also have other meanings for other objects. For
instance JSON.parse creates objects with Object.prototype as
[[Prototype]], so custom Object.prototype properties may be confusing in
"myData.someProp" cases. It's possible, but annoying to prefix every
[[Get]] with hasOwnProperty checks.
If someone tries to polyfill Array.prototype.contains, they may test
     'contains' in Array.prototype
If Object.prototype.contains is defined, this can mistakenly return
true. etc.
Writing defensive code is a serious relearning of how to write for the
language.

David
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

David Bruant-5
In reply to this post by Mark S. Miller-2
Le 13/03/2013 16:49, Mark S. Miller a écrit :
On Wed, Mar 13, 2013 at 8:26 AM, Nathan Wall <[hidden email]> wrote:
David Bruant wrote:
> Tom Van Cutsem wrote:
> > To my mind, the blame for the breakage lies with `Object.prototype` 
> > being mutated by the third-party script, not with property descriptors 
> > inheriting from Object.prototype. Thus, a fix for the breakage should 
> > address that directly, rather than tweaking the design of property 
> > descriptors, IMHO. 
> I agree. 

> The first (security) decision any JavaScript application should make 
> would be to freeze all built-ins like SES [3][4] does. (In the future, 
> it could even make sense to add a CSP [5] directive for that) 
> If necessary, the application can first enhance the environment by 
> adding polyfills/libraries and such, but that's pretty much the only 
> thing that's acceptable to run before freezing everything. 

Hey David and Tom.  This is good advice for application authors, but I don't work at the application level; I write libraries.  I don't want to freeze everything because I want to leave the environment open to monkey-patching and shimming by other libraries and the application authors. So this isn't an option for me.

> "what if an attacker switches Array.prototype.push and Array.prototype.pop?"

These are issues that are easy to address by using stored late-bound function references rather than methods and array-likes instead of true arrays.

    var push = Function.prototype.call.bind(Array.prototype.push),
        arrayLike = Object.create(null);
    arrayLike.length = 0;
    push(arrayLike, 'item-1');

As long as the environment is correct when my script initializes, I get all methods I need to use stored inside my library's closure. Freezing isn't needed.

That's correct. I've written a bit of code like that myself. At first, for those used to JS or even just conventional oo, the style needed is very counter-intuitive and awkward -- it goes against the grain of what the language tries to make convenient. But amazingly, it is possible, and one even gets used to it after a while.
Would it be easier to teach everyone to freeze their built-ins or to (re)write their code using this style?

We need a name for your use case. It is definitely distinct from the normal JS library writing practice, which implicitly assumes that the primordials haven't been too corrupted but does nothing to detect if this is true.
Given what you're describing as normal library writing practice, teaching everyone to freeze their built-ins sounds like a safer bet in the path of acceptability and making the web overall safer.

In my opinion, to a large extent, using a library and messing around with built-ins the library might be using is a recipe for disaster. This practice shouldn't be encouraged.

David

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Tom Van Cutsem-3
In reply to this post by Nathan Wall
[+Allen]

2013/3/13 Nathan Wall <[hidden email]>
However, as a matter of principle, my argument is that `Object.getOwnPropertyDescriptor` should, at the bare minimum, return a descriptor that can be known to work in `Object.defineProperty`.  If `Object.defineProperty` doesn't accept it, then you `getOwnPropertyDescriptor` didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like `Object.prototype.get`, (2) is a potential stumbling block, (3) has no real benefit -- really, there's not anything positive about this behavior, and (4) forces developers who want to support `Object.prototype.get` to add an extra layer of cleaning before using `defineProperty`.

While the monkey-patching of Object.prototype ("don't do that!") is still the culprit, I agree that it would have been better if defineProperty looked only at "own" properties of the descriptor. I almost always think of descriptors as "records" rather than "objects". Similarly, perhaps Object.getOwnPropertyDescriptor should have returned descriptors whose [[prototype]] was null.

It's true that Reflect.getOwnPropertyDescriptor and Reflect.defineProperty give us a chance to fix this. I'm just worried that these differences will bite developers that will assume that these methods are identical to the Object.* versions.

I'd like to hear Allen's opinion on this issue.

Cheers,
Tom

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Herby Vojčík


Tom Van Cutsem wrote:

> [+Allen]
>
> 2013/3/13 Nathan Wall <[hidden email] <mailto:[hidden email]>>
>
>     However, as a matter of principle, my argument is that
>     `Object.getOwnPropertyDescriptor` should, at the bare minimum,
>     return a descriptor that can be known to work in
>     `Object.defineProperty`.  If `Object.defineProperty` doesn't accept
>     it, then you `getOwnPropertyDescriptor` didn't really give me a
>     valid descriptor.
>
>     I think that this behavior (1) limits the creativity of developers
>     to define properties like `Object.prototype.get`, (2) is a potential
>     stumbling block, (3) has no real benefit -- really, there's not
>     anything positive about this behavior, and (4) forces developers who
>     want to support `Object.prototype.get` to add an extra layer of
>     cleaning before using `defineProperty`.
>
>
> While the monkey-patching of Object.prototype ("don't do that!") is
> still the culprit, I agree that it would have been better if
> defineProperty looked only at "own" properties of the descriptor. I
No, there are legitimate uses of Object.create(descriptorTemplate) with
descriptors.

> almost always think of descriptors as "records" rather than "objects".
> Similarly, perhaps Object.getOwnPropertyDescriptor should have returned
> descriptors whose [[prototype]] was null.

Herby
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

David Bruant-5
In reply to this post by Tom Van Cutsem-3
Le 14/03/2013 08:51, Tom Van Cutsem a écrit :
[+Allen]

2013/3/13 Nathan Wall <[hidden email]>
However, as a matter of principle, my argument is that `Object.getOwnPropertyDescriptor` should, at the bare minimum, return a descriptor that can be known to work in `Object.defineProperty`.  If `Object.defineProperty` doesn't accept it, then you `getOwnPropertyDescriptor` didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like `Object.prototype.get`, (2) is a potential stumbling block, (3) has no real benefit -- really, there's not anything positive about this behavior, and (4) forces developers who want to support `Object.prototype.get` to add an extra layer of cleaning before using `defineProperty`.

While the monkey-patching of Object.prototype ("don't do that!") is still the culprit, I agree that it would have been better if defineProperty looked only at "own" properties of the descriptor.
In a previous message, Brandon Benvie mentioned he uses inheritance to reuse a property descriptor [1] (I think there was another quote of him, but I can't find it now). I can imagine it's a used pattern.

I almost always think of descriptors as "records" rather than "objects". Similarly, perhaps Object.getOwnPropertyDescriptor should have returned descriptors whose [[prototype]] was null.

It's true that Reflect.getOwnPropertyDescriptor and Reflect.defineProperty give us a chance to fix this. I'm just worried that these differences will bite developers that will assume that these methods are identical to the Object.* versions.
I doubt differences would be a good idea.

Maybe an idea would be for Object.defineProperty to call Attributes.@@iterate is user-defined so that a user can restrict what property descriptor properties are being traversed.
If that's too heavy of a refactoring, maybe an ES6 map could be accepted as the 3rd argument of Object.defineProperty (with maps semantics, not objects semantics). This way, one could write the copy function as:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from)){
            let desc = Object.getOwnPropertyDescriptor(from, name);
            desc[@iterator] = ownIterator; // is that the proper syntax? I'm a bit lost :-/
            Object.defineProperty(to, name, new Map(desc));
        }
    }

ownIterator only iterates over own properties as its name indicates, so the Map will only list that. The extra map allocation isn't that big of a deal since it is very short-lived. It could be shared and cleared across iterations if necessary.

Nathan, how do you feel about such a solution?

David

[1] https://mail.mozilla.org/pipermail/es-discuss/2012-November/026081.html

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Brandon Benvie-2
On 3/14/2013 2:12 AM, David Bruant wrote:
Le 14/03/2013 08:51, Tom Van Cutsem a écrit :
[+Allen]

2013/3/13 Nathan Wall <[hidden email]>
However, as a matter of principle, my argument is that `Object.getOwnPropertyDescriptor` should, at the bare minimum, return a descriptor that can be known to work in `Object.defineProperty`.  If `Object.defineProperty` doesn't accept it, then you `getOwnPropertyDescriptor` didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like `Object.prototype.get`, (2) is a potential stumbling block, (3) has no real benefit -- really, there's not anything positive about this behavior, and (4) forces developers who want to support `Object.prototype.get` to add an extra layer of cleaning before using `defineProperty`.

While the monkey-patching of Object.prototype ("don't do that!") is still the culprit, I agree that it would have been better if defineProperty looked only at "own" properties of the descriptor.
In a previous message, Brandon Benvie mentioned he uses inheritance to reuse a property descriptor [1] (I think there was another quote of him, but I can't find it now). I can imagine it's a used pattern.

I also mentioned I thought it was unlikely to be commonly used, since I've never seen it used besides some of my own code (which exists in a couple libraries used by few or just me). I think, though, the other thing I mentioned that you're thinking of is that methods on the Descriptor class prototypes can be useful. Here's a simple version that demonstrates potential utility:


  const fields = new Set(['enumerable', 'configurable', 'writable', 'value', 'get', 'set', 'key']);

  class Descriptor extends null {
    constructor(desc){
      if (desc) {
        for (let [key, value] of items(desc)) {
          if (fields.has(key) && value === undefined || Object.prototype[key] !== value) {
            this[key] = value;
          }
        }
      }
    }
    hide(){
      this.enumerable = false
    }
    lock(){
      this.configurable = false;
    }
    define(object, key = this.key){
      Object.defineProperty(object, key, this);
    }
    /** etc **/
  }

  class AccessorDescriptor extends Descriptor {
    setOn(object, value){
      if (typeof this.set === 'function') {
        return call(this.set, object, value) !== false;
      }
      return false;
    }
    getOn(object){
      if (typeof this.get === 'function') {
        return call(this.get, object);
      }
    }
    /** etc **/
  }

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

David Bruant-5
Le 14/03/2013 17:01, Brandon Benvie a écrit :
I also mentioned I thought it was unlikely to be commonly used, since I've never seen it used besides some of my own code (which exists in a couple libraries used by few or just me).
Sincere apologies on missing an important part of your quote (I remember there was another message than the one I quoted, but I've been unable to find it) :-/

David

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Andrea Giammarchi-2
In reply to this post by Mark S. Miller-2
this is an excellent "Point" Mark, I wish I mentioned  this earlier in redefine.js 

For already panicing developers sake, the problem addressed by Mark can be easily solved going explicitly over the property definition so that this won't work:

Object.freeze(Object.prototype);
function Point(x, y) {
  this.x = x;
  this.y = y;
}
Point.prototype.toString = function() {
  return '<' + this.x + ',' + this.y + '>';
};

alert(new Point(1, 2)); // [object Object]

but this will:
Object.defineProperty(
  Point.prototype,
  'toString', {value:
  function() {
    return '<' + this.x + ',' + this.y + '>';
  }
});

alert(new Point(1, 2)); // <1,2>

br




On Wed, Mar 13, 2013 at 8:36 AM, Mark S. Miller <[hidden email]> wrote:
The one qualification everyone should be aware of is that if they simply freeze these themselves, rather than using the tamper-proofing abstractions defined by SES[1][2], then they will suffer from the override mistake[3] and conventional code such as the following will break:

  function Point(x, y) {
    this.x = x;
    this.y = y;
  }
  Point.prototype.toString = function() {
    return '<' + x + ',' + y + '>';
  };

This normal looking assignment to Point.prototype.toString fails because Point.prototype inherits from Object.prototype, on which toString is a non-writable non-configurable data property.





On Wed, Mar 13, 2013 at 7:18 AM, David Bruant <[hidden email]> wrote:
Le 12/03/2013 16:45, Tom Van Cutsem a écrit :
Hi Nathan,

2013/3/10 Nathan Wall <[hidden email]>
Given that `defineProperty` uses properties on the prototype of the descriptor[1] and `getOwnPropertyDescriptor` returns an object which inherits from `Object.prototype`, the following use-case is volatile:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from))
            Object.defineProperty(to, name,
                Object.getOwnPropertyDescriptor(from, name));
    }

If a third party script happens to add `get`, `set`, or `value` to `Object.prototype` the `copy` function breaks.

To my mind, the blame for the breakage lies with `Object.prototype` being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO.
I agree.

As Object.prototype-jacking threats are discussed more and more recently, I'd like to take a step back and "meta-discuss" JavaScript threats.

Currently, by default, any script that run can mutate the environment it is executed in (it can be fixed by sandboxing with things like Caja [1] and soon the module loader API used with proxies [2], but even then, there could be leaks of native built-ins).
The first (security) decision any JavaScript application should make would be to freeze all built-ins like SES [3][4] does. (In the future, it could even make sense to add a CSP [5] directive for that)
If necessary, the application can first enhance the environment by adding polyfills/libraries and such, but that's pretty much the only thing that's acceptable to run before freezing everything.

Given that freezing all built-ins (after polyfills) is a reasonable thing to do, I think JavaScript threat should be considered serious only if applicable assuming the environment is already frozen.
It naturally rules out threats related to property descriptors inheriting from Object.prototype or anything looking like "what if an attacker switches Array.prototype.push and Array.prototype.pop?"

David

[1] http://code.google.com/p/google-caja/
[2] http://wiki.ecmascript.org/doku.php?id=harmony:module_loaders
[3] http://code.google.com/p/es-lab/wiki/SecureEcmaScript
[4] http://code.google.com/p/es-lab/source/browse/#svn%2Ftrunk%2Fsrc%2Fses
[5] https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html



--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss



_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss