Bug 800915 - Reimplement PUNCTURE consumers in terms of isSafeToUnwrap() and remove PUNCTURE API. r=mrbkap
authorBobby Holley <bobbyholley@gmail.com>
Mon, 12 Nov 2012 17:35:32 -0800
changeset 113048 c567df2244f59d53a71467afa461a12adad8ba7f
parent 113047 8e3d976d5dc5c0029cf810a83f7f5bcecf0aeac9
child 113049 de9fff3a523240b175c0aca822911011b926d64d
push id23848
push useremorley@mozilla.com
push dateTue, 13 Nov 2012 16:29:34 +0000
treeherdermozilla-central@d56d537a1843 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs800915
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 800915 - Reimplement PUNCTURE consumers in terms of isSafeToUnwrap() and remove PUNCTURE API. r=mrbkap
js/src/jswrapper.cpp
js/src/jswrapper.h
js/xpconnect/wrappers/AccessCheck.cpp
js/xpconnect/wrappers/AccessCheck.h
--- a/js/src/jswrapper.cpp
+++ b/js/src/jswrapper.cpp
@@ -126,23 +126,21 @@ js::UnwrapOneChecked(JSContext *cx, Hand
     // Checked unwraps should never unwrap outer windows.
     if (!obj->isWrapper() ||
         JS_UNLIKELY(!!obj->getClass()->ext.innerObject))
     {
         return obj;
     }
 
     Wrapper *handler = Wrapper::wrapperHandler(obj);
-    bool rvOnFailure;
-    if (!handler->enter(cx, obj, JSID_VOID, Wrapper::PUNCTURE, &rvOnFailure))
-        return rvOnFailure ? (JSObject*) obj : NULL;
-    JSObject *ret = Wrapper::wrappedObject(obj);
-    JS_ASSERT(ret);
-
-    return ret;
+    if (!handler->isSafeToUnwrap()) {
+        JS_ReportError(cx, "Permission denied to access object");
+        return NULL;
+    }
+    return Wrapper::wrappedObject(obj);
 }
 
 bool
 js::IsCrossCompartmentWrapper(RawObject wrapper)
 {
     return wrapper->isWrapper() &&
            !!(Wrapper::wrapperHandler(wrapper)->flags() & Wrapper::CROSS_COMPARTMENT);
 }
