Bug 1101123. Don't allow redefining the getter of a non-configurable accessor property on native objects, even via the low-level JSAPI methods. r=efaust,bholley
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 09 Dec 2014 14:44:38 -0500
changeset 218904 c44465f2a4832426f0467a531c0a8ca2c6979fc3
parent 218903 b58035bdc84f898681b31c23d42aa7aa45e8fb15
child 218905 4247f2646a63525ad1ae3b99845e476109cbd0d2
push id27949
push usercbook@mozilla.com
push dateWed, 10 Dec 2014 10:50:45 +0000
treeherdermozilla-central@551c3cd74dbd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust, bholley
bugs1101123
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1101123. Don't allow redefining the getter of a non-configurable accessor property on native objects, even via the low-level JSAPI methods. r=efaust,bholley
js/src/jsapi.h
js/src/vm/NativeObject.cpp
js/xpconnect/src/Sandbox.cpp
js/xpconnect/tests/chrome/test_xrayToJS.xul
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -969,16 +969,23 @@ class MOZ_STACK_CLASS SourceBufferHolder
 #define JSPROP_INDEX            0x80    /* name is actually (int) index */
 
 #define JSFUN_STUB_GSOPS       0x200    /* use JS_PropertyStub getter/setter
                                            instead of defaulting to class gsops
                                            for property holding function */
 
 #define JSFUN_CONSTRUCTOR      0x400    /* native that can be called as a ctor */
 
