Closed Bug 472792 Opened 11 years ago Closed 11 years ago

Fix for bug 470720 can be circumvented

Categories

(Core :: Security, defect, P1)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [sg:high])

Attachments

(2 files, 3 obsolete files)

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
Attached file testcase
Flags: blocking1.9.1?
Flags: wanted1.9.0.x-
Depends on: 470720
Whiteboard: [sg:high]
Attached patch Better fix (obsolete) — Splinter Review
This fixes both bugs by making XPCNativeWrapper do security checks, like the other two wrappers.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #356287 - Flags: superreview?(jst)
Attachment #356287 - Flags: review?(jst)
Attached patch Betterer fix (obsolete) — Splinter Review
The previous patch left out the meat of the code in EnsureLegalActivity.
Attachment #356287 - Attachment is obsolete: true
Attachment #356587 - Flags: superreview?(jst)
Attachment #356587 - Flags: review?(jst)
Attachment #356287 - Flags: superreview?(jst)
Attachment #356287 - Flags: review?(jst)
Attachment #356587 - Flags: superreview?(jst)
Attachment #356587 - Flags: superreview+
Attachment #356587 - Flags: review?(jst)
Attachment #356587 - Flags: review+
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
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Attached patch Bettererest fix (obsolete) — Splinter Review
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);
Attachment #356587 - Attachment is obsolete: true
Attachment #357269 - Flags: superreview+
Attachment #357269 - Flags: review+
I checked this in but had to back it out because XPCNativeWrapper::GetWrappedNative doesn't deal with UniversalXPConnect scripts properly.
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.
Attachment #357269 - Attachment is obsolete: true
Attachment #357851 - Flags: superreview+
Attachment #357851 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/122cf69f6692 and http://hg.mozilla.org/mozilla-central/rev/427d147720e3 landed to fix this.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
Flags: wanted1.8.1.x-
You need to log in before you can comment on or make changes to this bug.