Bug 805807 - Rearchitect filtering policies so that check() doesn't throw on denial. r=mrbkap
☠☠ backed out by 57eeca4191dd ☠ ☠
authorBobby Holley <bobbyholley@gmail.com>
Fri, 02 Nov 2012 13:27:59 +0100
changeset 112155 23c9f61a243b4bbb833b9ac139d425c1805a6660
parent 112154 6ca11f4b470cf2c0cb331100eec7189548dc926d
child 112156 95187d8e170c6df98ca83267a65667a9bff92118
push id23798
push userryanvm@gmail.com
push dateSat, 03 Nov 2012 00:06:35 +0000
treeherdermozilla-central@6134edeea902 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs805807
milestone19.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 805807 - Rearchitect filtering policies so that check() doesn't throw on denial. r=mrbkap This is another one of those annoying situaitons in XPConnect right now where we can't ask a question without potentially throwing if the answer is no. There's also a bunch of unused cruft in here (like the Perm*Access stuff), so this stuff was ripe for a spring cleaning. Unfortunately, I wasn't able to divide this patch up nicely. Sorry for the big diff. :-( In a nutshell, this patch changes things so that Policy::check() just becomes a predicate that says whether the access is allowed or not. There's the remote possibility that one of the underlying JSAPI calls in a ::check() implementation might throw, so callers to ::check() should check JS_IsExceptionPending afterwards (this doesn't catch OOM, but we can just continue along until the next OOM-triggering operation and throw there). Aside from exceptional cases, callers should call Policy::deny if they want to report the failure. Policy::deny returns success value that should be returned to the wrapper's consumer.
js/src/jswrapper.h
js/xpconnect/wrappers/AccessCheck.cpp
js/xpconnect/wrappers/AccessCheck.h
js/xpconnect/wrappers/FilteringWrapper.cpp
--- a/js/src/jswrapper.h
+++ b/js/src/jswrapper.h
@@ -38,22 +38,16 @@ class JS_FRIEND_API(Wrapper) : public Di
         PUNCTURE
     };
 
     enum Flags {
         CROSS_COMPARTMENT = 1 << 0,
         LAST_USED_FLAG = CROSS_COMPARTMENT
     };
 
-    typedef enum {
-        PermitObjectAccess,
-        PermitPropertyAccess,
-        DenyAccess
-    } Permission;
-
     static JSObject *New(JSContext *cx, JSObject *obj, JSObject *proto,
                          JSObject *parent, Wrapper *handler);
 
     static Wrapper *wrapperHandler(RawObject wrapper);
 
     static JSObject *wrappedObject(RawObject wrapper);
 
     unsigned flags() const {
--- a/js/xpconnect/wrappers/AccessCheck.cpp
+++ b/js/xpconnect/wrappers/AccessCheck.cpp
@@ -325,48 +325,33 @@ AccessCheck::deny(JSContext *cx, jsid id
         if (chars)
             JS_ReportError(cx, "Permission denied to access property '%hs'", chars);
     }
 }
 
 enum Access { READ = (1<<0), WRITE = (1<<1), NO_ACCESS = 0 };
 
 static bool
-Deny(JSContext *cx, jsid id, Wrapper::Action act)
-{
-    // Refuse to perform the action and just return the default value.
-    if (act == Wrapper::GET)
-        return true;
-    // If its a set, deny it and throw an exception.
-    AccessCheck::deny(cx, id);
-    return false;
-}
-
-static bool
 IsInSandbox(JSContext *cx, JSObject *obj)
 {
     JSAutoCompartment ac(cx, obj);
     JSObject *global = JS_GetGlobalForObject(cx, obj);
     return !strcmp(js::GetObjectJSClass(global)->name, "Sandbox");
 }
 
 bool
