Currently the compiler, when it tries to bind a global name, calls the lookupProperty hook on the global object. That may lead to surprises if one sets proto of the global object to a proxy. That proxy, for example, can add a property to the global while reporting that the property does not exist to the lookupProperty caller. The compiler then proceeds to call js_DefineNativeProperty in Compiler::defineGlobals. That will replace the existing property even if it was added as non-writable and non-configurable as the following example demonstrates: var global = this; var done = false; global.__proto__ = Proxy.create({has: function(){ if (!done) { done = true; Object.defineProperty(global, "a", { value : 42, writable: false, configurable: false }); global.a = 10; assertEq(global.a, 42); } return false; }}); eval("var a = 1;"); assertEq(global.a, 42); Currently it throws as the eval will change the property to 1. One way to fix this would be to white-list what we allow on the prototype chain of the global object. But that may add quite a few lines of code since one can create an object with a proxy somewhere on its prototype chain and then assign that object to global's __proto__. This will also may not address the problem in case one obtains an object with a resolve hook that can be tricked to run a script similar to the one above. So a better alternative would be to have a version of js_LookupPropertyWithFlagsInline that does not do the lookup on the prototype while still running the resolve hooks on the object. This way the compiler (and few other places) would not need to do the extra obj == Lookup_result_holder check to filter out non-global properties. I close this bug in case its consequences is more serious than just a removal of the non-configurable property.

The problem is worse than I initially thought as the JSOP_DEFVAR in the interpreter also do the lookupProperty call and proceeds with the js_DefineNativeProperty if the former would not find a property. So if the proxy adds a property and compiles a script that uses the getglobal optimization that getglobal may refer to a deleted slot. The following example demonstrates that: var global = this; global.done = false; global.__proto__ = Proxy.create({has: function(){ if (!global.done) { global.done = true; Object.defineProperty(global, "a", {value: 42}); evalcx('var a; function f() { return a;}', global); } return false; }}); eval("var a = 1;"); delete global.a; assertEq(f(), 42); Note that prior the bug 561923 and on branches (1.9.* could use liveconnect in place of proxies) the bug would be rather mild. Yes, it would allow to delete non-configurable properties, but those properties would have to be added first and not via a resolve hook.

See ES5.1 10.5 steps 5(c) and others that call HasBinding, which is 10.2.1.2.1 HasBinding(N) for an object environment record. 10.2.1.2.1 step 3 says to call [[HasProperty]], 8.12.6, calls [[GetProperty]], 8.12.2, which indeed goes up the prototype chain. /be

No proxy allowed on global object prototype chain, I think. This is an Ecma TC39 issue. Andreas, you agree? /be

Yeah, we don't want to deal with proxies for scope lookups. Same for watch and similar complications. Would be great to specify that.

