diff --git a/content/xbl/src/nsXBLProtoImplField.cpp b/content/xbl/src/nsXBLProtoImplField.cpp index 642ed5c047d2..69ec59161e70 100644 --- a/content/xbl/src/nsXBLProtoImplField.cpp +++ b/content/xbl/src/nsXBLProtoImplField.cpp @@ -17,7 +17,6 @@ #include "nsXBLPrototypeBinding.h" #include "mozilla/dom/BindingUtils.h" #include "xpcpublic.h" -#include "WrapperFactory.h" using namespace mozilla; using namespace mozilla::dom; @@ -252,36 +251,8 @@ FieldGetterImpl(JSContext *cx, JS::CallArgs args) static JSBool FieldGetter(JSContext *cx, unsigned argc, JS::Value *vp) { - // FieldGetter generally lives in the XBL scope, and is defined as a cross- - // compartment wrapper on the in-content XBL prototype object. When content - // accesses the field for the first time, it ends up invoking the wrapped - // FieldGetter on the prototype, which enters the XBL scope, landing us here. - // We then use the nativeCall machinery to re-enter the content compartment - // (unwrapping |this|), define the field on the in-content |this|, and return - // the value of the field to the caller. - // - // There's one hitch, though. When code in the XBL scope accesses a field on - // the content object, we waive the usual Xray vision granted to XBL scopes - // in order to do the access, because there isn't really anything else sane to - // do. In this sequence of events, the chrome caller invokes a get() for the - // field on the Xrayed element. XrayWrapper::get bounces to BaseProxyHandler::get, - // Which invokes XrayWrapper::getPropertyDescriptor. This detects the field - // access, creates a waived version of the wrapper, and does a lookup for the - // property on the waived wrapper. This would normally result in the resulting - // getter being transitively waived, which would cause said getter to properly - // waive Xray on its return value when it is eventually invoked (by the XBL - // scope) further down in BaseProxyHandler::get. However, this getter is - // FieldGetter, which actually lives in the XBL scope, meaning that we end up - // stripping all the wrappers off, effectively losing track of the fact that - // we meant to be waiving Xray here. - // - // Since fields are already doing this special Xray waiving stuff, the simplest - // solution seems to be to waive Xray on the |this| object before invoking - // CallNonGenericMethod. This means that the nativeCall trap of WaiveXrayWrapper - // will properly waive the result on the way back. Whew. JS::CallArgs args = JS::CallArgsFromVp(argc, vp); - return xpc::WrapperFactory::WaiveXrayAndWrap(cx, args.mutableThisv().address()) && - JS::CallNonGenericMethod + return JS::CallNonGenericMethod (cx, args); } @@ -320,12 +291,7 @@ static JSBool FieldSetter(JSContext *cx, unsigned argc, JS::Value *vp) { JS::CallArgs args = JS::CallArgsFromVp(argc, vp); - // It's probably not actually necessary to waive Xray here given that - // FieldSetter doesn't return everything, but it's good to maintain - // consistency with FieldGetter. See the comment there for more details on - // why we do this. - return xpc::WrapperFactory::WaiveXrayAndWrap(cx, args.mutableThisv().address()) && - JS::CallNonGenericMethod + return JS::CallNonGenericMethod (cx, args); } diff --git a/content/xbl/test/file_bug821850.xhtml b/content/xbl/test/file_bug821850.xhtml index d0dc8515f00e..7454b486be09 100644 --- a/content/xbl/test/file_bug821850.xhtml +++ b/content/xbl/test/file_bug821850.xhtml @@ -44,8 +44,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=821850 is(bound.objectField.bar.a, 1, "Field Xrays work on objects"); is(bound.contentField.foo, 10, "Field Xrays work on content objects"); var hole = bound.contentField.rabbit.hole; - ok(hole.win === XPCNativeWrapper.unwrap(window), "Xray vision remains waived when hitting a native object"); - ok(!Cu.isXrayWrapper(hole.win), "Xray is waived"); + is(hole.win, window, "We gain back Xray vision when hitting a native object"); + ok(Cu.isXrayWrapper(hole.win), "Really is Xray"); // This gets invoked by an event handler. window.finish = function() { @@ -71,20 +71,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=821850 return "method:" + arg; - - - - is(typeof arg.prop, 'undefined', "No properties"); - is(Object.getOwnPropertyNames(arg).length, 0, "Should have no own properties"); - try { - arg.foo = 2; - ok(true, "Stuff fails silently"); - } catch (e) { - ok(false, "Stuff should fail silently"); - } - is(typeof arg.foo, 'undefined', "Shouldn't place props"); - - return this._prop; this._prop = "set:" + val; @@ -134,14 +120,11 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=821850 bound.prop = "someOtherVal"; is(bound.prop, "set:someOtherVal", "Can set properties from content"); - // Make sure we can't pass JS objects to the XBL scope. - var proto = bound.__proto__; - proto.passMeAJSObject({prop: 2}); - // // Try sticking a bunch of stuff on the prototype object. // + var proto = bound.__proto__; proto.someExpando = 201; is(bound.someExpando, 201, "Can stick non-XBL properties on the proto"); diff --git a/js/xpconnect/tests/unit/test_bug845862.js b/js/xpconnect/tests/unit/test_bug845862.js deleted file mode 100644 index 650bdf3cd4dc..000000000000 --- a/js/xpconnect/tests/unit/test_bug845862.js +++ /dev/null @@ -1,13 +0,0 @@ -const Cu = Components.utils; - -function run_test() { - // We rely on the crazy "wantXrays:false also causes values return from the - // sandbox to be waived" behavior, because it's the simplest way to get - // waivers out of the sandbox (which has no native objects). :-( - var sb = new Cu.Sandbox('http://www.example.com', {wantXrays: false}); - Cu.evalInSandbox("this.foo = {}; Object.defineProperty(foo, 'bar', {get: function() {return {};}});", sb); - do_check_true(sb.foo != XPCNativeWrapper(sb.foo), "sb.foo is waived"); - var desc = Object.getOwnPropertyDescriptor(sb.foo, 'bar'); - var b = desc.get(); - do_check_true(b != XPCNativeWrapper(b), "results from accessor descriptors are waived"); -} diff --git a/js/xpconnect/tests/unit/xpcshell.ini b/js/xpconnect/tests/unit/xpcshell.ini index 033881367108..3c3ecf90d9af 100644 --- a/js/xpconnect/tests/unit/xpcshell.ini +++ b/js/xpconnect/tests/unit/xpcshell.ini @@ -17,7 +17,6 @@ tail = [test_bug809652.js] [test_bug813901.js] [test_bug845201.js] -[test_bug845862.js] [test_bug849730.js] [test_bug851895.js] [test_bug854558.js] diff --git a/js/xpconnect/wrappers/AccessCheck.h b/js/xpconnect/wrappers/AccessCheck.h index 89a909d91a63..103a2d6d8ca4 100644 --- a/js/xpconnect/wrappers/AccessCheck.h +++ b/js/xpconnect/wrappers/AccessCheck.h @@ -53,30 +53,6 @@ struct Opaque : public Policy { } }; -// This policy is designed to protect privileged callers from untrusted non- -// Xrayable objects. Nothing is allowed, and nothing throws. -struct GentlyOpaque : public Policy { - static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act) { - return false; - } - static bool deny(js::Wrapper::Action act) { - return true; - } - static bool allowNativeCall(JSContext *cx, JS::IsAcceptableThis test, JS::NativeImpl impl) - { - // We allow nativeCall here because the alternative is throwing (which - // happens in SecurityWrapper::nativeCall), which we don't want. There's - // unlikely to be too much harm to letting this through, because this - // wrapper is only used to wrap less-privileged objects in more-privileged - // scopes, so unwrapping here only drops privileges. - return true; - } - - static bool isSafeToUnwrap() { - return false; - } -}; - // This policy only permits access to the object if the subject can touch // system objects. struct OnlyIfSubjectIsSystem : public Policy { diff --git a/js/xpconnect/wrappers/FilteringWrapper.cpp b/js/xpconnect/wrappers/FilteringWrapper.cpp index 4f1a9b20631a..4e492e9ef555 100644 --- a/js/xpconnect/wrappers/FilteringWrapper.cpp +++ b/js/xpconnect/wrappers/FilteringWrapper.cpp @@ -143,30 +143,6 @@ FilteringWrapper::nativeCall(JSContext *cx, JS::IsAcceptableThis t return Base::Restrictive::nativeCall(cx, test, impl, args); } -template -bool -FilteringWrapper::defaultValue(JSContext *cx, JS::Handle obj, - JSType hint, MutableHandleValue vp) -{ - return Base::defaultValue(cx, obj, hint, vp); -} - -// With our entirely-opaque wrapper, the DefaultValue algorithm throws, -// causing spurious exceptions. Manually implement something benign. -template<> -bool -FilteringWrapper - ::defaultValue(JSContext *cx, JS::Handle obj, - JSType hint, MutableHandleValue vp) -{ - JSString *str = JS_NewStringCopyZ(cx, "[Opaque]"); - if (!str) - return false; - vp.set(JS::StringValue(str)); - return true; -} - - template bool FilteringWrapper::enter(JSContext *cx, JS::Handle wrapper, @@ -203,7 +179,6 @@ FilteringWrapper::enter(JSContext *cx, JS::Handle wrapp #define NNXOW FilteringWrapper #define CW FilteringWrapper #define XCW FilteringWrapper -#define GO FilteringWrapper template<> SOW SOW::singleton(WrapperFactory::SOW_FLAG); template<> SCSOW SCSOW::singleton(WrapperFactory::SOW_FLAG); template<> XOW XOW::singleton(0); @@ -213,12 +188,9 @@ template<> NNXOW NNXOW::singleton(0); template<> CW CW::singleton(0); template<> XCW XCW::singleton(0); -template<> GO GO::singleton(0); - template class SOW; template class XOW; template class DXOW; template class NNXOW; template class ChromeObjectWrapperBase; -template class GO; } diff --git a/js/xpconnect/wrappers/FilteringWrapper.h b/js/xpconnect/wrappers/FilteringWrapper.h index 9bf1a47a7cbc..12002fdcc6ab 100644 --- a/js/xpconnect/wrappers/FilteringWrapper.h +++ b/js/xpconnect/wrappers/FilteringWrapper.h @@ -39,8 +39,6 @@ class FilteringWrapper : public Base { virtual bool nativeCall(JSContext *cx, JS::IsAcceptableThis test, JS::NativeImpl impl, JS::CallArgs args) MOZ_OVERRIDE; - virtual bool defaultValue(JSContext *cx, JS::Handle obj, JSType hint, JS::MutableHandleValue vp) MOZ_OVERRIDE; - virtual bool enter(JSContext *cx, JS::Handle wrapper, JS::Handle id, js::Wrapper::Action act, bool *bp) MOZ_OVERRIDE; diff --git a/js/xpconnect/wrappers/Makefile.in b/js/xpconnect/wrappers/Makefile.in index ad732db7176f..686f640183ec 100644 --- a/js/xpconnect/wrappers/Makefile.in +++ b/js/xpconnect/wrappers/Makefile.in @@ -12,9 +12,6 @@ include $(DEPTH)/config/autoconf.mk LIBRARY_NAME = xpcwrappers_s FORCE_STATIC_LIB = 1 LIBXUL_LIBRARY = 1 -EXPORTS = \ - WrapperFactory.h \ - $(NULL) CPPSRCS = \ AccessCheck.cpp \ diff --git a/js/xpconnect/wrappers/WaiveXrayWrapper.cpp b/js/xpconnect/wrappers/WaiveXrayWrapper.cpp index c6f0135ae9a3..0ef76e74bcb6 100644 --- a/js/xpconnect/wrappers/WaiveXrayWrapper.cpp +++ b/js/xpconnect/wrappers/WaiveXrayWrapper.cpp @@ -15,25 +15,6 @@ namespace xpc { -static bool -WaiveAccessors(JSContext *cx, js::PropertyDescriptor *desc) -{ - if ((desc->attrs & JSPROP_GETTER) && desc->getter) { - JS::Value v = JS::ObjectValue(*JS_FUNC_TO_DATA_PTR(JSObject *, desc->getter)); - if (!WrapperFactory::WaiveXrayAndWrap(cx, &v)) - return false; - desc->getter = js::CastAsJSPropertyOp(&v.toObject()); - } - - if ((desc->attrs & JSPROP_SETTER) && desc->setter) { - JS::Value v = JS::ObjectValue(*JS_FUNC_TO_DATA_PTR(JSObject *, desc->setter)); - if (!WrapperFactory::WaiveXrayAndWrap(cx, &v)) - return false; - desc->setter = js::CastAsJSStrictPropertyOp(&v.toObject()); - } - return true; -} - WaiveXrayWrapper::WaiveXrayWrapper(unsigned flags) : js::CrossCompartmentWrapper(flags) { } @@ -48,7 +29,7 @@ WaiveXrayWrapper::getPropertyDescriptor(JSContext *cx, JS::Handlewrap unsigned flags) { return CrossCompartmentWrapper::getPropertyDescriptor(cx, wrapper, id, desc, flags) && - WrapperFactory::WaiveXrayAndWrap(cx, &desc->value) && WaiveAccessors(cx, desc); + WrapperFactory::WaiveXrayAndWrap(cx, &desc->value); } bool @@ -57,7 +38,7 @@ WaiveXrayWrapper::getOwnPropertyDescriptor(JSContext *cx, JS::Handle unsigned flags) { return CrossCompartmentWrapper::getOwnPropertyDescriptor(cx, wrapper, id, desc, flags) && - WrapperFactory::WaiveXrayAndWrap(cx, &desc->value) && WaiveAccessors(cx, desc); + WrapperFactory::WaiveXrayAndWrap(cx, &desc->value); } bool @@ -85,14 +66,4 @@ WaiveXrayWrapper::construct(JSContext *cx, JS::Handle wrapper, WrapperFactory::WaiveXrayAndWrap(cx, rval.address()); } -// NB: This is important as the other side of a handshake with FieldGetter. See -// nsXBLProtoImplField.cpp. -bool -WaiveXrayWrapper::nativeCall(JSContext *cx, JS::IsAcceptableThis test, - JS::NativeImpl impl, JS::CallArgs args) -{ - return CrossCompartmentWrapper::nativeCall(cx, test, impl, args) && - WrapperFactory::WaiveXrayAndWrap(cx, args.rval().address()); -} - } diff --git a/js/xpconnect/wrappers/WaiveXrayWrapper.h b/js/xpconnect/wrappers/WaiveXrayWrapper.h index 87c955df681e..239eee74d5d3 100644 --- a/js/xpconnect/wrappers/WaiveXrayWrapper.h +++ b/js/xpconnect/wrappers/WaiveXrayWrapper.h @@ -36,9 +36,6 @@ class WaiveXrayWrapper : public js::CrossCompartmentWrapper { unsigned argc, js::Value *argv, JS::MutableHandle rval) MOZ_OVERRIDE; - virtual bool nativeCall(JSContext *cx, JS::IsAcceptableThis test, - JS::NativeImpl impl, JS::CallArgs args) MOZ_OVERRIDE; - static WaiveXrayWrapper singleton; }; diff --git a/js/xpconnect/wrappers/WrapperFactory.cpp b/js/xpconnect/wrappers/WrapperFactory.cpp index e3645dea9796..700331b353e1 100644 --- a/js/xpconnect/wrappers/WrapperFactory.cpp +++ b/js/xpconnect/wrappers/WrapperFactory.cpp @@ -293,10 +293,6 @@ DEBUG_CheckUnwrapSafety(JSObject *obj, js::Wrapper *handler, } else if (AccessCheck::needsSystemOnlyWrapper(obj)) { // SOWs have a dynamic unwrap check, so we can't really say anything useful // about them here :-( - } else if (handler == &FilteringWrapper::singleton) { - // We explicitly use a SecurityWrapper to protect privileged callers from - // less-privileged objects that they should never see. Skip the check in - // this case. } else { // Otherwise, it should depend on whether the target subsumes the origin. MOZ_ASSERT(handler->isSafeToUnwrap() == AccessCheck::subsumes(target, origin)); @@ -365,7 +361,6 @@ WrapperFactory::Rewrap(JSContext *cx, JSObject *existing, JSObject *obj, bool targetSubsumesOrigin = AccessCheck::subsumes(target, origin); bool sameOrigin = targetSubsumesOrigin && originSubsumesTarget; XrayType xrayType = GetXrayType(obj); - bool waiveXrayFlag = flags & WAIVE_XRAY_WRAPPER_FLAG; // By default we use the wrapped proto of the underlying object as the // prototype for our wrapper, but we may select something different below. @@ -400,20 +395,6 @@ WrapperFactory::Rewrap(JSContext *cx, JSObject *existing, JSObject *obj, OnlyIfSubjectIsSystem>::singleton; } - // Normally, a non-xrayable non-waived content object that finds itself in - // a privileged scope is wrapped with a CrossCompartmentWrapper, even though - // the lack of a waiver _really_ should give it an opaque wrapper. This is - // a bit too entrenched to change for content-chrome, but we can at least fix - // it for XBL scopes. - // - // See bug 843829. - else if (targetSubsumesOrigin && !originSubsumesTarget && - !waiveXrayFlag && xrayType == NotXray && - IsXBLScope(target)) - { - wrapper = &FilteringWrapper::singleton; - } - // // Now, handle the regular cases. // @@ -436,7 +417,8 @@ WrapperFactory::Rewrap(JSContext *cx, JSObject *existing, JSObject *obj, // If Xrays are warranted, the caller may waive them for non-security // wrappers. - bool waiveXrays = wantXrays && !securityWrapper && waiveXrayFlag; + bool waiveXrays = wantXrays && !securityWrapper && + (flags & WAIVE_XRAY_WRAPPER_FLAG); wrapper = SelectWrapper(securityWrapper, wantXrays, xrayType, waiveXrays); } diff --git a/js/xpconnect/wrappers/XrayWrapper.cpp b/js/xpconnect/wrappers/XrayWrapper.cpp index 4eef9e283051..ddf757aa99c6 100644 --- a/js/xpconnect/wrappers/XrayWrapper.cpp +++ b/js/xpconnect/wrappers/XrayWrapper.cpp @@ -1303,7 +1303,7 @@ XrayWrapper::~XrayWrapper() namespace XrayUtils { bool -NeedsWaive(JSContext *cx, HandleObject wrapper, HandleId id) +IsTransparent(JSContext *cx, HandleObject wrapper, HandleId id) { // We dynamically waive Xray vision for XBL bindings accessing fields // on bound elements, since there's no way to access such things sanely @@ -1456,11 +1456,18 @@ XrayWrapper::getPropertyDescriptor(JSContext *cx, HandleObject wra typename Traits::ResolvingIdImpl resolving(wrapper, id); - if (XrayUtils::NeedsWaive(cx, wrapper, id)) { - RootedObject waived(cx, WrapperFactory::WaiveXray(cx, wrapper)); - if (!waived || !JS_WrapObject(cx, waived.address())) - return false; - return JS_GetPropertyDescriptorById(cx, waived, id, flags, desc); + // Redirect access straight to the wrapper if we should be transparent. + if (XrayUtils::IsTransparent(cx, wrapper, id)) { + RootedObject obj(cx, Traits::getTargetObject(wrapper)); + { + JSAutoCompartment ac(cx, obj); + if (!JS_GetPropertyDescriptorById(cx, obj, id, flags, desc)) + return false; + } + + if (desc->obj) + desc->obj = wrapper; + return JS_WrapPropertyDescriptor(cx, desc); } if (!holder) @@ -1592,16 +1599,17 @@ XrayWrapper::getOwnPropertyDescriptor(JSContext *cx, HandleObject // NB: Nothing we do here acts on the wrapped native itself, so we don't // enter our policy. + // Redirect access straight to the wrapper if we should be transparent. + if (XrayUtils::IsTransparent(cx, wrapper, id)) { + RootedObject obj(cx, Traits::getTargetObject(wrapper)); + { + JSAutoCompartment ac(cx, obj); + if (!JS_GetPropertyDescriptorById(cx, obj, id, flags, desc)) + return false; + } - if (XrayUtils::NeedsWaive(cx, wrapper, id)) { - RootedObject waived(cx, WrapperFactory::WaiveXray(cx, wrapper)); - if (!waived || !JS_WrapObject(cx, waived.address())) - return false; - if (!JS_GetPropertyDescriptorById(cx, waived, id, flags, desc)) - return false; - if (desc->obj != waived) - desc->obj = nullptr; - return true; + desc->obj = (desc->obj == obj) ? wrapper.get() : nullptr; // XXX + return JS_WrapPropertyDescriptor(cx, desc); } if (!Traits::singleton.resolveOwnProperty(cx, *this, wrapper, holder, id, desc, flags)) @@ -1628,11 +1636,14 @@ XrayWrapper::defineProperty(JSContext *cx, HandleObject wrapper, HandleId id, PropertyDescriptor *desc) { assertEnteredPolicy(cx, wrapper, id); - if (XrayUtils::NeedsWaive(cx, wrapper, id)) { - RootedObject waived(cx, WrapperFactory::WaiveXray(cx, wrapper)); - if (!waived || !JS_WrapObject(cx, waived.address())) + // Redirect access straight to the wrapper if we should be transparent. + if (XrayUtils::IsTransparent(cx, wrapper, id)) { + RootedObject obj(cx, Traits::getTargetObject(wrapper)); + JSAutoCompartment ac(cx, obj); + if (!JS_WrapPropertyDescriptor(cx, desc)) return false; - return JS_DefinePropertyById(cx, waived, id, desc->value, desc->getter, desc->setter, + + return JS_DefinePropertyById(cx, obj, id, desc->value, desc->getter, desc->setter, desc->attrs); } @@ -1687,6 +1698,19 @@ XrayWrapper::delete_(JSContext *cx, HandleObject wrapper, HandleId id, bool *bp) { assertEnteredPolicy(cx, wrapper, id); + // Redirect access straight to the wrapper if we should be transparent. + if (XrayUtils::IsTransparent(cx, wrapper, id)) { + RootedObject obj(cx, Traits::getTargetObject(wrapper)); + + JSAutoCompartment ac(cx, obj); + + JSBool b; + RootedValue v(cx); + if (!JS_DeletePropertyById2(cx, obj, id, v.address()) || !JS_ValueToBoolean(cx, v, &b)) + return false; + *bp = !!b; + return true; + } // Check the expando object. RootedObject target(cx, Traits::getTargetObject(wrapper)); @@ -1711,6 +1735,13 @@ XrayWrapper::enumerate(JSContext *cx, HandleObject wrapper, unsign AutoIdVector &props) { assertEnteredPolicy(cx, wrapper, JSID_VOID); + // Redirect access straight to the wrapper if we should be transparent. + if (XrayUtils::IsTransparent(cx, wrapper, JSID_VOIDHANDLE)) { + RootedObject obj(cx, Traits::getTargetObject(wrapper)); + JSAutoCompartment ac(cx, obj); + return js::GetPropertyNames(cx, obj, flags, &props); + } + if (!AccessCheck::wrapperSubsumes(wrapper)) { JS_ReportError(cx, "Not allowed to enumerate cross origin objects"); return false;