-ExposedPropertiesOnly::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper::Action act,
-                             Permission &perm)
+ExposedPropertiesOnly::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper::Action act)
 {
     JSObject *wrappedObject = Wrapper::wrappedObject(wrapper);
 
-    if (act == Wrapper::CALL) {
-        perm = PermitObjectAccess;
+    if (act == Wrapper::CALL)
         return true;
-    }
 
-    perm = DenyAccess;
     if (act == Wrapper::PUNCTURE)
-        return Deny(cx, id, act);
+        return false;
 
     jsid exposedPropsId = GetRTIdByIndex(cx, XPCJSRuntime::IDX_EXPOSEDPROPS);
 
     // We need to enter the wrappee's compartment to look at __exposedProps__,
     // but we want to be in the wrapper's compartment if we call Deny().
     //
     // Unfortunately, |cx| can be in either compartment when we call ::check. :-(
     JSAutoCompartment ac(cx, wrappedObject);
@@ -375,17 +360,16 @@ ExposedPropertiesOnly::check(JSContext *
     if (!JS_HasPropertyById(cx, wrappedObject, exposedPropsId, &found))
         return false;
 
     // Always permit access to "length" and indexed properties of arrays.
     if ((JS_IsArrayObject(cx, wrappedObject) ||
          JS_IsTypedArrayObject(wrappedObject, cx)) &&
         ((JSID_IS_INT(id) && JSID_TO_INT(id) >= 0) ||
          (JSID_IS_STRING(id) && JS_FlatStringEqualsAscii(JSID_TO_FLAT_STRING(id), "length")))) {
-        perm = PermitPropertyAccess;
         return true; // Allow
     }
 
     // If no __exposedProps__ existed, deny access.
     if (!found) {
         // Everything below here needs to be done in the wrapper's compartment.
         JSAutoCompartment wrapperAC(cx, wrapper);
         // Make a temporary exception for objects in a chrome sandbox to help
@@ -400,55 +384,47 @@ ExposedPropertiesOnly::check(JSContext *
                 nsCOMPtr<nsIDocument> doc =
                     do_QueryInterface(win->GetExtantDocument());
                 if (doc) {
                     doc->WarnOnceAbout(nsIDocument::eNoExposedProps,
                                        /* asError = */ true);
                 }
             }
 
-            perm = PermitPropertyAccess;
             return true;
         }
-        return Deny(cx, id, act);
+        return false;
     }
 
-    if (id == JSID_VOID) {
-        // This will force the caller to call us back for individual property accesses.
-        perm = PermitPropertyAccess;
+    if (id == JSID_VOID)
         return true;
-    }
 
     JS::Value exposedProps;
     if (!JS_LookupPropertyById(cx, wrappedObject, exposedPropsId, &exposedProps))
         return false;
 
-    if (exposedProps.isNullOrUndefined()) {
-        JSAutoCompartment wrapperAC(cx, wrapper);
-        return Deny(cx, id, act);
-    }
+    if (exposedProps.isNullOrUndefined())
+        return false;
 
     if (!exposedProps.isObject()) {
         JS_ReportError(cx, "__exposedProps__ must be undefined, null, or an Object");
         return false;
     }
 
     JSObject *hallpass = &exposedProps.toObject();
 
     Access access = NO_ACCESS;
 
     JSPropertyDescriptor desc;
     memset(&desc, 0, sizeof(desc));
     if (!JS_GetPropertyDescriptorById(cx, hallpass, id, JSRESOLVE_QUALIFIED, &desc)) {
         return false; // Error
     }
-    if (desc.obj == NULL || !(desc.attrs & JSPROP_ENUMERATE)) {
-        JSAutoCompartment wrapperAC(cx, wrapper);
-        return Deny(cx, id, act);
-    }
+    if (!desc.obj || !(desc.attrs & JSPROP_ENUMERATE))
+        return false;
 
     if (!JSVAL_IS_STRING(desc.value)) {
         JS_ReportError(cx, "property must be a string");
         return false;
     }
 
     JSString *str = JSVAL_TO_STRING(desc.value);
     size_t length;
@@ -482,49 +458,42 @@ ExposedPropertiesOnly::check(JSContext *
 
     if (access == NO_ACCESS) {
         JS_ReportError(cx, "specified properties must have a permission bit set");
         return false;
     }
 
     if ((act == Wrapper::SET && !(access & WRITE)) ||
         (act != Wrapper::SET && !(access & READ))) {
-        JSAutoCompartment wrapperAC(cx, wrapper);
-        return Deny(cx, id, act);
+        return false;
     }
 
-    perm = PermitPropertyAccess;
-    return true; // Allow
+    return true;
 }
 
 bool
-ComponentsObjectPolicy::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper::Action act,
-                              Permission &perm) 
+ComponentsObjectPolicy::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper::Action act)
 {
-    perm = DenyAccess;
     JSAutoCompartment ac(cx, wrapper);
 
     if (JSID_IS_STRING(id) && act == Wrapper::GET) {
         JSFlatString *flatId = JSID_TO_FLAT_STRING(id);
         if (JS_FlatStringEqualsAscii(flatId, "isSuccessCode") ||
             JS_FlatStringEqualsAscii(flatId, "lookupMethod") ||
             JS_FlatStringEqualsAscii(flatId, "interfaces") ||
             JS_FlatStringEqualsAscii(flatId, "interfacesByID") ||
             JS_FlatStringEqualsAscii(flatId, "results"))
         {
-            perm = PermitPropertyAccess;
             return true;
         }
     }
 
     // We don't have any way to recompute same-compartment Components wrappers,
     // so we need this dynamic check. This can go away when we expose Components
     // as SpecialPowers.wrap(Components) during automation.
     if (xpc::IsUniversalXPConnectEnabled(cx)) {
-        perm = PermitPropertyAccess;
         return true;
     }
 
-    AccessCheck::deny(cx, id);
     return false;
 }
 
 }
