From 94a044f0b1a062482b71f08e2389c293d54431d3 Mon Sep 17 00:00:00 2001 From: "bzbarsky@mit.edu" Date: Tue, 18 Mar 2008 14:14:49 -0700 Subject: [PATCH] Finally kill off CheckSameOriginPrincipal, fix remaining callers to do the checks they really want to be doing. Fix screw-up in nsPrincipal::Equals if one principal has a cert and the other does not. Bug 418996, r=mrbkap,dveditz, sr=jst --- caps/idl/nsIScriptSecurityManager.idl | 9 +--- caps/include/nsScriptSecurityManager.h | 10 ++--- caps/src/nsPrincipal.cpp | 18 ++++---- caps/src/nsScriptSecurityManager.cpp | 41 +++++++++++-------- .../xpconnect/src/XPCCrossOriginWrapper.cpp | 39 ++++++++++-------- js/src/xpconnect/src/XPCWrapper.h | 4 +- 6 files changed, 63 insertions(+), 58 deletions(-) diff --git a/caps/idl/nsIScriptSecurityManager.idl b/caps/idl/nsIScriptSecurityManager.idl index b6bc339ee664..1a8da8809b7f 100644 --- a/caps/idl/nsIScriptSecurityManager.idl +++ b/caps/idl/nsIScriptSecurityManager.idl @@ -42,7 +42,7 @@ interface nsIURI; interface nsIChannel; -[scriptable, uuid(ce216cf7-3bcb-48ab-9ff8-d03a24f19ca5)] +[scriptable, uuid(3fffd8e8-3fea-442e-a0ed-2ba81ae197d5)] interface nsIScriptSecurityManager : nsIXPCSecurityManager { ///////////////// Security Checks ////////////////// @@ -291,13 +291,6 @@ interface nsIScriptSecurityManager : nsIXPCSecurityManager in nsIURI aTargetURI, in boolean reportError); - /** - * Returns OK if aSourcePrincipal and aTargetPrincipal - * have the same "origin" (scheme, host, and port). - */ - void checkSameOriginPrincipal(in nsIPrincipal aSourcePrincipal, - in nsIPrincipal aTargetPrincipal); - /** * Returns the principal of the global object of the given context, or null * if no global or no principal. diff --git a/caps/include/nsScriptSecurityManager.h b/caps/include/nsScriptSecurityManager.h index d9d9c4278e35..fca256633cd1 100644 --- a/caps/include/nsScriptSecurityManager.h +++ b/caps/include/nsScriptSecurityManager.h @@ -406,7 +406,10 @@ public: static nsresult ReportError(JSContext* cx, const nsAString& messageTag, nsIURI* aSource, nsIURI* aTarget); - + static nsresult + CheckSameOriginPrincipal(nsIPrincipal* aSubject, + nsIPrincipal* aObject, + PRBool aIsCheckConnect); private: // GetScriptSecurityManager is the only call that can make one @@ -441,11 +444,6 @@ private: const char* aClassName, jsval aProperty, void** aCachedClassPolicy); - nsresult - CheckSameOriginPrincipalInternal(nsIPrincipal* aSubject, - nsIPrincipal* aObject, - PRBool aIsCheckConnect); - nsresult CheckSameOriginDOMProp(nsIPrincipal* aSubject, nsIPrincipal* aObject, diff --git a/caps/src/nsPrincipal.cpp b/caps/src/nsPrincipal.cpp index 38e331e83e2d..91aebbce4104 100755 --- a/caps/src/nsPrincipal.cpp +++ b/caps/src/nsPrincipal.cpp @@ -249,13 +249,14 @@ nsPrincipal::Equals(nsIPrincipal *aOther, PRBool *aResult) } if (this != aOther) { - if (mCert) { - PRBool otherHasCert; - aOther->GetHasCertificate(&otherHasCert); - if (!otherHasCert) { - return NS_OK; - } + PRBool otherHasCert; + aOther->GetHasCertificate(&otherHasCert); + if (otherHasCert != (mCert != nsnull)) { + // One has a cert while the other doesn't. Not equal. + return NS_OK; + } + if (mCert) { nsCAutoString str; aOther->GetFingerprint(str); *aResult = str.Equals(mCert->fingerprint); @@ -292,8 +293,9 @@ nsPrincipal::Equals(nsIPrincipal *aOther, PRBool *aResult) // Codebases are equal if they have the same origin. *aResult = - NS_SUCCEEDED(nsScriptSecurityManager::GetScriptSecurityManager() - ->CheckSameOriginPrincipal(this, aOther)); + NS_SUCCEEDED(nsScriptSecurityManager::CheckSameOriginPrincipal(this, + aOther, + PR_FALSE)); return NS_OK; } diff --git a/caps/src/nsScriptSecurityManager.cpp b/caps/src/nsScriptSecurityManager.cpp index 00c87959fad0..a9b9d35bf008 100644 --- a/caps/src/nsScriptSecurityManager.cpp +++ b/caps/src/nsScriptSecurityManager.cpp @@ -726,16 +726,6 @@ nsScriptSecurityManager::CheckSameOriginURI(nsIURI* aSourceURI, return NS_OK; } -NS_IMETHODIMP -nsScriptSecurityManager::CheckSameOriginPrincipal(nsIPrincipal* aSourcePrincipal, - nsIPrincipal* aTargetPrincipal) -{ - return CheckSameOriginPrincipalInternal(aSourcePrincipal, - aTargetPrincipal, - PR_FALSE); -} - - nsresult nsScriptSecurityManager::CheckPropertyAccessImpl(PRUint32 aAction, nsAXPCNativeCallContext* aCallContext, @@ -962,10 +952,11 @@ nsScriptSecurityManager::CheckPropertyAccessImpl(PRUint32 aAction, return rv; } +/* static */ nsresult -nsScriptSecurityManager::CheckSameOriginPrincipalInternal(nsIPrincipal* aSubject, - nsIPrincipal* aObject, - PRBool aIsCheckConnect) +nsScriptSecurityManager::CheckSameOriginPrincipal(nsIPrincipal* aSubject, + nsIPrincipal* aObject, + PRBool aIsCheckConnect) { /* ** Get origin of subject and object and compare. @@ -1035,8 +1026,19 @@ nsScriptSecurityManager::CheckSameOriginDOMProp(nsIPrincipal* aSubject, PRUint32 aAction, PRBool aIsCheckConnect) { - nsresult rv = CheckSameOriginPrincipalInternal(aSubject, aObject, - aIsCheckConnect); + nsresult rv; + if (aIsCheckConnect) { + // Don't do equality compares, just do a same-origin compare, + // since the object principal isn't a real principal, just a + // GetCodebasePrincipal() on whatever URI we started with. + rv = CheckSameOriginPrincipal(aSubject, aObject, aIsCheckConnect); + } else { + PRBool subsumes; + rv = aSubject->Subsumes(aObject, &subsumes); + if (NS_SUCCEEDED(rv) && !subsumes) { + rv = NS_ERROR_DOM_PROP_ACCESS_DENIED; + } + } if (NS_SUCCEEDED(rv)) return NS_OK; @@ -1696,9 +1698,12 @@ nsScriptSecurityManager::CheckFunctionAccess(JSContext *aCx, void *aFunObj, if (!object) return NS_ERROR_FAILURE; - // Note that CheckSameOriginPrincipalInternal already does an equality - // comparison on subject and object, so no need for us to do it. - return CheckSameOriginPrincipalInternal(subject, object, PR_TRUE); + PRBool subsumes; + rv = subject->Subsumes(object, &subsumes); + if (NS_SUCCEEDED(rv) && !subsumes) { + rv = NS_ERROR_DOM_PROP_ACCESS_DENIED; + } + return rv; } NS_IMETHODIMP diff --git a/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp b/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp index 08810107a451..0f48544fab8f 100644 --- a/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp +++ b/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp @@ -250,14 +250,14 @@ IsValFrame(JSObject *obj, jsval v, XPCWrappedNative *wn) return domwin != nsnull; } -// Returns whether the currently executing code has the same origin as the -// wrapper. Uses nsIScriptSecurityManager::CheckSameOriginPrincipal. +// Returns whether the currently executing code is allowed to access +// the wrapper. Uses nsIPrincipal::Subsumes. // |cx| must be the top context on the context stack. -// If the two principals have the same origin, returns NS_OK. If they differ, +// If the subject is allowed to access the object returns NS_OK. If not, // returns NS_ERROR_DOM_PROP_ACCESS_DENIED, returns another error code on // failure. nsresult -IsWrapperSameOrigin(JSContext *cx, JSObject *wrappedObj) +CanAccessWrapper(JSContext *cx, JSObject *wrappedObj) { // Get the subject principal from the execution stack. nsIScriptSecurityManager *ssm = XPCWrapper::GetSecurityManager(); @@ -279,6 +279,8 @@ IsWrapperSameOrigin(JSContext *cx, JSObject *wrappedObj) // If we somehow end up being called from chrome, just allow full access. // This can happen from components with xpcnativewrappers=no. + // Note that this is just an optimization to avoid getting the + // object principal in this case, since Subsumes() would return true. if (isSystem) { return NS_OK; } @@ -296,7 +298,12 @@ IsWrapperSameOrigin(JSContext *cx, JSObject *wrappedObj) } // Now, we have our two principals, compare them! - return ssm->CheckSameOriginPrincipal(subjectPrin, objectPrin); + PRBool subsumes; + rv = subjectPrin->Subsumes(objectPrin, &subsumes); + if (NS_SUCCEEDED(rv) && !subsumes) { + rv = NS_ERROR_DOM_PROP_ACCESS_DENIED; + } + return rv; } static JSBool @@ -333,7 +340,7 @@ XPC_XOW_FunctionWrapper(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, return ThrowException(NS_ERROR_ILLEGAL_VALUE, cx); } - nsresult rv = IsWrapperSameOrigin(cx, JSVAL_TO_OBJECT(funToCall)); + nsresult rv = CanAccessWrapper(cx, JSVAL_TO_OBJECT(funToCall)); if (NS_FAILED(rv) && rv != NS_ERROR_DOM_PROP_ACCESS_DENIED) { return ThrowException(rv, cx); } @@ -551,7 +558,7 @@ XPC_XOW_AddProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp) if (!wrappedObj) { return ThrowException(NS_ERROR_ILLEGAL_VALUE, cx); } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) { // Can't override properties on foreign objects. @@ -571,7 +578,7 @@ XPC_XOW_DelProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp) if (!wrappedObj) { return ThrowException(NS_ERROR_ILLEGAL_VALUE, cx); } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) { // Can't delete properties on foreign objects. @@ -620,7 +627,7 @@ XPC_XOW_GetOrSetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp, if (!wrappedObj) { return ThrowException(NS_ERROR_ILLEGAL_VALUE, cx); } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { if (rv != NS_ERROR_DOM_PROP_ACCESS_DENIED) { return JS_FALSE; @@ -734,7 +741,7 @@ XPC_XOW_Enumerate(JSContext *cx, JSObject *obj) // Nothing to enumerate. return JS_TRUE; } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) { // Can't enumerate on foreign objects. @@ -760,7 +767,7 @@ XPC_XOW_NewResolve(JSContext *cx, JSObject *obj, jsval id, uintN flags, return JS_TRUE; } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { if (rv != NS_ERROR_DOM_PROP_ACCESS_DENIED) { return JS_FALSE; @@ -840,7 +847,7 @@ XPC_XOW_Convert(JSContext *cx, JSObject *obj, JSType type, jsval *vp) } // Note: JSTYPE_VOID and JSTYPE_STRING are equivalent. - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv) && (rv != NS_ERROR_DOM_PROP_ACCESS_DENIED || (type != JSTYPE_STRING && type != JSTYPE_VOID))) { @@ -908,7 +915,7 @@ XPC_XOW_Call(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) // Nothing to call. return JS_TRUE; } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) { // Can't call. @@ -939,7 +946,7 @@ XPC_XOW_Construct(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, // Nothing to construct. return JS_TRUE; } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) { // Can't construct. @@ -963,7 +970,7 @@ JS_STATIC_DLL_CALLBACK(JSBool) XPC_XOW_HasInstance(JSContext *cx, JSObject *obj, jsval v, JSBool *bp) { JSObject *iface = GetWrappedObject(cx, obj); - nsresult rv = IsWrapperSameOrigin(cx, iface); + nsresult rv = CanAccessWrapper(cx, iface); if (NS_FAILED(rv)) { if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) { // Don't do this test across origins. @@ -1093,7 +1100,7 @@ XPC_XOW_toString(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, return JS_TRUE; } - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) { nsIScriptSecurityManager *ssm = XPCWrapper::GetSecurityManager(); if (!ssm) { diff --git a/js/src/xpconnect/src/XPCWrapper.h b/js/src/xpconnect/src/XPCWrapper.h index 19363df3964a..aaf9ea390ac6 100644 --- a/js/src/xpconnect/src/XPCWrapper.h +++ b/js/src/xpconnect/src/XPCWrapper.h @@ -79,7 +79,7 @@ XPC_XOW_WrapperMoved(JSContext *cx, XPCWrappedNative *innerObj, XPCWrappedNativeScope *newScope); nsresult -IsWrapperSameOrigin(JSContext *cx, JSObject *wrappedObj); +CanAccessWrapper(JSContext *cx, JSObject *wrappedObj); inline JSBool XPC_XOW_ClassNeedsXOW(const char *name) @@ -208,7 +208,7 @@ public: } JSObject *wrappedObj = JSVAL_TO_OBJECT(v); - nsresult rv = IsWrapperSameOrigin(cx, wrappedObj); + nsresult rv = CanAccessWrapper(cx, wrappedObj); if (NS_FAILED(rv)) { JS_ClearPendingException(cx); return nsnull;