+#define JSPROP_REDEFINE_NONCONFIGURABLE 0x800 /* If set, will allow redefining a
+                                                 non-configurable property, but
+                                                 only on a non-DOM global.  This
+                                                 is a temporary hack that will
+                                                 need to go away in bug
+                                                 1105518 */
+
 #define JSPROP_IGNORE_ENUMERATE 0x1000  /* ignore the value in JSPROP_ENUMERATE.
                                            This flag only valid when defining over
                                            an existing property. */
 #define JSPROP_IGNORE_READONLY  0x2000  /* ignore the value in JSPROP_READONLY.
                                            This flag only valid when defining over
                                            an existing property. */
 #define JSPROP_IGNORE_PERMANENT 0x4000  /* ignore the value in JSPROP_PERMANENT.
                                            This flag only valid when defining over
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1388,16 +1388,47 @@ PurgeScopeChainHelper(ExclusiveContext *
 static inline bool
 PurgeScopeChain(ExclusiveContext *cx, HandleObject obj, HandleId id)
 {
     if (obj->isDelegate() && obj->isNative())
         return PurgeScopeChainHelper(cx, obj, id);
     return true;
 }
 
+/*
+ * Check whether we're redefining away a non-configurable getter, and
+ * throw if so.
+ */
+static inline bool
+CheckAccessorRedefinition(ExclusiveContext *cx, HandleObject obj, HandleShape shape,
+                          PropertyOp getter, StrictPropertyOp setter, HandleId id, unsigned attrs)
+{
+    MOZ_ASSERT(shape->isAccessorDescriptor());
+    if (shape->configurable() || (getter == shape->getter() && setter == shape->setter()))
+        return true;
+
+    /*
+     *  Only allow redefining if JSPROP_REDEFINE_NONCONFIGURABLE is set _and_
+     *  the object is a non-DOM global.  The idea is that a DOM object can
+     *  never have such a thing on its proto chain directly on the web, so we
+     *  should be OK optimizing access to accessors found on such an object.
+     */
+    if ((attrs & JSPROP_REDEFINE_NONCONFIGURABLE) &&
+        obj->is<GlobalObject>() &&
+        !obj->getClass()->isDOMClass())
+    {
+        return true;
+    }
+
+    if (!cx->isJSContext())
+        return false;
+
+    return Throw(cx->asJSContext(), id, JSMSG_CANT_REDEFINE_PROP);
+}
+
 bool
 js::DefineNativeProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId id, HandleValue value,
                          PropertyOp getter, StrictPropertyOp setter, unsigned attrs)
 {
     MOZ_ASSERT(!(attrs & JSPROP_PROPOP_ACCESSORS));
 
     AutoRooterGetterSetter gsRoot(cx, attrs, &getter, &setter);
 
@@ -1423,16 +1454,18 @@ js::DefineNativeProperty(ExclusiveContex
                     /* Ignore getter/setter properties added to typed arrays. */
                     return true;
                 }
                 if (!NativeObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                     return false;
                 shape = obj->lookup(cx, id);
             }
             if (shape->isAccessorDescriptor()) {
+                if (!CheckAccessorRedefinition(cx, obj, shape, getter, setter, id, attrs))
+                    return false;
                 attrs = ApplyOrDefaultAttributes(attrs, shape);
                 shape = NativeObject::changeProperty<SequentialExecution>(cx, obj, shape, attrs,
                                                                           JSPROP_GETTER | JSPROP_SETTER,
                                                                           (attrs & JSPROP_GETTER)
                                                                           ? getter
                                                                           : shape->getter(),
                                                                           (attrs & JSPROP_SETTER)
                                                                           ? setter
@@ -1449,18 +1482,25 @@ js::DefineNativeProperty(ExclusiveContex
          * a data property.
          * FIXME: All this logic should be removed when Proxies use PropDesc, but we need to
          *        remove JSPropertyOp getters and setters first.
          * FIXME: This is still wrong for various array types, and will set the wrong attributes
          *        by accident, but we can't use NativeLookupOwnProperty in this case, because of resolve
          *        loops.
          */
         shape = obj->lookup(cx, id);
-        if (shape && shape->isDataDescriptor())
-            attrs = ApplyOrDefaultAttributes(attrs, shape);
+        if (shape) {
+            if (shape->isAccessorDescriptor() &&
+                !CheckAccessorRedefinition(cx, obj, shape, getter, setter, id, attrs))
+            {
+                return false;
+            }
+            if (shape->isDataDescriptor())
+                attrs = ApplyOrDefaultAttributes(attrs, shape);
+        }
     } else {
         /*
          * We have been asked merely to update some attributes by a caller of
          * Object.defineProperty, laundered through the proxy system, and returning here. We can
          * get away with just using JSObject::changeProperty here.
          */
         if (!NativeLookupOwnProperty(cx, obj, id, &shape))
             return false;
@@ -1476,16 +1516,22 @@ js::DefineNativeProperty(ExclusiveContex
                      */
                     return true;
                 }
                 if (!NativeObject::sparsifyDenseElement(cx, obj, JSID_TO_INT(id)))
                     return false;
                 shape = obj->lookup(cx, id);
             }
 
+            if (shape->isAccessorDescriptor() &&
+                !CheckAccessorRedefinition(cx, obj, shape, getter, setter, id, attrs))
+            {
+                return false;
+            }
+
             attrs = ApplyOrDefaultAttributes(attrs, shape);
 
             /* Keep everything from the shape that isn't the things we're changing */
             unsigned attrMask = ~(JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
             shape = NativeObject::changeProperty<SequentialExecution>(cx, obj, shape, attrs, attrMask,
                                                                       shape->getter(), shape->setter());
             if (!shape)
                 return false;
--- a/js/xpconnect/src/Sandbox.cpp
+++ b/js/xpconnect/src/Sandbox.cpp
@@ -441,17 +441,17 @@ sandbox_addProperty(JSContext *cx, Handl
         if (!JS_CopyPropertyFrom(cx, id, unwrappedProto, obj))
             return false;
     }
 
     if (!JS_GetPropertyDescriptorById(cx, obj, id, &pd))
         return false;
     unsigned attrs = pd.attributes() & ~(JSPROP_GETTER | JSPROP_SETTER);
     if (!JS_DefinePropertyById(cx, obj, id, vp,
-                               attrs | JSPROP_PROPOP_ACCESSORS,
+                               attrs | JSPROP_PROPOP_ACCESSORS | JSPROP_REDEFINE_NONCONFIGURABLE,
                                JS_PROPERTYOP_GETTER(writeToProto_getProperty),
                                JS_PROPERTYOP_SETTER(writeToProto_setProperty)))
         return false;
 
     return true;
 }
 
 #define XPCONNECT_SANDBOX_CLASS_METADATA_SLOT (XPCONNECT_GLOBAL_EXTRA_SLOT_OFFSET)
--- a/js/xpconnect/tests/chrome/test_xrayToJS.xul
+++ b/js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ -330,27 +330,32 @@ https://bugzilla.mozilla.org/show_bug.cg
     // Construct an object full of tricky things.
     let symbolProps = '';
     if (jsHasSymbols) {
       uniqueSymbol = iwin.eval('var uniqueSymbol = Symbol("uniqueSymbol"); uniqueSymbol');
       symbolProps = `, [uniqueSymbol]: 43,
                      [Symbol.for("registrySymbolProp")]: 44`;
     }
     var trickyObject =
-      iwin.eval(`new Object({
+      iwin.eval(`(function() {
+                 var o = new Object({
                    primitiveProp: 42, objectProp: { foo: 2 },
                    xoProp: top.location, hasOwnProperty: 10,
                    get getterProp() { return 2; },
                    set setterProp(x) { },
                    get getterSetterProp() { return 3; },
                    set getterSetterProp(x) { },
                    callableProp: function() { },
                    nonXrayableProp: new WeakMap()
                    ${symbolProps}
-                 })`);
+                 });
+                 Object.defineProperty(o, "nonConfigurableGetterSetterProp",
+                    { get: function() { return 5; }, set: function() {} });
+                 return o;
+                 })()`);
     testTrickyObject(trickyObject);
   }
 
   function testArray() {
     // The |length| property is generally very weird, especially with respect
     // to its behavior on the prototype. Array.prototype is actually an Array
     // instance, and therefore has a vestigial .length. But we don't want to
     // show that over Xrays, and generally want .length to just appear as an
@@ -368,17 +373,18 @@ https://bugzilla.mozilla.org/show_bug.cg
     var trickyArray =
       iwin.eval(`var trickyArray = [];
                  trickyArray.primitiveProp = 42;
                  trickyArray.objectProp = { foo: 2 };
                  trickyArray.xoProp = top.location;
                  trickyArray.hasOwnProperty = 10;
                  Object.defineProperty(trickyArray, 'getterProp', { get: function() { return 2; }});
                  Object.defineProperty(trickyArray, 'setterProp', { set: function(x) {}});
-                 Object.defineProperty(trickyArray, 'getterSetterProp', { get: function() { return 3; }, set: function(x) {}});
+                 Object.defineProperty(trickyArray, 'getterSetterProp', { get: function() { return 3; }, set: function(x) {}, configurable: true});
+                 Object.defineProperty(trickyArray, 'nonConfigurableGetterSetterProp', { get: function() { return 5; }, set: function(x) {}});
                  trickyArray.callableProp = function() {};
                  trickyArray.nonXrayableProp = new WeakMap();
                  ${symbolProps}
                  trickyArray;`);
 
     // Test indexed access.
     trickyArray.wrappedJSObject[9] = "some indexed property";
     is(trickyArray[9], "some indexed property", "indexed properties work correctly over Xrays");
@@ -482,16 +488,30 @@ https://bugzilla.mozilla.org/show_bug.cg
     // We should be able to overwrite an existing accessor prop and convert it
     // to a value prop.
     is(trickyObject.wrappedJSObject.getterSetterProp, 3, "Underlying object has getter");
     is(trickyObject.getterSetterProp, undefined, "Filtering properly over Xray");
     trickyObject.getterSetterProp = 'redefined';
     is(trickyObject.getterSetterProp, 'redefined', "Redefinition works");
     is(trickyObject.wrappedJSObject.getterSetterProp, 'redefined', "Redefinition forwards");
 
+    // We should NOT be able to overwrite an existing non-configurable accessor
+    // prop, though.
+    is(trickyObject.wrappedJSObject.nonConfigurableGetterSetterProp, 5,
+       "Underlying object has getter");
+    is(trickyObject.nonConfigurableGetterSetterProp, undefined,
+       "Filtering properly over Xray here too");
+    checkThrows(function() {
+	trickyObject.nonConfigurableGetterSetterProp = 'redefined';
+    }, /config/, "Should throw when redefining non-configurable prop");
+    is(trickyObject.nonConfigurableGetterSetterProp, undefined,
+       "Redefinition should have thrown");
+    is(trickyObject.wrappedJSObject.nonConfigurableGetterSetterProp, 5,
+       "Redefinition really should have thrown");
+
     checkThrows(function() { trickyObject.hasOwnProperty = 33; }, /shadow/,
                 "Should reject shadowing of pre-existing inherited properties over Xrays");
 
     checkThrows(function() { Object.defineProperty(trickyObject, 'rejectedProp', { get: function() {}}); },
                 /accessor property/, "Should reject accessor property definition");
   }
 
   function testTypedArrays() {