--- a/js/xpconnect/wrappers/AccessCheck.h
+++ b/js/xpconnect/wrappers/AccessCheck.h
@@ -34,62 +34,40 @@ class AccessCheck {
     static bool needsSystemOnlyWrapper(JSObject *obj);
 
     static bool isScriptAccessOnly(JSContext *cx, JSObject *wrapper);
 
     static void deny(JSContext *cx, jsid id);
 };
 
 struct Policy {
-    typedef js::Wrapper::Permission Permission;
-
-    static const Permission PermitObjectAccess = js::Wrapper::PermitObjectAccess;
-    static const Permission PermitPropertyAccess = js::Wrapper::PermitPropertyAccess;
-    static const Permission DenyAccess = js::Wrapper::DenyAccess;
-};
-
-// This policy permits access to all properties.
-struct Permissive : public Policy {
-    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act,
-                      Permission &perm) {
-        perm = PermitObjectAccess;
-        return true;
-    }
 };
 
 // This policy only permits access to the object if the subject can touch
 // system objects.
 struct OnlyIfSubjectIsSystem : public Policy {
-    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act,
-                      Permission &perm) {
-        if (AccessCheck::isSystemOnlyAccessPermitted(cx)) {
-            perm = PermitObjectAccess;
-            return true;
-        }
-        perm = DenyAccess;
-        JSAutoCompartment ac(cx, wrapper);
+    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act) {
+        return AccessCheck::isSystemOnlyAccessPermitted(cx);
+    }
+
+    static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
         AccessCheck::deny(cx, id);
         return false;
     }
 };
 
 // This policy only permits access to properties that are safe to be used
 // across origins.
 struct CrossOriginAccessiblePropertiesOnly : public Policy {
-    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act,
-                      Permission &perm) {
+    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act) {
         // Location objects should always use LocationPolicy.
         MOZ_ASSERT(!WrapperFactory::IsLocationObject(js::UnwrapObject(wrapper)));
-
-        if (AccessCheck::isCrossOriginAccessPermitted(cx, wrapper, id, act)) {
-            perm = PermitPropertyAccess;
-            return true;
-        }
-        perm = DenyAccess;
-        JSAutoCompartment ac(cx, wrapper);
+        return AccessCheck::isCrossOriginAccessPermitted(cx, wrapper, id, act);
+    }
+    static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
         AccessCheck::deny(cx, id);
         return false;
     }
 };
 
 // We need a special security policy for Location objects.
 //
 // Location objects are special because their effective principal is that of
