472792 - Fix for bug 470720 can be circumvented

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

Fix for bug 470720 can be circumvented since XPC_NW_RewrapIfDeepWrapper unwraps
XOW.

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/XPCNativeWrapper.cpp#347

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better fix

This fixes both bugs by making XPCNativeWrapper do security checks, like the other two wrappers.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Betterer fix

The previous patch left out the meat of the code in EnsureLegalActivity.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 356587 [details] [diff] [review]
Betterer fix

- In EnsureLegalActivity():

+  PRBool isPrivileged = PR_FALSE;
+  if (NS_SUCCEEDED(ssm->IsSystemPrincipal(subjectPrincipal, &isPrivileged)) &&
+      isPrivileged) {
+    // Chrome code can do anything.
+    return JS_TRUE;
+  }
+
+  // Alternatively, this might be content code with UniversalXPConnect.
+  void *annotation = JS_GetFrameAnnotation(cx, fp);
+  nsresult rv = subjectPrincipal->IsCapabilityEnabled("UniversalXPConnect",
+                                                      annotation,
+                                                      &isPrivileged);

IsCapabilityEnabled() always returns true if you're chrome (as you said yourself :), so you should be able to remove the above IsSystemPrincipal() check.

- In a/js/src/xpconnect/src/xpcwrappednative.cpp
+            XPCWrappedNative* wrapper;
+            if(!XPCNativeWrapper::GetWrappedNative(cx, cur, &wrapper))
+                JS_ClearPendingException(cx);

Nothing in there throws, right? So no need to clear exceptions here.

r+sr=jst

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bettererest fix

With jst's comments addressed:

diff --git a/js/src/xpconnect/src/XPCNativeWrapper.cpp b/js/src/xpconnect/src/XPCNativeWrapper.cpp
--- a/js/src/xpconnect/src/XPCNativeWrapper.cpp
+++ b/js/src/xpconnect/src/XPCNativeWrapper.cpp
@@ -204,25 +204,19 @@ EnsureLegalActivity(JSContext *cx, JSObj
 
   JSStackFrame *fp;
   nsIPrincipal *subjectPrincipal = ssm->GetCxSubjectPrincipalAndFrame(cx, &fp);
   if (!subjectPrincipal || !fp) {
     // We must allow the access if there is no code running.
     return JS_TRUE;
   }
 
+  // This might be chrome code or content code with UniversalXPConnect.
+  void *annotation = JS_GetFrameAnnotation(cx, fp);
   PRBool isPrivileged = PR_FALSE;
-  if (NS_SUCCEEDED(ssm->IsSystemPrincipal(subjectPrincipal, &isPrivileged)) &&
-      isPrivileged) {
-    // Chrome code can do anything.
-    return JS_TRUE;
-  }
-
-  // Alternatively, this might be content code with UniversalXPConnect.
-  void *annotation = JS_GetFrameAnnotation(cx, fp);
   nsresult rv = subjectPrincipal->IsCapabilityEnabled("UniversalXPConnect",
                                                       annotation,
                                                       &isPrivileged);
   if (NS_SUCCEEDED(rv) && isPrivileged) {
     return JS_TRUE;
   }
 
   // We're in unprivileged code, ensure that we're allowed to access the
diff --git a/js/src/xpconnect/src/xpcwrappednative.cpp b/js/src/xpconnect/src/xpcwrappednative.cpp
--- a/js/src/xpconnect/src/xpcwrappednative.cpp
+++ b/js/src/xpconnect/src/xpcwrappednative.cpp
@@ -1440,19 +1440,17 @@ return_tearoff:
         if(clazz == &sXPC_XOW_JSClass.base &&
            (unsafeObj = XPCWrapper::Unwrap(cx, cur)))
             return GetWrappedNativeOfJSObject(cx, unsafeObj, funobj, pobj2,
                                               pTearOff);
 
         if(XPCNativeWrapper::IsNativeWrapperClass(clazz))
         {
             XPCWrappedNative* wrapper;
-            if(!XPCNativeWrapper::GetWrappedNative(cx, cur, &wrapper))
-                JS_ClearPendingException(cx);
-            else if(wrapper)
+            if(XPCNativeWrapper::GetWrappedNative(cx, cur, &wrapper) && wrapper)
                 return GetWrappedNativeOfJSObject(cx, wrapper->GetFlatJSObject(),
                                                   funobj, pobj2, pTearOff);
         }
 
         if(IsXPCSafeJSObjectWrapperClass(clazz) &&
            (unsafeObj = STOBJ_GET_PARENT(cur)))
             return GetWrappedNativeOfJSObject(cx, unsafeObj, funobj, pobj2,
                                               pTearOff);

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I checked this in but had to back it out because XPCNativeWrapper::GetWrappedNative doesn't deal with UniversalXPConnect scripts properly.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(http://hg.mozilla.org/mozilla-central/rev/5d3af3ff9639 was the checkin and http://hg.mozilla.org/mozilla-central/rev/cd3ec07611fc was the backout).

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bettererester fix

This has r+sr=jst in person. I had to push null around one of the calls in the window.open() chain since the current JS can't actually touch the brand-new window until it's loaded.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/122cf69f6692 and http://hg.mozilla.org/mozilla-central/rev/427d147720e3 landed to fix this.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/61beb1b0e247 (http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9ac8c9a8346 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/130653abfd3f) fixed this on 1.9.1.