Object identity revisited

api/src/main/java/javax/jdo/identity/ObjectIdentity.java

@@ -64,6 +64,11 @@ private static <T> T doPrivileged(PrivilegedAction<T> privilegedAction) {
   /**
    * Constructor with class and key.
    *
+   * <p>If <code>param</code> is a <code>String</code>, it is parsed as
+   * "&lt;className&gt;:&lt;keyString&gt;" and the key instance is constructed by {@link
+   * JDOImplHelper#construct(String, String)}. Because the String form may originate from an
+   * untrusted source, only allowed identity key classes may be named by it.
+   *
    * @param pcClass the class
    * @param param the key
    */

api/src/main/java/javax/jdo/spi/JDOImplHelper.java

@@ -29,6 +29,7 @@
 import java.text.ParsePosition;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Currency;
@@ -41,6 +42,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.WeakHashMap;
+import java.util.stream.Collectors;
 import javax.jdo.Constants;
 import javax.jdo.JDOException;
 import javax.jdo.JDOFatalInternalException;
@@ -160,11 +162,8 @@ private static Set<String> createUserConfigurableStandardProperties() {
 
   static Set<String> createUserConfigurableStandardPropertiesLowerCased() {
     Set<String> mixedCased = createUserConfigurableStandardProperties();
-    Set<String> lowerCased = new HashSet<>(mixedCased.size());
-
-    for (String propertyName : mixedCased) {
-      lowerCased.add(propertyName.toLowerCase());
-    }
+    Set<String> lowerCased =
+        mixedCased.stream().map(String::toLowerCase).collect(Collectors.toSet());
     return Collections.unmodifiableSet(lowerCased);
   }
 
@@ -814,11 +813,48 @@ private static boolean isClassLoadable(String className) {
     }
   }
 
+  /** Package prefixes of key classes always allowed for String construction. */
+  private static final String[] DEFAULT_ALLOWED_KEY_CLASS_PREFIXES = {
+    "java.lang.", "java.math.", "java.time." // NOI18N
+  };
+
+  /** Individual key classes always allowed for String construction. */
+  private static final Set<String> DEFAULT_ALLOWED_KEY_CLASS_NAMES =
+      new HashSet<>(
+          Arrays.asList(
+              "java.util.Date", // NOI18N
+              "java.util.Locale", // NOI18N
+              "java.util.Currency", // NOI18N
+              "java.util.UUID")); // NOI18N
+
+  /**
+   * Determine whether the given class may be instantiated from a String by {@link
+   * #construct(String, String)}. A class is allowed if it belongs to the built-in set of value
+   * classes.
+   *
+   * @param keyClass the candidate key class
+   * @return true if the class may be constructed from a String
+   */
+  private static boolean isIdentityKeyClassAllowed(Class<?> keyClass) {
+    String name = keyClass.getName();
+    for (String prefix : DEFAULT_ALLOWED_KEY_CLASS_PREFIXES) {
+      if (name.startsWith(prefix)) {
+        return true;
+      }
+    }
+    return DEFAULT_ALLOWED_KEY_CLASS_NAMES.contains(name);
+  }
+
   /**
    * Construct an instance of the parameter class, using the keyString as an argument to the
-   * constructor. If the class has a StringConstructor instance registered, use it. If not, try to
-   * find a constructor for the class with a single String argument. Otherwise, throw a
-   * JDOUserException.
+   * constructor. If the class has a StringConstructor instance registered, use it. If not, and the
+   * class is an allowed identity key class, try to find a constructor for the class with a single
+   * String argument. Otherwise, throw a JDOUserException.
+   *
+   * <p>Because the class name typically originates from the String form of an identity, which may
+   * come from an untrusted source, only classes with a registered {@link StringConstructor}, or the
+   * built-in value classes are instantiated. The named class is loaded without initializing it, so
+   * no code of a disallowed class runs.
    *
    * @param className the name of the class
    * @param keyString the String parameter for the constructor
@@ -827,16 +863,22 @@ private static boolean isClassLoadable(String className) {
   public static Object construct(String className, String keyString) {
     StringConstructor stringConstructor;
     try {
-      Class<?> keyClass = Class.forName(className);
+      // load without initializing: no static initializer of an unvetted class may run
+      Class<?> keyClass = Class.forName(className, false, JDOImplHelper.class.getClassLoader());
       synchronized (stringConstructorMap) {
         stringConstructor = stringConstructorMap.get(keyClass);
       }
       if (stringConstructor != null) {
         return stringConstructor.construct(keyString);
-      } else {
-        Constructor<?> keyConstructor = keyClass.getConstructor(String.class);
-        return keyConstructor.newInstance(keyString);
       }
+      if (!isIdentityKeyClassAllowed(keyClass)) {
+        throw new JDOUserException(
+            msg.msg(
+                "EXC_ObjectIdentityStringConstructionKeyClassNotAllowed", // NOI18N
+                className));
+      }
+      Constructor<?> keyConstructor = keyClass.getConstructor(String.class);
+      return keyConstructor.newInstance(keyString);
     } catch (JDOException ex) {
       throw ex;
     } catch (Exception ex) {

api/src/main/resources/javax/jdo/Bundle.properties

@@ -73,6 +73,9 @@ EXC_ObjectIdentityStringConstructionTooShort: Parameter is too short.
 EXC_ObjectIdentityStringConstructionUsage: The instance could not be constructed \
 from the parameter String "{0}". \
 \nThe parameter String is of the form "<className>:<keyString>".
+EXC_ObjectIdentityStringConstructionKeyClassNotAllowed: The class "{0}" is not an \
+allowed identity key class, so no instance was constructed from the String form \
+of the identity.
 EXC_CreateKeyAsObjectMustNotBeCalled: The method createKeyAsObject must not be called \
 because the keyAsObject field must never be null for this class.
 EXC_CurrencyStringConstructorIllegalArgument: The instance could not be constructed \

api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java

@@ -118,25 +118,6 @@ void testCurrencyConstructor() {
     Assertions.assertNotEquals(c1, c3, "Not equal ObjectIdentity instances compare equal");
   }
 
-  @Test
-  void testStringConstructor() {
-    ObjectIdentity c1 =
-        new ObjectIdentity(Object.class, "javax.jdo.identity.ObjectIdentityTest$IdClass:1");
-    ObjectIdentity c2 =
-        new ObjectIdentity(Object.class, "javax.jdo.identity.ObjectIdentityTest$IdClass:1");
-    ObjectIdentity c3 =
-        new ObjectIdentity(Object.class, "javax.jdo.identity.ObjectIdentityTest$IdClass:2");
-    Assertions.assertEquals(c1, c2, "Equal ObjectIdentity instances compare not equal.");
-    Assertions.assertNotEquals(c1, c3, "Not equal ObjectIdentity instances compare equal");
-  }
-
-  @Test
-  void testToStringConstructor() {
-    ObjectIdentity c1 = new ObjectIdentity(Object.class, new IdClass(1));
-    ObjectIdentity c2 = new ObjectIdentity(Object.class, c1.toString());
-    Assertions.assertEquals(c1, c2, "Equal ObjectIdentity instances compare not equal.");
-  }
-
   @Test
   void testDateCompareTo() {
     ObjectIdentity c1 = new ObjectIdentity(Object.class, new Date(1));
@@ -186,51 +167,11 @@ void testBadStringConstructorNoDelimiter() {
   }
 
   @Test
-  void testBadStringConstructorBadClassName() {
-    JDOUserException ex =
-        Assertions.assertThrows(
-            JDOUserException.class,
-            () -> new ObjectIdentity(Object.class, "xx:yy"),
-            "Failed to catch expected ClassNotFoundException.");
-    validateNestedException(ex, ClassNotFoundException.class);
-  }
-
-  @Test
-  void testBadStringConstructorNoStringConstructor() {
-    JDOUserException ex =
-        Assertions.assertThrows(
-            JDOUserException.class,
-            () ->
-                new ObjectIdentity(
-                    Object.class,
-                    "javax.jdo.identity.ObjectIdentityTest$BadIdClassNoStringConstructor:yy"),
-            "Failed to catch expected NoSuchMethodException.");
-    validateNestedException(ex, NoSuchMethodException.class);
-  }
-
-  @Test
-  void testBadStringConstructorNoPublicStringConstructor() {
-    JDOUserException ex =
-        Assertions.assertThrows(
-            JDOUserException.class,
-            () ->
-                new ObjectIdentity(
-                    Object.class,
-                    "javax.jdo.identity.ObjectIdentityTest$BadIdClassNoPublicStringConstructor:yy"),
-            "Failed to catch expected NoSuchMethodException.");
-    validateNestedException(ex, NoSuchMethodException.class);
-  }
-
-  @Test
-  void testBadStringConstructorIllegalArgument() {
-    JDOUserException ex =
-        Assertions.assertThrows(
-            JDOUserException.class,
-            () ->
-                new ObjectIdentity(
-                    Object.class, "javax.jdo.identity.ObjectIdentityTest$IdClass:yy"),
-            "Failed to catch expected InvocationTargetException.");
-    validateNestedException(ex, InvocationTargetException.class);
+  void testNotAnAllowedIdentityKeyClass() {
+    Assertions.assertThrows(
+        JDOUserException.class,
+        () -> new ObjectIdentity(Object.class, "javax.jdo.identity.ObjectIdentityTest$IdClass:yy"),
+        "Failed to catch expected JDOUserException.");
   }
 
   @Test
@@ -425,6 +366,34 @@ void testGetKeyAsObject() {
     Assertions.assertEquals(c1.getKeyAsObject(), new IdClass(1), "keyAsObject doesn't match.");
   }
 
+  @Test
+  void testStringConstructorDefaultAllowedKeyClass() {
+    // java.math.* is in the built-in allowlist; no system property entry needed
+    ObjectIdentity c1 = new ObjectIdentity(Object.class, "java.math.BigDecimal:123.45");
+    Assertions.assertEquals(new BigDecimal("123.45"), c1.getKeyAsObject());
+  }
+
+  @Test
+  void testStringConstructorDisallowedKeyClass() {
+    // java.io.File has a public (String) constructor but is not an allowed key class
+    Assertions.assertThrows(
+        JDOUserException.class,
+        () -> new ObjectIdentity(Object.class, "java.io.File:/tmp/x"),
+        "Failed to catch expected JDOUserException for disallowed key class.");
+  }
+
+  @Test
+  void testStringConstructorDisallowedKeyClassNotInitialized() {
+    Assertions.assertThrows(
+        JDOUserException.class,
+        () ->
+            new ObjectIdentity(
+                Object.class, "javax.jdo.identity.ObjectIdentityTest$StaticInitCanary:x"),
+        "Failed to catch expected JDOUserException for disallowed key class.");
+    Assertions.assertFalse(
+        canaryStaticInitRun, "Static initializer of a disallowed key class must not run.");
+  }
+
   private <T> void validateNestedException(JDOUserException ex, Class<T> expected) {
     Throwable[] nesteds = ex.getNestedExceptions();
     if (nesteds == null || nesteds.length != 1) {
@@ -476,6 +445,20 @@ public boolean equals(Object obj) {
     }
   }
 
+  /** Set by StaticInitCanary's static initializer; must remain false. */
+  static boolean canaryStaticInitRun = false;
+
+  /** Not in the allowlist; its static initializer must never run via ObjectIdentity. */
+  public static class StaticInitCanary {
+    static {
+      canaryStaticInitRun = true;
+    }
+
+    public StaticInitCanary(String str) {
+      // This method body is intentionally left blank
+    }
+  }
+
   public static class BadIdClassNoStringConstructor {}
 
   public static class BadIdClassNoPublicStringConstructor {

tck/src/main/resources/conf/jdo-signatures.txt

@@ -254,7 +254,7 @@ public interface javax.jdo.Constants {
         = "javax/jdo/jdoquery_3_0.xsd";
     static String ANONYMOUS_PERSISTENCE_MANAGER_FACTORY_NAME
         = "";
-    public static final String TX_READ_UNCOMMITTED = "read-uncommitted"; 
+    public static final String TX_READ_UNCOMMITTED = "read-uncommitted";
     public static final String TX_READ_COMMITTED = "read-committed"; 
     public static final String TX_REPEATABLE_READ = "repeatable-read"; 
     public static final String TX_SNAPSHOT = "snapshot";