(In reply to comment #4) > Yeah, we don't want to deal with proxies for scope lookups. Same for watch and > similar complications. Would be great to specify that. Does that leave anything to fix here?

(In reply to comment #5) > (In reply to comment #4) > > Yeah, we don't want to deal with proxies for scope lookups. Same for watch and > > similar complications. Would be great to specify that. > > Does that leave anything to fix here? Seems like -- we don't forbid proxies on the global object's proto chain. /be

(In reply to comment #4) > Yeah, we don't want to deal with proxies for scope lookups. Do we want to support with statements over proxies? (In reply to comment #2) > See ES5.1 10.5 steps 5(c) and others that call HasBinding, which is 10.2.1.2.1 > HasBinding(N) for an object environment record. 10.2.1.2.1 step 3 says to call > [[HasProperty]], 8.12.6, calls [[GetProperty]], 8.12.2, which indeed goes up > the prototype chain. 10.2.1.1.2 CreateMutableBinding specifies "Assert: envRec does not already have a binding for N.". I guess it is possible to deduce from here that proxies are not allowed as they would allow to violate the assertion. On the other hand it seems the specs has a bug. If a global object has foo as a property on its prototype then "var foo;" would declare foo as configurable even outside the eval as the specs would skip the CreateMutableBinding step after [[HasProperty]] returns true.

(In reply to comment #7) > (In reply to comment #4) > > Yeah, we don't want to deal with proxies for scope lookups. > > Do we want to support with statements over proxies? No "with" statement in Harmony (where Proxies are being spec'ed; built on ES5 strict mode), so I say throw an error at any such. > (In reply to comment #2) > > See ES5.1 10.5 steps 5(c) and others that call HasBinding, which is 10.2.1.2.1 > > HasBinding(N) for an object environment record. 10.2.1.2.1 step 3 says to call > > [[HasProperty]], 8.12.6, calls [[GetProperty]], 8.12.2, which indeed goes up > > the prototype chain. > > 10.2.1.1.2 CreateMutableBinding specifies "Assert: envRec does not already have > a binding for N.". I guess it is possible to deduce from here that proxies are > not allowed as they would allow to violate the assertion. This (10.2.1.1) is for declarative environment records, not object environment records (10.2.1.2). > On the other hand it seems the specs has a bug. If a global object has foo as a > property on its prototype then "var foo;" would declare foo as configurable > even outside the eval as the specs would skip the CreateMutableBinding step > after [[HasProperty]] returns true. Cc'ing Allen /be

(In reply to comment #2) > 10.2.1.2.1 step 3 says to call > [[HasProperty]], 8.12.6, calls [[GetProperty]], 8.12.2, which indeed goes up > the prototype chain. On the other hand 15.1 states that "The values of the [[Prototype]] and [[Class]] internal properties of the global object are implementation-dependent." So the spec allows to skip the prototype lookup. An implementation can say that the global has a special object on the proto before the real prototype. [[GetProperty]] for that special object returns undefined when it is called as a part of global's [[HasProperty]] implementation.

I have to say, I wonder if these algorithms should be using [[GetProperty]] rather than [[GetOwnProperty]]. More complexity, certainly, to make HasBinding and SetMutableBinding work correctly (and don't forget the top-level function definition erratum), but maybe that's a better fix.

(Not all of them, in all situations where they're used now, of course, but maybe a narrow fix is somehow possible.)

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #4) > > > Yeah, we don't want to deal with proxies for scope lookups. > > > > Do we want to support with statements over proxies? > > No "with" statement in Harmony (where Proxies are being spec'ed; built on ES5 > strict mode), so I say throw an error at any such. > I not sure that I buy this. While strict mode does not include "with" statements, objects defined from within strict mode, including those that use features that are unique to strict mode can be passed out to non-strict mode code. In that light, it seems quite possible that an object with a Proxy on its prototype chain might be used as a with object. In general, it would seem that we have to allow Proxy objects to appear on the prototype chain and be recognized by normal property access. Given that, I would expect that the property accesses done implicitly within "with" statements should also work correctly with properties defined via prototypes that are Proxies. Regarding the asserts in the 10.2.1.2 Object Environment Records. I think all of these algorithms probably need to be review from a proxy perspective. However, the intent is clear enough. Normal object access is used to access the objects that are the backing of object env records so inheritance and even proxies should work

I still think that the right solution to the bug is to bypass the protoype chain lookup when defining global variables and functions. Without proxies this would be an invisible optimization as SM implementation treats properties coming from the prototype as non-existent. Then proxies on the prototype would be harmless and their presence there could be discussed in another bug.

(In reply to comment #13) > I still think that the right solution to the bug is to bypass the protoype > chain lookup when defining global variables and functions. Without proxies this > would be an invisible optimization as SM implementation treats properties > coming from the prototype as non-existent. Then proxies on the prototype would > be harmless and their presence there could be discussed in another bug. Also note that the correct ES5.1 spec. has special logic for dealing with creating global function bindings in step 5.e of 10.5 I think that because an implementation gets to control what it uses as its global object, it is possible to restrict or tightly define what is on its prototype chain and still conform to the spec. However, I think it is a reasonable user expectation that any property defined on Object.prototype shows as a global binding unless it is explicitly over-ridden. matter.

(In reply to comment #12) > However, the intent is clear enough. Normal object access is used to access > the objects that are the backing of object env records so inheritance and even > proxies should work The rules prefixes the "Normal object access" with extra HasProperty checks. Those checks lead to rather unexpected behavior that "var foo;" would make foo deletable if foo happens to be on the prototype of the global object. It is easy to fix this if the rules would be changed to use HasOwnProperty, not HasProperty. Such change would match what SpiderMonkey is doing many years and would made Proxy on prototypes much safer.

(In reply to comment #14) > However, I think it is a reasonable user expectation that any property defined > on Object.prototype shows as a global binding unless it is explicitly > over-ridden. Another reasonable IMO expectation for user would be that properties of Object.prototype has no influence on bindings of his variables. I.e. a read access is rather different the declarations and assignments.

I totally agree with comment 16 here. js> Object.prototype.x = 5 5 js> x 5 This is insane. I can't see anyone doing this intentionally, even on the crazy internets.

(In reply to comment #15) > (In reply to comment #12) > > However, the intent is clear enough. Normal object access is used to access > > the objects that are the backing of object env records so inheritance and even > > proxies should work > > The rules prefixes the "Normal object access" with extra HasProperty checks. > Those checks lead to rather unexpected behavior that "var foo;" would make foo > deletable if foo happens to be on the prototype of the global object. It is > easy to fix this if the rules would be changed to use HasOwnProperty, not > HasProperty. Such change would match what SpiderMonkey is doing many years and > would made Proxy on prototypes much safer. All the binding creation methods of Object Env Records have to be consider int he context of their use in 10.5. They are trying to implement the semantics that had been specified by ES3 for instantiating global declaration, but ES3 used quite imprecise language. It says WRT global vars "if there is already a property of the variables object..." which could mean an inherited or own property. It's quite possible that even though the ES3 language reasonable implies using HasProperty that HasOwnProperty is more consistent with what Moz has traditionally done. However, there is a lot of wiggle room given that the prototype of the global object is undefined. I know that IE didn't even have a prototype on the the global object prior to IE9

(In reply to comment #17) In current FF daily: [17:41:25.352] toString() [17:41:25.354] "[object Window]" [17:41:45.208] hasOwnProperty("toString") [17:41:45.210] false [17:42:04.716] var toString="oops" [17:42:04.717] undefined [17:42:26.001] toString [17:42:26.002] "oops" seems to imply Object.prototype properties are visible as globals

(In reply to comment #16) > (In reply to comment #14) > > However, I think it is a reasonable user expectation that any property defined > > on Object.prototype shows as a global binding unless it is explicitly > > over-ridden. > > Another reasonable IMO expectation for user would be that properties of > Object.prototype has no influence on bindings of his variables. I.e. a read > access is rather different the declarations and assignments. I need to verify but I think IE9 ended up doing something like this. Things get even more complicated if the global object is a DOM object with its own odd DOMish prototype hierarchy.

(In reply to comment #19) > seems to imply Object.prototype properties are visible as globals Yes, but the presence of the property on the prototype does not influence the bindings of variables in FF and the following prints "Good evening!" and undefined while per ES5 spec that should be something like "function toString() {[native code]}" and undefined. var toString = "Good evening!"; delete toString; print(toString); delete(Object.prototype.toString); print(Object.prototype.toString);

The test case checks: 1. If a browser is doing a lookup on a Object.prototype when searching for a name. 2. If existence of the property on the prototype makes matching var and functions removable. On my Linux laptop FF 3.6, FF development tip, Google Chrome 8.0.552.224 and Opera 11.0 all shows: Name lookup on Object.prototype: true Object.prototype makes var deletable: false Object.prototype makes function deletable: false I do not Windows with MSIE 9 to test there. AFAICS to match this behavior the specs would need to be extended with something like HasOwnBindings function for environment records that would be used to decide about calling CreateMutableBinding. Another possibility would be to extend SetMutableBinding with an argument telling how to define a non-existing binding. The latter is preferable as it would specify one operation instead of current 3 steps like if-has-then-create-else-set. This would avoid surprises if "has" would have side effects.

IE9 says: Name lookup on Object.prototype: true Object.prototype makes var deletable: false Object.prototype makes function deletable: false

(In reply to comment #22) > > On my Linux laptop FF 3.6, FF development tip, Google Chrome 8.0.552.224 and > Opera 11.0 all shows: > > Name lookup on Object.prototype: true > Object.prototype makes var deletable: false > Object.prototype makes function deletable: false > All the major browsers being consistent in this regard is probably good enough for now... However, note that this item is also closely related to bug 577325 which dealt with global function declarations that overwrite an already existing property whose attributes differ from those normally set by declarations. We developed an Es5 spec. change to address that issue. I think we need a similar change to deal with vars. However, the existing spec. change also uses [[GetProperty]] rather than [[GetOwnProperty]] so there may be inheritance effects. I'll work on a spec. fix for both. I believe where we want to end up with is that global function and var declarations create own properties on the global object that unconditionally over-ride an inherited property of the same name. In other words, if there isn't already an own property matching the declared name we do a [[DefineOwnPropety]]. Things are more complicated if there is already a own property.

See proposed ES5.1 errata item for this at https://mail.mozilla.org/pipermail/es5-discuss/2011-January/003882.html

Some questions we had trying to prioritize this: - is this exploitable? - is this is a regression? - does this need to block FF4.0 at all? Setting as a softblocker for now.

> - is this exploitable? I think Igor was saying it was when he added [critical] to the whiteboard, adding sg: prefix on that assumption.

(In reply to comment #27) > > - is this exploitable? > > I think Igor was saying it was when he added [critical] to the whiteboard, > adding sg: prefix on that assumption. We weren't clear on what the security threat was.

(In reply to comment #26) > Some questions we had trying to prioritize this: > - is this exploitable? This allows to read/write beyond allocated memory with script-controllable values. (In reply to comment #27) > I think Igor was saying it was when he added [critical] to the whiteboard, > adding sg: prefix on that assumption. Yes, the intention was to write [sg:critical], thanks for fixing this.

The core reason for the critical status of this bug is that DefineNativeProperty allows to replace permanent properties with removable ones when our runtime has started to do the optimizations based on the permanent status of the properties. If instead we would report an error for such attempts the bug would lead just to unexpected semantics. Brendan: do you know the reasons why DefineNativeProperty is allowed to replace permanent properties with non-permanent ones? Without changing DefineNativeProperty semantics a possible fix does not look attractive. Initially I tried to implement the lookup without looking into the prototype for the global object, but resulting patch starts to look rather big. So I considered again that idea of disabling proxies to appear on the protype chains. My plan was to not allow to set the prototype of any object to object that is itself a proxy or has a proxy on the prototype chain. This way one would not be able to introduce proxy for the global object using something like that does not involve global objects or proxies directly: var p = Proxy.create(...); var o = Object.create(p); Object.prototype.__proto__ = o; This rule still allows to pass proxies to Object.create. But then similar bugs related to an ability to remove non-configurable properties would not be fixed. For example, consider JS_SetWatchPoint, http://hg.mozilla.org/tracemonkey/file/284811f39ca6/js/src/jsdbgapi.cpp#l900 Similar to Compiler::defineGlobals that contains a pair of lookupProperty/DefineNativeProperty where the code calls DefineNativeProperty after an unsuccessful lookup. This would allow to replace non-configurable properties in the object using a similar trick to the example in the comment 1. The script creates an object with a proxy on the prototype and calls the watch. The proxy would pretend that the property does not exist and then add a non-configurable property before lookupProperty returns. To address this we can disable passing proxy to Object.create. But that does not necessary means that we stop this bug as some resolve hook on some object may still allow to run some scripts.

(In reply to comment #30) > The core reason for the critical status of this bug is that > DefineNativeProperty allows to replace permanent properties with removable ones > when our runtime has started to do the optimizations based on the permanent > status of the properties. If instead we would report an error for such attempts > the bug would lead just to unexpected semantics. Where does js_DefineNativeProperty allow this? JSOP_DEFFUN has this comment and related code: /* * We may be processing a function sub-statement or declaration in * function code: we assign rather than redefine if the essential * JSPROP_PERMANENT (not [[Configurable]] in ES5 terms) attribute * is not changing (note that JSPROP_ENUMERATE is set for all Call * object properties). */ > Brendan: do you know the reasons why DefineNativeProperty is allowed to replace > permanent properties with non-permanent ones? No, that is not intended. What's the data and control flow that leads to this? /be

(In reply to comment #31) > Where does js_DefineNativeProperty allow this? When the attrs passed to js_DefineNativeProperty does not include JSPROP_GETTER | JSPROP_SETTER, js_DefineNativeProperty calls putProperty. That does not check attributes for an existing property and overwrites it with new ones. > JSOP_DEFFUN has this comment and > related code: > > /* > * We may be processing a function sub-statement or declaration in > * function code: we assign rather than redefine if the essential > * JSPROP_PERMANENT (not [[Configurable]] in ES5 terms) attribute > * is not changing (note that JSPROP_ENUMERATE is set for all Call > * object properties). > */ The example from the comment 0 demonstrates how the proxy defeats that. A proxy on the prototype in its "has" method can create a JSPROP_PERMANENT property in the object the lookupProperty was called on. Then it can compile/eval code that optimizes the slot access to such property under an assumption that the property always exists. Then the "has" method returns false leading to lookupProperty reporting that the property does not exist. That in turn triggers a call to js_DefineNativeProperty. Under eval that removes JSPROP_PERMANENT from the property attribute.

Igor: ok, I was checking if there was another way. No, it's ok for our internal [[DefineOwnProperty]] method, JSObject::defineProperty or its subroutine the old C style js_DefineNativeProperty, to blow away a permanent property. That is what ECMA-262's [[DefineOwnProperty]] can do too, and it is up to callers and calling logic to avoid that outcome where it is bad. This bug is really about the lack of [[GetOwnProperty]] testing, which Allen is fixing the spec to do (after ES5.1, the ISO version of ES5, IIRC). We should fix our code accordingly. /be

(In reply to comment #33) > Igor: ok, I was checking if there was another way. No, it's ok for our internal > [[DefineOwnProperty]] method, JSObject::defineProperty or its subroutine the > old C style js_DefineNativeProperty, to blow away a permanent property. That is > what ECMA-262's [[DefineOwnProperty]] can do too, and it is up to callers and > calling logic to avoid that outcome where it is bad. According to 8.12.9 [[DefineOwnProperty]] rejects attempts to change non-configurable property into configurable and in many other cases like changing non-configurable data property into non-data property. As this and other bugs demonstrates relying on callers to do the job does not work reliably. Given that now we have optimizations that relies on permanent status of the properties I suggest to add to js_DefineNativeProperty checks that ensures that those optimizations cannot be defeated. In particular, I will try to add a check that a permanent property cannot be changed into non-permanent and a permanent property with a slot cannot be changed into a slotless. This way the watch function should continue to work while compiler can proceed with aggressive optimizations. As regarding the lack of [[GetOwnProperty]] we can deal with that in another bug.

(In reply to comment #34) > (In reply to comment #33) > > Igor: ok, I was checking if there was another way. No, it's ok for our internal > > [[DefineOwnProperty]] method, JSObject::defineProperty or its subroutine the > > old C style js_DefineNativeProperty, to blow away a permanent property. That is > > what ECMA-262's [[DefineOwnProperty]] can do too, and it is up to callers and > > calling logic to avoid that outcome where it is bad. > > According to 8.12.9 [[DefineOwnProperty]] rejects attempts to change > non-configurable property into configurable and in many other cases like > changing non-configurable data property into non-data property. Oops, you're right. In that case, we would do better to match the spec. > As regarding the lack of [[GetOwnProperty]] we can deal with that in another > bug. Ok, please file that one, not s-s. Thanks, /be

The new title for the bug reflects the nature of the fix. An implementation of [[GetOwnProperty]] will proceed under the bug 628298.

The patch intention is not to enforce [[DefineOwnproperty]] semantics, but rather to make sure that the current slot access optimizations are sound with a minimal breakage. For this reason, when js_DefineNativeProperty is called with the attributes that do not include JSPROP_PERMANENT but the existing property has JSPROP_PERMANENT, the patch just adds JSPROP_PERMANENT to the property set rather than throwing an exception. This is necessary to support resolve hooks that recursively calls some init methods that define a set of properties and then to proceed with DefineProperty calls on its own omitting JSPROP_PERMANENT from the attribute set. The only case when the patch throws is when one tries to mutate a data property with a slot into a slotless form. With this the patch passes the try server.

Comment on attachment 507440 [details] [diff] [review] v1 I'm glad to see this change going in. Good find, good discussion, good fix. I think we probably do have to have DefineOwnProperty semantics eventually, but this is the right short-term approach. >+ /* Permanent property must stay permanent. */ "A permanent property" >+ if (!(*attrsp & JSPROP_PERMANENT)) >+ *attrsp |= JSPROP_PERMANENT; Why not just: *attrsp |= JSPROP_PERMANENT; This needs a test or two. r=me with that.

(In reply to comment #38) > This needs a test or two. r=me with that. This bug exists on 1.9.* branches where instead of a Proxy one can use liveconnect to trigger an arbitrary script execution during lookupProperty. Although it is unclear if it can be used to defeat gvar optimization and get read/write past allocated memory, I prefer to play safe and do not expose examples until the bug is fixed on branches.

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/378cfa4ae4f5

Nominating for branches to make sure that over gvar optimization could not be defeated on 19[12] branches via liveconnect.

"needed" but not "blocking" branches since comment 1 says "on branches ... the bug would be rather mild". We'll approve a patch if you back-port it.

If we're not going to fix this on the 1.9.2 branch should we un-hide the bug?