From 790a107f1e80534d1ab491f1253dcfa793f88d74 Mon Sep 17 00:00:00 2001 From: Johnny Stenback Date: Thu, 30 Jul 2009 16:53:04 -0700 Subject: [PATCH] Fixing bug 492713. Remove security checks that are no longer needed. r+sr=mrbkap@gmail.com --- dom/base/nsDOMClassInfo.cpp | 145 +----------------------------------- dom/base/nsDOMClassInfo.h | 4 - 2 files changed, 3 insertions(+), 146 deletions(-) diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp index 2291ed8a17f..c1fd796626a 100644 --- a/dom/base/nsDOMClassInfo.cpp +++ b/dom/base/nsDOMClassInfo.cpp @@ -512,7 +512,6 @@ static const char kDOMStringBundleURL[] = (NODE_SCRIPTABLE_FLAGS | \ nsIXPCScriptable::WANT_POSTCREATE | \ nsIXPCScriptable::WANT_ADDPROPERTY | \ - nsIXPCScriptable::WANT_DELPROPERTY | \ nsIXPCScriptable::WANT_GETPROPERTY | \ nsIXPCScriptable::WANT_ENUMERATE) @@ -1448,8 +1447,6 @@ jsval nsDOMClassInfo::sPackages_id = JSVAL_VOID; static const JSClass *sObjectClass = nsnull; const JSClass *nsDOMClassInfo::sXPCNativeWrapperClass = nsnull; -static PRBool sDoSecurityCheckInAddProperty = PR_TRUE; - /** * Set our JSClass pointer for the Object class */ @@ -3667,10 +3664,7 @@ nsDOMClassInfo::Init() RegisterClassProtos(i); } - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; RegisterExternalClasses(); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; sDisableDocumentAllSupport = nsContentUtils::GetBoolPref("browser.dom.document.all.disabled"); @@ -4955,19 +4949,6 @@ nsWindowSH::AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx, } } - // If we're in a state where we're not supposed to do a security - // check, return early. - if (!sDoSecurityCheckInAddProperty) { - return NS_OK; - } - - if (id == sLocation_id) { - // Don't allow adding a window.location setter or getter, allowing - // that could lead to security bugs (see bug 143369). - - return NS_ERROR_DOM_SECURITY_ERR; - } - return nsEventReceiverSH::AddProperty(wrapper, cx, obj, id, vp, _retval); } @@ -5015,13 +4996,6 @@ nsWindowSH::DelProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx, } } - if (id == sLocation_id) { - // Don't allow deleting window.location, allowing that could lead - // to security bugs (see bug 143369). - - return NS_ERROR_DOM_SECURITY_ERR; - } - // Notify any XOWs on our outer window. nsGlobalWindow *outerWin = win->GetOuterWindowInternal(); @@ -5314,16 +5288,12 @@ public: nsresult Install(JSContext *cx, JSObject *target, jsval thisAsVal) { - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - JSBool ok = ::JS_DefineUCProperty(cx, target, reinterpret_cast(mClassName), nsCRT::strlen(mClassName), thisAsVal, nsnull, nsnull, JSPROP_PERMANENT); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; return ok ? NS_OK : NS_ERROR_UNEXPECTED; } @@ -5686,18 +5656,12 @@ ResolvePrototype(nsIXPConnect *aXPConnect, nsGlobalWindow *aWin, JSContext *cx, getter_AddRefs(constructor)); NS_ENSURE_SUCCESS(rv, rv); - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - nsCOMPtr holder; jsval v; rv = nsDOMClassInfo::WrapNative(cx, obj, constructor, &NS_GET_IID(nsIDOMDOMConstructor), PR_FALSE, &v, getter_AddRefs(holder)); - - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - NS_ENSURE_SUCCESS(rv, rv); if (install) { @@ -5879,17 +5843,10 @@ nsWindowSH::GlobalResolve(nsGlobalWindow *aWin, JSContext *cx, getter_AddRefs(constructor)); NS_ENSURE_SUCCESS(rv, rv); - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - nsCOMPtr holder; jsval v; - rv = WrapNative(cx, obj, constructor, &NS_GET_IID(nsIDOMDOMConstructor), PR_FALSE, &v, getter_AddRefs(holder)); - - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - NS_ENSURE_SUCCESS(rv, rv); rv = constructor->Install(cx, obj, v); @@ -6042,15 +5999,11 @@ nsWindowSH::GlobalResolve(nsGlobalWindow *aWin, JSContext *cx, NS_ENSURE_SUCCESS(rv, rv); - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - JSBool ok = ::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str), ::JS_GetStringLength(str), prop_val, nsnull, nsnull, JSPROP_ENUMERATE); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; *did_resolve = PR_TRUE; return ok ? NS_OK : NS_ERROR_FAILURE; @@ -6181,14 +6134,9 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, // A numeric property accessed and the numeric property is a // child frame. Define a property for this index. - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - *_retval = ::JS_DefineElement(cx, obj, JSVAL_TO_INT(id), JSVAL_VOID, nsnull, nsnull, 0); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - if (*_retval) { *objp = obj; } @@ -6228,21 +6176,12 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, JSObject *realObj; wrapper->GetJSObject(&realObj); - // Resolving a standard class won't do any evil, and it's possible - // for caps to get the answer wrong, so disable the security check - // for this case. - - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - // Don't resolve standard classes on XPCNativeWrapper etc, only // resolve them if we're resolving on the real global object. ok = obj == realObj ? ::JS_ResolveStandardClass(my_cx, obj, id, &did_resolve) : JS_TRUE; - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - if (!ok) { // Trust the JS engine (or the script security manager) to set // the exception in the JS engine. @@ -6341,18 +6280,6 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, } #endif - // Script is accessing a child frame and this access can - // potentially come from a context from a different domain. - // ::JS_DefineUCProperty() will call - // nsWindowSH::AddProperty(), and that method will do a - // security check and that security check will fail since - // other domains can't add properties to a global object in - // this domain. Set the sDoSecurityCheckInAddProperty flag to - // false (and set it to true immediagtely when we're done) to - // tell nsWindowSH::AddProperty() that defining this new - // property is 'ok' in this case, even if the call comes from - // a different context. - JSAutoRequest ar(cx); PRBool ok = ::JS_DefineUCProperty(cx, obj, chars, @@ -6451,8 +6378,8 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, wrapper->GetJSObject(&scope); } - jsval v; nsCOMPtr holder; + jsval v; rv = WrapNative(cx, scope, location, &NS_GET_IID(nsIDOMLocation), PR_TRUE, &v, getter_AddRefs(holder)); NS_ENSURE_SUCCESS(rv, rv); @@ -6466,17 +6393,12 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, } #endif - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - JSAutoRequest ar(cx); JSBool ok = ::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str), ::JS_GetStringLength(str), v, nsnull, nsnull, JSPROP_ENUMERATE); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - if (!ok) { return NS_ERROR_FAILURE; } @@ -6572,9 +6494,6 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, JSAutoRequest ar(cx); - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - jsval winVal = OBJECT_TO_JSVAL(win->GetGlobalJSObject()); if (!win->IsChromeWindow()) { JSObject *scope; @@ -6598,8 +6517,6 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, winVal, JS_PropertyStub, JS_PropertyStub, JSPROP_READONLY | JSPROP_ENUMERATE); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - if (!ok) { return NS_ERROR_FAILURE; } @@ -6614,17 +6531,12 @@ nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, if (!isResolvingJavaProperties) { isResolvingJavaProperties = PR_TRUE; - PRBool oldVal = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - // Tell the window to initialize the Java properties. The // window needs to do this as we need to do this only once, // and detecting that reliably from here is hard. win->InitJavaProperties(); - sDoSecurityCheckInAddProperty = oldVal; - PRBool hasProp; PRBool ok = ::JS_HasProperty(cx, obj, ::JS_GetStringBytes(str), &hasProp); @@ -7000,10 +6912,6 @@ nsNodeSH::DefineVoidProp(JSContext* cx, JSObject* obj, jsval id, JSString* str = JSVAL_TO_STRING(id); - // We might have a document here. - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - // We want this to be as invisible to content script as possible. So // don't enumerate this, and set is as JSPROP_SHARED so it won't get // cached on the object. @@ -7011,8 +6919,6 @@ nsNodeSH::DefineVoidProp(JSContext* cx, JSObject* obj, jsval id, ::JS_GetStringLength(str), JSVAL_VOID, nsnull, nsnull, JSPROP_SHARED); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - if (!ok) { return NS_ERROR_FAILURE; } @@ -8179,44 +8085,6 @@ nsContentListSH::GetNamedItem(nsISupports *aNative, const nsAString& aName, return list->GetNamedItem(aName, aResult); } -// Document helper for document.location and document.on* - -NS_IMETHODIMP -nsDocumentSH::AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx, - JSObject *obj, jsval id, jsval *vp, - PRBool *_retval) -{ - // If we're in a state where we're not supposed to do a security - // check, return early. - if (!sDoSecurityCheckInAddProperty) { - return NS_OK; - } - - if (id == sLocation_id) { - // Don't allow adding a document.location setter or getter, allowing - // that could lead to security bugs (see bug 143369). - - return NS_ERROR_DOM_SECURITY_ERR; - } - - return NS_OK; -} - -NS_IMETHODIMP -nsDocumentSH::DelProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx, - JSObject *obj, jsval id, jsval *vp, - PRBool *_retval) -{ - if (id == sLocation_id) { - // Don't allow deleting document.location, allowing that could lead - // to security bugs (see bug 143369). - - return NS_ERROR_DOM_SECURITY_ERR; - } - - return NS_OK; -} - NS_IMETHODIMP nsDocumentSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, JSObject *obj, jsval id, PRUint32 flags, @@ -8225,10 +8093,8 @@ nsDocumentSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, nsresult rv; if (id == sLocation_id) { - // This must be done even if we're just getting the value of - // document.location (i.e. no checking flags & JSRESOLVE_ASSIGNING - // here) since we must define document.location to prevent the - // getter from being overriden (for security reasons). + // Define the location property on the document object itself so + // that we can intercept getting and setting of document.location. nsCOMPtr doc(do_QueryWrappedNative(wrapper, obj)); NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED); @@ -8244,9 +8110,6 @@ nsDocumentSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, &v, getter_AddRefs(holder)); NS_ENSURE_SUCCESS(rv, rv); - PRBool doSecurityCheckInAddProperty = sDoSecurityCheckInAddProperty; - sDoSecurityCheckInAddProperty = PR_FALSE; - JSAutoRequest ar(cx); JSString *str = JSVAL_TO_STRING(id); @@ -8254,8 +8117,6 @@ nsDocumentSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, ::JS_GetStringLength(str), v, nsnull, nsnull, JSPROP_ENUMERATE); - sDoSecurityCheckInAddProperty = doSecurityCheckInAddProperty; - if (!ok) { return NS_ERROR_FAILURE; } diff --git a/dom/base/nsDOMClassInfo.h b/dom/base/nsDOMClassInfo.h index 16dc5d2c37f..32b6380e6f2 100644 --- a/dom/base/nsDOMClassInfo.h +++ b/dom/base/nsDOMClassInfo.h @@ -899,10 +899,6 @@ public: } public: - NS_IMETHOD AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx, - JSObject *obj, jsval id, jsval *vp, PRBool *_retval); - NS_IMETHOD DelProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx, - JSObject *obj, jsval id, jsval *vp, PRBool *_retval); NS_IMETHOD NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx, JSObject *obj, jsval id, PRUint32 flags, JSObject **objp, PRBool *_retval);