diff --git a/api/src/main/java/javax/jdo/identity/ObjectIdentity.java b/api/src/main/java/javax/jdo/identity/ObjectIdentity.java index ca8cd0a8e..5cb0e8665 100644 --- a/api/src/main/java/javax/jdo/identity/ObjectIdentity.java +++ b/api/src/main/java/javax/jdo/identity/ObjectIdentity.java @@ -64,6 +64,11 @@ private static T doPrivileged(PrivilegedAction privilegedAction) { /** * Constructor with class and key. * + *

If param is a String, it is parsed as + * "<className>:<keyString>" 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 */ diff --git a/api/src/main/java/javax/jdo/spi/JDOImplHelper.java b/api/src/main/java/javax/jdo/spi/JDOImplHelper.java index 10fd95c40..95522374e 100644 --- a/api/src/main/java/javax/jdo/spi/JDOImplHelper.java +++ b/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 createUserConfigurableStandardProperties() { static Set createUserConfigurableStandardPropertiesLowerCased() { Set mixedCased = createUserConfigurableStandardProperties(); - Set lowerCased = new HashSet<>(mixedCased.size()); - - for (String propertyName : mixedCased) { - lowerCased.add(propertyName.toLowerCase()); - } + Set 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 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. + * + *

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) { diff --git a/api/src/main/resources/javax/jdo/Bundle.properties b/api/src/main/resources/javax/jdo/Bundle.properties index 208ef4561..141ab2d23 100644 --- a/api/src/main/resources/javax/jdo/Bundle.properties +++ b/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 ":". +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 \ diff --git a/api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java b/api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java index 05e7bae0a..5dded2e8b 100644 --- a/api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java +++ b/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 void validateNestedException(JDOUserException ex, Class 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 { diff --git a/tck/src/main/resources/conf/jdo-signatures.txt b/tck/src/main/resources/conf/jdo-signatures.txt index 01cc8ad77..ec4ecb72a 100644 --- a/tck/src/main/resources/conf/jdo-signatures.txt +++ b/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";