@@ -217,35 +215,32 @@ Wrapper::enumerate(JSContext *cx, JSObje
 {
     JS_ASSERT(!hasPrototype()); // Should never be called when there's a prototype.
     // if we refuse to perform this action, props remains empty
     static jsid id = JSID_VOID;
     GET(DirectProxyHandler::enumerate(cx, wrapper, props));
 }
 
 /*
- * Ordinarily, the convert trap would require a PUNCTURE. However, the default
+ * Ordinarily, the convert trap would require unwrapping. However, the default
  * implementation of convert, JS_ConvertStub, obtains a default value by calling
- * the toString/valueOf method on the wrapper, if any. Doing a PUNCTURE in this
- * case would be overly conservative. To make matters worse, XPConnect sometimes
+ * the toString/valueOf method on the wrapper, if any. Throwing if we can't unwrap
+ * in this case would be overly conservative. To make matters worse, XPConnect sometimes
  * installs a custom convert trap that obtains a default value by calling the
  * toString method on the wrapper. Doing a puncture in this case would be overly
- * conservative as well. We deal with these anomalies by clearing the pending
- * exception and falling back to the DefaultValue algorithm whenever the
- * PUNCTURE fails.
+ * conservative as well. We deal with these anomalies by falling back to the DefaultValue
+ * algorithm whenever unwrapping is forbidden.
  */
 bool
 Wrapper::defaultValue(JSContext *cx, JSObject *wrapper_, JSType hint, Value *vp)
 {
     RootedObject wrapper(cx, wrapper_);
 
-    bool status;
-    if (!enter(cx, wrapper_, JSID_VOID, PUNCTURE, &status)) {
+    if (!wrapperHandler(wrapper)->isSafeToUnwrap()) {
         RootedValue v(cx);
-        JS_ClearPendingException(cx);
         if (!DefaultValue(cx, wrapper, hint, &v))
             return false;
         *vp = v;
         return true;
     }
     /*
      * We enter the compartment of the wrappee here, even if we're not a cross
      * compartment wrapper. Moreover, cross compartment wrappers do not enter
--- a/js/src/jswrapper.h
+++ b/js/src/jswrapper.h
@@ -30,18 +30,17 @@ class JS_FRIEND_API(Wrapper) : public Di
 {
     unsigned mFlags;
     bool mSafeToUnwrap;
 
   public:
     enum Action {
         GET,
         SET,
-        CALL,
-        PUNCTURE
+        CALL
     };
 
     enum Flags {
         CROSS_COMPARTMENT = 1 << 0,
         LAST_USED_FLAG = CROSS_COMPARTMENT
     };
 
     /*
@@ -67,34 +66,17 @@ class JS_FRIEND_API(Wrapper) : public Di
     }
 
     /* Policy enforcement traps.
      *
      * enter() allows the policy to specify whether the caller may perform |act|
      * on the underlying object's |id| property. In the case when |act| is CALL,
      * |id| is generally JSID_VOID.
      *
-     * The |act| parameter to enter() specifies the action being performed. GET,
-     * SET, and CALL are self-explanatory, but PUNCTURE requires more
-     * explanation:
-     *
-     * GET and SET allow for a very fine-grained security membrane, through
-     * which access can be granted or denied on a per-property, per-object, and
-     * per-action basis. Sometimes though, we just want to asks if we can access
-     * _everything_ behind the wrapper barrier. For example, when the structured
-     * clone algorithm runs up against a cross-compartment wrapper, it needs to
-     * know whether it can enter the compartment and keep cloning, or whether it
-     * should throw. This is the role of PUNCTURE.
-     *
-     * PUNCTURE allows the policy to specify whether the wrapper barrier may
-     * be lifted - that is to say, whether the caller is allowed to access
-     * anything that the wrapped object could access. This is a very powerful
-     * permission, and thus should generally be denied for security wrappers
-     * except under very special circumstances. When |act| is PUNCTURE, |id|
-     * should be JSID_VOID.
+     * The |act| parameter to enter() specifies the action being performed.
      */
     virtual bool enter(JSContext *cx, JSObject *wrapper, jsid id, Action act,
                        bool *bp);
 
     explicit Wrapper(unsigned flags, bool hasPrototype = false);
 
     virtual ~Wrapper();
 
--- a/js/xpconnect/wrappers/AccessCheck.cpp
+++ b/js/xpconnect/wrappers/AccessCheck.cpp
@@ -226,21 +226,16 @@ AccessCheck::isCrossOriginAccessPermitte
     if (!XPCWrapper::GetSecurityManager())
         return true;
 
     if (act == Wrapper::CALL)
         return true;
 
     JSObject *obj = Wrapper::wrappedObject(wrapper);
 
-    // PUNCTURE Is always denied for cross-origin access.
-    if (act == Wrapper::PUNCTURE) {
-        return false;
-    }
-
     const char *name;
     js::Class *clasp = js::GetObjectClass(obj);
     NS_ASSERTION(Jsvalify(clasp) != &XrayUtils::HolderClass, "shouldn't have a holder here");
     if (clasp->ext.innerObject)
         name = "Window";
     else
         name = clasp->name;
 
@@ -340,19 +335,16 @@ IsInSandbox(JSContext *cx, JSObject *obj
 bool
 ExposedPropertiesOnly::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper::Action act)
 {
     JSObject *wrappedObject = Wrapper::wrappedObject(wrapper);
 
     if (act == Wrapper::CALL)
         return true;
 
-    if (act == Wrapper::PUNCTURE)
-        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);
 
--- a/js/xpconnect/wrappers/AccessCheck.h
+++ b/js/xpconnect/wrappers/AccessCheck.h
@@ -91,19 +91,17 @@ struct CrossOriginAccessiblePropertiesOn
 // 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) {
         // We should only be dealing with Location objects here.
         MOZ_ASSERT(WrapperFactory::IsLocationObject(js::UnwrapObject(wrapper)));
 
-        // Location object security is complicated enough. Don't allow punctures.
-        if (act != js::Wrapper::PUNCTURE &&
-            (AccessCheck::isCrossOriginAccessPermitted(cx, wrapper, id, act) ||
+        if ((AccessCheck::isCrossOriginAccessPermitted(cx, wrapper, id, act) ||
              AccessCheck::isLocationObjectSameOrigin(cx, wrapper))) {
             return true;
         }
         return false;
     }
     static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
         AccessCheck::deny(cx, id);
         return false;