-- var o = wrap(Error); Object.defineProperty(o, 'prototype', {}); o(); -- $ ./js test.js Assertion failure: isObject(), at ../jsvalue.h:602 Marking s-s because this crashes in a release build.

Note that using a getter fails: Object.defineProperty(o, "prototype", {get: function() {}}); TypeError: property Object.defineProperty(o, "prototype", {get: function () {}}) is non-configurable and can't be deleted And __defineGetter__ also throws: o.__defineGetter__("prototype", function() {}); TypeError: redeclaration of const prototype

Why do you think this should block? Please do not nominate without providing rationale.

(In reply to comment #3) > Why do you think this should block? Please do not nominate without providing > rationale. Sorry. This crashes in release shells and might crash the browser and be exploitable. It is a bit obscure though, maybe 2.x?

This is totally exploitable. Nice find. Change: > Object.defineProperty(o, 'prototype', {}); To: > Object.defineProperty(o, 'prototype', { value: 0x1234567 }); 0x080b2114 in Exception(JSContext*, unsigned int, js::Value*) () (gdb) x/i $pc => 0x80b2114 <_ZL9ExceptionP9JSContextjPN2js5ValueE+100>: mov 0x18(%eax),%edx (gdb) p/x $eax $1 = 0x1234567 Error() assumes that prototype will be an object.

Couple of bugs: 1. Proxies expose the bug where Exception reflects using callee.getProperty(..., 'prototype'), ancient code that predates memoizing "the original value of" the built-ins objects. 2. In jswrapper.cpp we have bool JSWrapper::defineProperty(JSContext *cx, JSObject *wrapper, jsid id, PropertyDescriptor *desc) { SET(JS_DefinePropertyById(cx, wrappedObject(wrapper), id, Jsvalify(desc->value), Jsvalify(desc->getter), Jsvalify(desc->setter), desc->attrs)); } but JS_DefinePropertyById indirectly calls obj->defineProperty, which does not check non-configurable status and replaces any non-configurable property (see bug 624364). Fixing 1 via js_GetClassPrototype is doable. Fixing 2 wants more of the DefinePropertyOn{Object,Array} logic from ES5 to go in CheckCanChangeAttrs or somewhere common. The lack of such a non-configurable check already burned us with bug 624364. Can we beef up CheckCanChangeAttrs without conflicting with or duplicating too much of DefinePropertyOn{Object,Array}? /be

(In reply to comment #6) > Fixing 2 wants more of the DefinePropertyOn{Object,Array} logic from ES5 to go > in CheckCanChangeAttrs or somewhere common. The lack of such a non-configurable > check already burned us with bug 624364. Can we beef up CheckCanChangeAttrs > without conflicting with or duplicating too much of > DefinePropertyOn{Object,Array}? Thanks, yeah, it seems like the current checks could be at a better pinch point. I would like to move it into the shape code and only address #2 in this bug.

Agreed, #1 should be spun out. I'm crashing so I'll let Andreas file it! /be

Everyone sing along: we need the ES5/Harmony-friendly internal MOP now! /be

This is exploitable, so I think this is going to have to block final :(

Okay, I can take this, but I think I'd like to do a minimal fix if we block final. Cleanups or work toward MOP can wait for shipping :) deserves more time and thought.

Absolutely. Smallest possible fix please, rest is followup.

Right, comment 9 was more of a whine, or hope for next month. But also, knowing where to go in the longer term can help with the minimal patch (knock on wood). /be

Zero sg:crit or bust!

Not sure if this is entirely right. autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 42733:4dd9be00049c user: Andreas Gal date: Tue May 18 19:21:43 2010 -0700 summary: Implement ES Harmony Proxies (bug 546590, r=mrbkap).

Could very well be. This bug uses proxies and exposes a latent bug at the least, see comment 6.

Taking Brendan's suggestion and special casing this inside proxies, for now.

Comment on attachment 514633 [details] [diff] [review] fix, take 1 dvander and I wondered if DefinePropertyOnObject could not be used here instead of essentially dup'ing it, but the PropertyDescriptor vs. PropDesc impedance mismatch gave us pause. But Waldo will know what to do, and if we could use DefinePropertyOnObject, we would be taking a step toward the better ES5/Harmony friendly engine-MOP, since DefinePropertyOnObject is likely to become the defineOwnProperty internal method (aka [[DefineOwnProperty]] in ES5). /be

There are two tricky things about going from PropertyDescriptor -> PropDesc; (1) PropDesc knows whether writability or configurability was explicitly specified, whereas PropertyDescriptor does not, and this changes semantics but maybe not in meaningful ways. (2) PropDesc uses Value for its get/set fields, whereas PropertyDescriptor uses property ops, so stuff like SameValue would need to change. With these addressed, we could use DefinePropertyOnObject I think.

Comment on attachment 514633 [details] [diff] [review] fix, take 1 >diff --git a/js/src/jswrapper.cpp b/js/src/jswrapper.cpp >--- a/js/src/jswrapper.cpp >+++ b/js/src/jswrapper.cpp >@@ -48,16 +48,17 @@ > #include "methodjit/PolyIC.h" > #include "methodjit/MonoIC.h" > #ifdef JS_METHODJIT > # include "assembler/jit/ExecutableAllocator.h" > #endif > #include "jscompartment.h" > > #include "jsobjinlines.h" >+#include "jsatominlines.h" > > using namespace js; > using namespace js::gc; > > static int sWrapperFamily; > > void * > JSWrapper::getWrapperFamily() >@@ -131,20 +132,113 @@ bool > JSWrapper::getOwnPropertyDescriptor(JSContext *cx, JSObject *wrapper, jsid id, bool set, > PropertyDescriptor *desc) > { > desc->obj= NULL; // default result if we refuse to perform this action > CHECKED(GetOwnPropertyDescriptor(cx, wrappedObject(wrapper), id, JSRESOLVE_QUALIFIED, > Jsvalify(desc)), set ? SET : GET); > } > >+static bool >+Reject(JSContext *cx, uintN errorNumber, jsid id) >+{ >+ jsid idstr; >+ if (!js_ValueToStringId(cx, IdToValue(id), &idstr)) >+ return false; >+ JSAutoByteString bytes(cx, JSID_TO_STRING(idstr)); >+ if (!bytes) >+ return false; >+ JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, errorNumber, bytes.ptr()); >+ return false; >+} >+ >+// Duplicates some logic from DefinePropertyOnObject to reject some attempts >+// to change read-only, non-configurable properties. >+static bool >+CheckPropertyRedefinition(JSContext *cx, JSObject *obj, jsid id, PropertyDescriptor *desc) >+{ >+ JSObject *holder; >+ JSProperty *prop; >+ if (!js_HasOwnProperty(cx, NULL, obj, id, &holder, &prop)) >+ return false; We usually call this pobj I think, or obj2. >+ >+ if (prop && obj != holder && holder->isNative()) { >+ Shape *shape = (Shape *)prop; >+ if (shape->isSharedPermanent()) >+ prop = NULL; >+ } Why not just return true here? >+ >+ if (!prop) >+ return true; >+ >+ Shape *shape = (Shape *)prop; What if pobj isn't native? You didn't check that. We definitely need test coverage for this. Can you adopt Waldo's tests for this? >+ if (shape->configurable()) >+ return true; >+ >+ // Can't go from non-configurable to configurable. >+ if (!(desc->attrs & JSPROP_PERMANENT)) >+ return Reject(cx, JSMSG_CANT_REDEFINE_PROP, id); >+ >+ // Can't change accessor to data descriptor, or vice versa. >+ bool isAccessor = !!(desc->attrs & (JSPROP_GETTER | JSPROP_SETTER)); >+ if (isAccessor != shape->isAccessorDescriptor()) >+ return Reject(cx, JSMSG_CANT_REDEFINE_PROP, id); >+ >+ if (!isAccessor) { >+ // Disallow a non-configurable js::PropertyOp-guarded property >+ // becoming a writable unguarded data property, since such a >+ // property can have its value changed to one the getter and >+ // setter preclude. >+ if ((!shape->hasDefaultGetter() || !shape->hasDefaultSetter()) && >+ (!(desc->attrs & JSPROP_READONLY) || shape->writable())) { >+ return Reject(cx, JSMSG_CANT_REDEFINE_PROP, id); >+ } >+ >+ if (!shape->writable()) { >+ // Disallow reconfiguring from readonly to writable. >+ if (!(desc->attrs & JSPROP_READONLY)) >+ return Reject(cx, JSMSG_CANT_REDEFINE_PROP, id); >+ >+ // Both descriptors are read-only, non-configurable. Make sure they >+ // have the same value. >+ Value v; >+ if (!js_NativeGet(cx, obj, holder, shape, JSGET_NO_METHOD_BARRIER, &v)) >+ return false; >+ >+ JSBool same; >+ if (!SameValue(cx, desc->value, v, &same)) >+ return false; >+ >+ if (!same) >+ return Reject(cx, JSMSG_CANT_REDEFINE_PROP, id); >+ } >+ } else { >+ if (((desc->attrs & JSPROP_GETTER) && shape->getter() != desc->getter) || >+ ((desc->attrs & JSPROP_SETTER) && shape->setter() != desc->setter)) { >+ return Reject(cx, JSMSG_CANT_REDEFINE_PROP, id); >+ } >+ } >+ >+ return true; >+} >+ >+static bool >+DefinePropertyById(JSContext *cx, JSObject *obj, jsid id, PropertyDescriptor *desc) >+{ >+ if (!CheckPropertyRedefinition(cx, obj, id, desc)) >+ return false; >+ return JS_DefinePropertyById(cx, obj, id, Jsvalify(desc->value), >+ Jsvalify(desc->getter), Jsvalify(desc->setter), desc->attrs); >+} >+ > bool > JSWrapper::defineProperty(JSContext *cx, JSObject *wrapper, jsid id, > PropertyDescriptor *desc) > { >+ SET(DefinePropertyById(cx, wrappedObject(wrapper), id, desc)); > SET(JS_DefinePropertyById(cx, wrappedObject(wrapper), id, Jsvalify(desc->value), > Jsvalify(desc->getter), Jsvalify(desc->setter), desc->attrs)); > } > > bool > JSWrapper::getOwnPropertyNames(JSContext *cx, JSObject *wrapper, AutoIdVector &props) > { > // if we refuse to perform this action, props remains empty

...yes, what gal said. You should copy the existing Object.defineProperty tests, tweak the inputs however needed to test this, and then make the patch run that gauntlet. I'll trust results with such tests far, far more than I'll trust any reading I'll ever be able to do of this code.

> We usually call this pobj I think, or obj2. JM style is "holder" which is much clearer to me, but I can rename it. > Why not just return true here? Good point, thanks. > What if pobj isn't native? You didn't check that Yup, thanks, really this whole function should be skipped if obj isn't native. (In reply to comment #21) > You should copy the existing Object.defineProperty tests I'm not trying to preserve the semantics of those tests. At this point in the pipeline, we've lost information that DefinePropertyOnObject's algorithm uses. I just want to fix the security bug reported, which is that proxies can accidentally wreak havoc on read-only properties. If we want those tests to pass it makes more sense to call DefinePropertyOnObject() directly, but (see comment #18, 19) I'm not sure how to do that, or whether it's out of this bug's scope. (I'll definitely write tests covering the problems in this bug, anyway - new patch tomorrow.)

Comment on attachment 514633 [details] [diff] [review] fix, take 1 I suggested, if we're not going to go to the effort of really making sure we have exact, per-spec semantics, that we just throw an error for any attempt to define a property on a proxy where the property already exists and is non-configurable. It's simple to implement, simple to read and understand (important for avoiding further problems), easy to explain, and easy for web developers to understand. This does expose the presence of proxies, to be sure, but I suspect they're generally rare enough (and property definition upon them rarer yet) that it shouldn't be a problem.

This is getting too hairy to fix with straight-up hacks, there's too much logic to duplicate and I'm sure there are holes in it. Approach #2: subclass PropertyDescriptor into ESPropertyDescriptor. This has extra fields for ES5 defineProperty semantics, and an extra bit that says "I came from scripted defineProperty, so if you want, you may use me as such." This patch fixes the original bug. I need to adapt Waldo's tests, and take a look at XPConnect next.

Very _very_ nice.

Hey guys, are we close getting this one wrapped up?

Comment on attachment 514976 [details] [diff] [review] WIP v2 Smart approach. Could you go slightly further and s/ESPropertyDescriptor/PropDesc/g so we wouldn't have yet another such thing (albeit a subtype)? I.e., keep the subtype relation but merge the new one into the PropDesc one added to jsobj.* for ES5. /be

(In reply to comment #26) > Hey guys, are we close getting this one wrapped up? Close, David can say more, but this needs a day or two from what I can see, and it'll be good. /be

We can fix the sg-crit part of this bug with a 1-liner and move the rest into a non-crit bug. The 1-liner (disallow invalid values for prototype) can be removed after the other bug is fixed.

Okay, I agree, at this point I don't want to weigh down an RC by trundling through test suite changes and failures. This patch is a 4-line fix that stops the bug at Exception. It's possible there are other exploitable areas due to proxies breaking invariants, but I'll split that out as a second bug, and follow-up on Brendan's suggestion there.

Comment on attachment 515126 [details] [diff] [review] trivial spot-fix This is the right thing to do at this point in the cycle.