7165628: Issues with java.lang.invoke.MethodHandles.Lookup

Summary: Base SecurityManager checks on either of Lookup.lookupClass or caller class; also clarify Lookup access checks.
Reviewed-by: twisti

src/share/classes/java/lang/invoke/MethodHandles.java

@@ -407,7 +407,7 @@ public class MethodHandles {
          * an access$N method.
          */
         Lookup() {
-            this(getCallerClassAtEntryPoint(), ALL_MODES);
+            this(getCallerClassAtEntryPoint(false), ALL_MODES);
             // make sure we haven't accidentally picked up a privileged class:
             checkUnprivilegedlookupClass(lookupClass);
         }
@@ -461,8 +461,8 @@ public class MethodHandles {
                 && !VerifyAccess.isSamePackageMember(this.lookupClass, requestedLookupClass)) {
                 newModes &= ~PRIVATE;
             }
-            if (newModes == PUBLIC
-                && !VerifyAccess.isClassAccessible(requestedLookupClass, this.lookupClass)) {
+            if ((newModes & PUBLIC) != 0
+                && !VerifyAccess.isClassAccessible(requestedLookupClass, this.lookupClass, allowedModes)) {
                 // The requested class it not accessible from the lookup class.
                 // No permissions.
                 newModes = 0;
@@ -540,13 +540,17 @@ public class MethodHandles {
             }
         }
 
-        // call this from an entry point method in Lookup with extraFrames=0.
-        private static Class<?> getCallerClassAtEntryPoint() {
+        /* Obtain the external caller class, when called from Lookup.<init> or a first-level subroutine. */
+        private static Class<?> getCallerClassAtEntryPoint(boolean inSubroutine) {
             final int CALLER_DEPTH = 4;
+            //  Stack for the constructor entry point (inSubroutine=false):
             // 0: Reflection.getCC, 1: getCallerClassAtEntryPoint,
             // 2: Lookup.<init>, 3: MethodHandles.*, 4: caller
+            //  The stack is slightly different for a subroutine of a Lookup.find* method:
+            // 2: Lookup.*, 3: Lookup.find*.*, 4: caller
             // Note:  This should be the only use of getCallerClass in this file.
-            assert(Reflection.getCallerClass(CALLER_DEPTH-1) == MethodHandles.class);
+            assert(Reflection.getCallerClass(CALLER_DEPTH-2) == Lookup.class);
+            assert(Reflection.getCallerClass(CALLER_DEPTH-1) == (inSubroutine ? Lookup.class : MethodHandles.class));
             return Reflection.getCallerClass(CALLER_DEPTH);
         }
 
@@ -1087,7 +1091,7 @@ return mh1;
 
         void checkSymbolicClass(Class<?> refc) throws IllegalAccessException {
             Class<?> caller = lookupClassOrNull();
-            if (caller != null && !VerifyAccess.isClassAccessible(refc, caller))
+            if (caller != null && !VerifyAccess.isClassAccessible(refc, caller, allowedModes))
                 throw new MemberName(refc).makeAccessException("symbolic reference class is not public", this);
         }
 
@@ -1102,7 +1106,13 @@ return mh1;
             // Step 1:
             smgr.checkMemberAccess(refc, Member.PUBLIC);
             // Step 2:
-            if (!VerifyAccess.classLoaderIsAncestor(lookupClass, refc))
+            Class<?> callerClass = ((allowedModes & PRIVATE) != 0
+                                    ? lookupClass  // for strong access modes, no extra check
+                                    // next line does stack walk magic; do not refactor:
+                                    : getCallerClassAtEntryPoint(true));
+            if (!VerifyAccess.classLoaderIsAncestor(lookupClass, refc) ||
+                (callerClass != lookupClass &&
+                 !VerifyAccess.classLoaderIsAncestor(callerClass, refc)))
                 smgr.checkPackageAccess(VerifyAccess.getPackageName(refc));
             // Step 3:
             if (m.isPublic()) return;
@@ -1153,9 +1163,10 @@ return mh1;
             int requestedModes = fixmods(mods);  // adjust 0 => PACKAGE
             if ((requestedModes & allowedModes) != 0
                 && VerifyAccess.isMemberAccessible(refc, m.getDeclaringClass(),
-                                                   mods, lookupClass()))
+                                                   mods, lookupClass(), allowedModes))
                 return;
             if (((requestedModes & ~allowedModes) & PROTECTED) != 0
+                && (allowedModes & PACKAGE) != 0
                 && VerifyAccess.isSamePackage(m.getDeclaringClass(), lookupClass()))
                 // Protected members can also be checked as if they were package-private.
                 return;
@@ -1170,9 +1181,9 @@ return mh1;
                                (defc == refc ||
                                 Modifier.isPublic(refc.getModifiers())));
             if (!classOK && (allowedModes & PACKAGE) != 0) {
-                classOK = (VerifyAccess.isClassAccessible(defc, lookupClass()) &&
+                classOK = (VerifyAccess.isClassAccessible(defc, lookupClass(), ALL_MODES) &&
                            (defc == refc ||
-                            VerifyAccess.isClassAccessible(refc, lookupClass())));
+                            VerifyAccess.isClassAccessible(refc, lookupClass(), ALL_MODES)));
             }
             if (!classOK)
                 return "class is not public";