@@ -109,46 +87,54 @@ struct CrossOriginAccessiblePropertiesOn
 // we need to do a dynamic security check to determine whether the outer window is
 // same-origin with the caller.
 //
 // So this policy first checks whether the access is something that any code,
 // same-origin or not, is allowed to make. If it isn't, it _also_ checks the
 // state of the outer window to determine whether we happen to be same-origin
 // at the moment.
 struct LocationPolicy : public Policy {
-    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act,
-                      Permission &perm) {
+    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act) {
         // We should only be dealing with Location objects here.
         MOZ_ASSERT(WrapperFactory::IsLocationObject(js::UnwrapObject(wrapper)));
 
-        // Default to deny.
-        perm = DenyAccess;
-
         // Location object security is complicated enough. Don't allow punctures.
         if (act != js::Wrapper::PUNCTURE &&
             (AccessCheck::isCrossOriginAccessPermitted(cx, wrapper, id, act) ||
              AccessCheck::isLocationObjectSameOrigin(cx, wrapper))) {
-            perm = PermitPropertyAccess;
             return true;
         }
-
-        JSAutoCompartment ac(cx, wrapper);
+        return false;
+    }
+    static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
         AccessCheck::deny(cx, id);
         return false;
     }
 };
 
 // This policy only permits access to properties if they appear in the
 // objects exposed properties list.
 struct ExposedPropertiesOnly : public Policy {
-    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act,
-                      Permission &perm);
+    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act);
+
+    static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
+        // For gets, silently fail.
+        if (act == js::Wrapper::GET)
+            return true;
+        // For sets,throw an exception.
+        AccessCheck::deny(cx, id);
+        return false;
+    }
 };
 
 // Components specific policy
 struct ComponentsObjectPolicy : public Policy {
-    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act,
-                      Permission &perm);
+    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act);
+
+    static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
+        AccessCheck::deny(cx, id);
+        return false;
+    }
 };
 
 }
 
 #endif /* __AccessCheck_h__ */
--- a/js/xpconnect/wrappers/FilteringWrapper.cpp
+++ b/js/xpconnect/wrappers/FilteringWrapper.cpp
@@ -24,34 +24,27 @@ FilteringWrapper<Base, Policy>::Filterin
 {
 }
 
 template <typename Base, typename Policy>
 FilteringWrapper<Base, Policy>::~FilteringWrapper()
 {
 }
 
-typedef Wrapper::Permission Permission;
-
-static const Permission PermitObjectAccess = Wrapper::PermitObjectAccess;
-static const Permission PermitPropertyAccess = Wrapper::PermitPropertyAccess;
-static const Permission DenyAccess = Wrapper::DenyAccess;
-
 template <typename Policy>
 static bool
 Filter(JSContext *cx, JSObject *wrapper, AutoIdVector &props)
 {
     size_t w = 0;
     for (size_t n = 0; n < props.length(); ++n) {
         jsid id = props[n];
-        Permission perm;
-        if (!Policy::check(cx, wrapper, id, Wrapper::GET, perm))
-            return false; // Error
-        if (perm != DenyAccess)
+        if (Policy::check(cx, wrapper, id, Wrapper::GET))
             props[w++] = id;
+        else if (JS_IsExceptionPending(cx))
+            return false;
     }
     props.resize(w);
     return true;
 }
 
 template <typename Base, typename Policy>
 bool
 FilteringWrapper<Base, Policy>::getOwnPropertyNames(JSContext *cx, JSObject *wrapper, AutoIdVector &props)
@@ -87,24 +80,26 @@ FilteringWrapper<Base, Policy>::iterate(
     return js::BaseProxyHandler::iterate(cx, wrapper, flags, vp);
 }
 
 template <typename Base, typename Policy>
 bool
 FilteringWrapper<Base, Policy>::enter(JSContext *cx, JSObject *wrapper, jsid id,
                                       Wrapper::Action act, bool *bp)
 {
-    Permission perm;
-    if (!Policy::check(cx, wrapper, id, act, perm)) {
-        *bp = false;
+    if (!Policy::check(cx, wrapper, id, act)) {
+        if (JS_IsExceptionPending(cx)) {
+            *bp = false;
+            return false;
+        }
+        JSAutoCompartment ac(cx, wrapper);
+        *bp = Policy::deny(cx, id, act);
         return false;
     }
     *bp = true;
-    if (perm == DenyAccess)
-        return false;
     return Base::enter(cx, wrapper, id, act, bp);
 }
 
 #define SOW FilteringWrapper<CrossCompartmentSecurityWrapper, OnlyIfSubjectIsSystem>
 #define SCSOW FilteringWrapper<SameCompartmentSecurityWrapper, OnlyIfSubjectIsSystem>
 #define XOW FilteringWrapper<XrayWrapper<CrossCompartmentSecurityWrapper>, \
                              CrossOriginAccessiblePropertiesOnly>
 #define DXOW   FilteringWrapper<XrayDOM, \