8210496: Improve filtering for classes with security sensitive fields

Reviewed-by: plevart, mchung

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

@@ -54,6 +54,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -643,6 +644,10 @@ class Lookup {
         /** The allowed sorts of members which may be looked up (PUBLIC, etc.). */
         private final int allowedModes;
 
+        static {
+            Reflection.registerFieldsToFilter(Lookup.class, Set.of("lookupClass", "allowedModes"));
+        }
+
         /** A single-bit mask representing {@code public} access,
          *  which may contribute to the result of {@link #lookupModes lookupModes}.
          *  The value, {@code 0x01}, happens to be the same as the value of the

src/java.base/share/classes/jdk/internal/reflect/ConstantPool.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,7 @@
 package jdk.internal.reflect;
 
 import java.lang.reflect.*;
+import java.util.Set;
 
 /** Provides reflective access to the constant pools of classes.
     Currently this is needed to provide reflective access to annotations
@@ -104,7 +105,7 @@ private static Tag valueOf(byte v) {
   //
 
   static {
-      Reflection.registerFieldsToFilter(ConstantPool.class, new String[] { "constantPoolOop" });
+      Reflection.registerFieldsToFilter(ConstantPool.class, Set.of("constantPoolOop"));
   }
 
   // HotSpot-internal constant pool object (set by the VM, name known to the VM)

src/java.base/share/classes/jdk/internal/reflect/Reflection.java

@@ -25,13 +25,12 @@
 
 package jdk.internal.reflect;
 
-
 import java.lang.reflect.*;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import jdk.internal.HotSpotIntrinsicCandidate;
-import jdk.internal.loader.ClassLoaders;
 import jdk.internal.misc.VM;
 
 /** Common utility routines used by both java.lang and
@@ -43,18 +42,23 @@ public class Reflection {
         view, where they are sensitive or they may contain VM-internal objects.
         These Maps are updated very rarely. Rather than synchronize on
         each access, we use copy-on-write */
-    private static volatile Map<Class<?>,String[]> fieldFilterMap;
-    private static volatile Map<Class<?>,String[]> methodFilterMap;
+    private static volatile Map<Class<?>, Set<String>> fieldFilterMap;
+    private static volatile Map<Class<?>, Set<String>> methodFilterMap;
+    private static final String WILDCARD = "*";
+    public static final Set<String> ALL_MEMBERS = Set.of(WILDCARD);
 
     static {
-        Map<Class<?>,String[]> map = new HashMap<Class<?>,String[]>();
-        map.put(Reflection.class,
-            new String[] {"fieldFilterMap", "methodFilterMap"});
-        map.put(System.class, new String[] {"security"});
-        map.put(Class.class, new String[] {"classLoader"});
-        fieldFilterMap = map;
-
-        methodFilterMap = new HashMap<>();
+        fieldFilterMap = Map.of(
+            Reflection.class, ALL_MEMBERS,
+            AccessibleObject.class, ALL_MEMBERS,
+            Class.class, Set.of("classLoader"),
+            ClassLoader.class, ALL_MEMBERS,
+            Constructor.class, ALL_MEMBERS,
+            Field.class, ALL_MEMBERS,
+            Method.class, ALL_MEMBERS,
+            System.class, Set.of("security")
+        );
+        methodFilterMap = Map.of();
     }
 
     /** Returns the class of the caller of the method calling this method,
@@ -236,31 +240,31 @@ static boolean isSubclassOf(Class<?> queryClass,
 
     // fieldNames must contain only interned Strings
     public static synchronized void registerFieldsToFilter(Class<?> containingClass,
-                                              String ... fieldNames) {
+                                                           Set<String> fieldNames) {
         fieldFilterMap =
             registerFilter(fieldFilterMap, containingClass, fieldNames);
     }
 
     // methodNames must contain only interned Strings
     public static synchronized void registerMethodsToFilter(Class<?> containingClass,
-                                              String ... methodNames) {
+                                                            Set<String> methodNames) {
         methodFilterMap =
             registerFilter(methodFilterMap, containingClass, methodNames);
     }
 
-    private static Map<Class<?>,String[]> registerFilter(Map<Class<?>,String[]> map,
-            Class<?> containingClass, String ... names) {
+    private static Map<Class<?>, Set<String>> registerFilter(Map<Class<?>, Set<String>> map,
+                                                             Class<?> containingClass,
+                                                             Set<String> names) {
         if (map.get(containingClass) != null) {
             throw new IllegalArgumentException
                             ("Filter already registered: " + containingClass);
         }
-        map = new HashMap<Class<?>,String[]>(map);
-        map.put(containingClass, names);
+        map = new HashMap<>(map);
+        map.put(containingClass, Set.copyOf(names));
         return map;
     }
 
-    public static Field[] filterFields(Class<?> containingClass,
-                                       Field[] fields) {
+    public static Field[] filterFields(Class<?> containingClass, Field[] fields) {
         if (fieldFilterMap == null) {
             // Bootstrapping
             return fields;
@@ -276,35 +280,24 @@ public static Method[] filterMethods(Class<?> containingClass, Method[] methods)
         return (Method[])filter(methods, methodFilterMap.get(containingClass));
     }
 
-    private static Member[] filter(Member[] members, String[] filteredNames) {
+    private static Member[] filter(Member[] members, Set<String> filteredNames) {
         if ((filteredNames == null) || (members.length == 0)) {
             return members;
         }
+        Class<?> memberType = members[0].getClass();
+        if (filteredNames.contains(WILDCARD)) {
+            return (Member[]) Array.newInstance(memberType, 0);
+        }
         int numNewMembers = 0;
         for (Member member : members) {
-            boolean shouldSkip = false;
-            for (String filteredName : filteredNames) {
-                if (member.getName() == filteredName) {
-                    shouldSkip = true;
-                    break;
-                }
-            }
-            if (!shouldSkip) {
+            if (!filteredNames.contains(member.getName())) {
                 ++numNewMembers;
             }
         }
-        Member[] newMembers =
-            (Member[])Array.newInstance(members[0].getClass(), numNewMembers);
+        Member[] newMembers = (Member[])Array.newInstance(memberType, numNewMembers);
         int destIdx = 0;
         for (Member member : members) {
-            boolean shouldSkip = false;
-            for (String filteredName : filteredNames) {
-                if (member.getName() == filteredName) {
-                    shouldSkip = true;
-                    break;
-                }
-            }
-            if (!shouldSkip) {
+            if (!filteredNames.contains(member.getName())) {
                 newMembers[destIdx++] = member;
             }
         }

src/java.base/share/classes/jdk/internal/reflect/UnsafeStaticFieldAccessorImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2012, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,6 +28,8 @@
 import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
 import java.security.AccessController;
+import java.util.Set;
+
 import jdk.internal.misc.Unsafe;
 
 /** Base class for jdk.internal.misc.Unsafe-based FieldAccessors for static
@@ -40,7 +42,7 @@
 abstract class UnsafeStaticFieldAccessorImpl extends UnsafeFieldAccessorImpl {
     static {
         Reflection.registerFieldsToFilter(UnsafeStaticFieldAccessorImpl.class,
-                                          new String[] { "base" });
+                                          Set.of("base"));
     }
 
     protected final Object base; // base

src/jdk.unsupported/share/classes/sun/misc/Unsafe.java

@@ -33,6 +33,7 @@
 import sun.nio.ch.DirectBuffer;
 
 import java.lang.reflect.Field;
+import java.util.Set;
 
 
 /**
@@ -56,7 +57,7 @@
 public final class Unsafe {
 
     static {
-        Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
+        Reflection.registerMethodsToFilter(Unsafe.class, Set.of("getUnsafe"));
     }
 
     private Unsafe() {}

test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -49,6 +49,8 @@
 import static org.junit.Assert.assertTrue;
 
 import java.lang.annotation.Annotation;
+import java.lang.reflect.AccessibleObject;
+import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
@@ -762,6 +764,13 @@ private static boolean isHiddenFromReflection(ResolvedJavaField f) {
         if (f.getDeclaringClass().equals(metaAccess.lookupJavaType(Class.class)) && f.getName().equals("classLoader")) {
             return true;
         }
+        if (f.getDeclaringClass().equals(metaAccess.lookupJavaType(ClassLoader.class)) ||
+            f.getDeclaringClass().equals(metaAccess.lookupJavaType(AccessibleObject.class)) ||
+            f.getDeclaringClass().equals(metaAccess.lookupJavaType(Constructor.class)) ||
+            f.getDeclaringClass().equals(metaAccess.lookupJavaType(Field.class)) ||
+            f.getDeclaringClass().equals(metaAccess.lookupJavaType(Method.class))) {
+            return true;
+        }
         return false;
     }
 

test/jdk/java/lang/reflect/callerCache/AccessTest.java

@@ -113,25 +113,17 @@ public FinalField(String name) throws Exception {
             if (!Modifier.isFinal(f.getModifiers())) {
                 throw new RuntimeException("not a final field");
             }
-            makeFinalNonFinal(f);
         }
         public Void call() throws Exception {
             Members obj = isStatic ? null : new Members();
             try {
                 f.set(obj, 20);
-                checkValue(obj, 20);
+                throw new RuntimeException("IllegalAccessException expected");
             } catch (IllegalAccessException e) {
-                throw e;
+                // expected
             }
             return null;
         }
-
-        void checkValue(Object obj, int expected) throws Exception {
-            int value = (int) f.get(obj);
-            if (value != expected) {
-                throw new RuntimeException("unexpectd value: " + value);
-            }
-        }
     }
 
     public static class PublicFinalField extends FinalField {
@@ -157,15 +149,4 @@ public PrivateStaticFinalField() throws Exception {
             super("privateStaticFinalField");
         }
     }
-
-    private static void makeFinalNonFinal(Field f) throws ReflectiveOperationException {
-        Field modifiers = Field.class.getDeclaredField("modifiers");
-        modifiers.setAccessible(true);
-        modifiers.set(f, modifiers.getInt(f) & ~Modifier.FINAL);
-        f.setAccessible(true);
-
-        if (Modifier.isFinal(f.getModifiers())) {
-            throw new RuntimeException("should be a non-final field");
-        }
-    }
 }

test/jdk/jdk/internal/reflect/Reflection/Filtering.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @bug 8210496
+ * @modules java.base/jdk.internal.reflect
+ * @run testng Filtering
+ * @summary Test that security sensitive fields that filtered by core reflection
+ */
+
+import java.lang.reflect.*;
+import java.lang.invoke.MethodHandles.Lookup;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import static org.testng.Assert.assertTrue;
+
+public class Filtering {
+
+    @DataProvider(name = "sensitiveClasses")
+    private Object[][] sensitiveClasses() {
+        return new Object[][]{
+            { jdk.internal.reflect.Reflection.class, null },
+            { AccessibleObject.class, null },
+            { ClassLoader.class, null },
+            { Constructor.class, null },
+            { Field.class, null },
+            { Method.class, null },
+        };
+    }
+
+    @DataProvider(name = "sensitiveFields")
+    private Object[][] sensitiveFields() {
+        return new Object[][]{
+            { AccessibleObject.class, "override" },
+            { Class.class, "classLoader" },
+            { ClassLoader.class, "parent" },
+            { Field.class, "clazz" },
+            { Field.class, "modifiers" },
+            { Lookup.class, "lookupClass" },
+            { Lookup.class, "allowedModes" },
+            { Method.class, "clazz" },
+            { Method.class, "modifiers" },
+            { System.class, "security" },
+        };
+    }
+
+    @Test(dataProvider = "sensitiveClasses")
+    public void testClass(Class<?> clazz, Object ignore) throws Exception {
+        Field[] fields = clazz.getDeclaredFields();
+        assertTrue(fields.length == 0);
+    }
+
+    @Test(dataProvider = "sensitiveFields",
+          expectedExceptions = NoSuchFieldException.class)
+    public void testField(Class<?> clazz, String name) throws Exception {
+        clazz.getDeclaredField(name);
+    }
+
+}