src/share/classes/sun/invoke/util/VerifyAccess.java

@@ -37,6 +37,8 @@ public class VerifyAccess {
     private VerifyAccess() { }  // cannot instantiate
 
     private static final int PACKAGE_ONLY = 0;
+    private static final int PACKAGE_ALLOWED = java.lang.invoke.MethodHandles.Lookup.PACKAGE;
+    private static final int PROTECTED_OR_PACKAGE_ALLOWED = (PACKAGE_ALLOWED|PROTECTED);
     private static final int ALL_ACCESS_MODES = (PUBLIC|PRIVATE|PROTECTED|PACKAGE_ONLY);
     private static final boolean ALLOW_NESTMATE_ACCESS = false;
 
@@ -82,14 +84,19 @@ public class VerifyAccess {
     public static boolean isMemberAccessible(Class<?> refc,  // symbolic ref class
                                              Class<?> defc,  // actual def class
                                              int      mods,  // actual member mods
-                                             Class<?> lookupClass) {
+                                             Class<?> lookupClass,
+                                             int      allowedModes) {
+        if (allowedModes == 0)  return false;
+        assert((allowedModes & PUBLIC) != 0 &&
+               (allowedModes & ~(ALL_ACCESS_MODES|PACKAGE_ALLOWED)) == 0);
         // Usually refc and defc are the same, but if they differ, verify them both.
         if (refc != defc) {
-            if (!isClassAccessible(refc, lookupClass)) {
+            if (!isClassAccessible(refc, lookupClass, allowedModes)) {
                 // Note that defc is verified in the switch below.
                 return false;
             }
-            if ((mods & (ALL_ACCESS_MODES|STATIC)) == (PROTECTED|STATIC)) {
+            if ((mods & (ALL_ACCESS_MODES|STATIC)) == (PROTECTED|STATIC) &&
+                (allowedModes & PROTECTED_OR_PACKAGE_ALLOWED) != 0) {
                 // Apply the special rules for refc here.
                 if (!isRelatedClass(refc, lookupClass))
                     return isSamePackage(defc, lookupClass);
@@ -98,19 +105,28 @@ public class VerifyAccess {
                 // a superclass of the lookup class.
             }
         }
-        if (defc == lookupClass)
+        if (defc == lookupClass &&
+            (allowedModes & PRIVATE) != 0)
             return true;        // easy check; all self-access is OK
         switch (mods & ALL_ACCESS_MODES) {
         case PUBLIC:
             if (refc != defc)  return true;  // already checked above
-            return isClassAccessible(refc, lookupClass);
+            return isClassAccessible(refc, lookupClass, allowedModes);
         case PROTECTED:
-            return isSamePackage(defc, lookupClass) || isPublicSuperClass(defc, lookupClass);
-        case PACKAGE_ONLY:
-            return isSamePackage(defc, lookupClass);
+            if ((allowedModes & PROTECTED_OR_PACKAGE_ALLOWED) != 0 &&
+                isSamePackage(defc, lookupClass))
+                return true;
+            if ((allowedModes & PROTECTED) != 0 &&
+                isPublicSuperClass(defc, lookupClass))
+                return true;
+            return false;
+        case PACKAGE_ONLY:  // That is, zero.  Unmarked member is package-only access.
+            return ((allowedModes & PACKAGE_ALLOWED) != 0 &&
+                    isSamePackage(defc, lookupClass));
         case PRIVATE:
             // Loosened rules for privates follows access rules for inner classes.
             return (ALLOW_NESTMATE_ACCESS &&
+                    (allowedModes & PRIVATE) != 0 &&
                     isSamePackageMember(defc, lookupClass));
         default:
             throw new IllegalArgumentException("bad modifiers: "+Modifier.toString(mods));
@@ -138,11 +154,16 @@ public class VerifyAccess {
      * @param refc the symbolic reference class to which access is being checked (C)
      * @param lookupClass the class performing the lookup (D)
      */
-    public static boolean isClassAccessible(Class<?> refc, Class<?> lookupClass) {
+    public static boolean isClassAccessible(Class<?> refc, Class<?> lookupClass,
+                                            int allowedModes) {
+        if (allowedModes == 0)  return false;
+        assert((allowedModes & PUBLIC) != 0 &&
+               (allowedModes & ~(ALL_ACCESS_MODES|PACKAGE_ALLOWED)) == 0);
         int mods = refc.getModifiers();
         if (isPublic(mods))
             return true;
-        if (isSamePackage(lookupClass, refc))
+        if ((allowedModes & PACKAGE_ALLOWED) != 0 &&
+            isSamePackage(lookupClass, refc))
             return true;
         return false;
     }
@@ -157,7 +178,7 @@ public class VerifyAccess {
         assert(!class1.isArray() && !class2.isArray());
         if (class1 == class2)
             return true;
-        if (!loadersAreRelated(class1.getClassLoader(), class2.getClassLoader(), false))
+        if (class1.getClassLoader() != class2.getClassLoader())
             return false;
         String name1 = class1.getName(), name2 = class2.getName();
         int dot = name1.lastIndexOf('.');

test/java/lang/invoke/AccessControlTest_subpkg/Acquaintance_remote.java

+package test.java.lang.invoke.AccessControlTest_subpkg;
+import test.java.lang.invoke.AccessControlTest;
+import java.lang.invoke.*;
+import static java.lang.invoke.MethodHandles.*;
+
+// This guy tests access from outside the package test.java.lang.invoke:
+public class Acquaintance_remote {
+    public static Lookup[] lookups() {
+        return new Lookup[] {
+            Acquaintance_remote.lookup_in_remote(),
+            Remote_subclass.lookup_in_subclass(),
+            Remote_hidden.lookup_in_hidden()
+        };
+    }
+
+    public static Lookup lookup_in_remote() {
+        return MethodHandles.lookup();
+    }
+    static public      void pub_in_remote() { }
+    static protected   void pro_in_remote() { }
+    static /*package*/ void pkg_in_remote() { }
+    static private     void pri_in_remote() { }
+
+    static public class Remote_subclass extends AccessControlTest {
+        static Lookup lookup_in_subclass() {
+            return MethodHandles.lookup();
+        }
+        static public      void pub_in_subclass() { }
+        static protected   void pro_in_subclass() { }
+        static /*package*/ void pkg_in_subclass() { }
+        static private     void pri_in_subclass() { }
+    }
+    static /*package*/ class Remote_hidden {
+        static Lookup lookup_in_hidden() {
+            return MethodHandles.lookup();
+        }
+        static public      void pub_in_hidden() { }
+        static protected   void pro_in_hidden() { }
+        static /*package*/ void pkg_in_hidden() { }
+        static private     void pri_in_hidden() { }
+    }
+}