diff --git a/api/src/main/java/javax/jdo/identity/ObjectIdentity.java b/api/src/main/java/javax/jdo/identity/ObjectIdentity.java index ca8cd0a8e..f82c11500 100644 --- a/api/src/main/java/javax/jdo/identity/ObjectIdentity.java +++ b/api/src/main/java/javax/jdo/identity/ObjectIdentity.java @@ -83,7 +83,7 @@ public ObjectIdentity(Class pcClass, Object param) { + // NOI18N MSG.msg( "EXC_ObjectIdentityStringConstructionUsage", // NOI18N - paramString)); + sanitized(paramString))); } int indexOfDelimiter = paramString.indexOf(STRING_DELIMITER); if (indexOfDelimiter < 0) { @@ -92,7 +92,7 @@ public ObjectIdentity(Class pcClass, Object param) { + // NOI18N MSG.msg( "EXC_ObjectIdentityStringConstructionUsage", // NOI18N - paramString)); + sanitized(paramString))); } keyString = paramString.substring(indexOfDelimiter + 1); className = paramString.substring(0, indexOfDelimiter); @@ -106,6 +106,25 @@ public ObjectIdentity(Class pcClass, Object param) { /** Constructor only for Externalizable. */ public ObjectIdentity() {} + /** + * Replace ISO control characters (e.g. CR/LF) in untrusted text that is echoed into exception + * messages, so crafted input cannot forge additional log lines when the message is logged. + * + * @param text the untrusted text, possibly null + * @return the text with control characters replaced by spaces + */ + private static String sanitized(String text) { + if (text == null) { + return null; + } + StringBuilder buf = new StringBuilder(text.length()); + for (int i = 0; i < text.length(); i++) { + char c = text.charAt(i); + buf.append(Character.isISOControl(c) ? ' ' : c); + } + return buf.toString(); + } + /** * Return 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..e87c172a4 100644 --- a/api/src/main/java/javax/jdo/spi/JDOImplHelper.java +++ b/api/src/main/java/javax/jdo/spi/JDOImplHelper.java @@ -845,14 +845,40 @@ public static Object construct(String className, String keyString) { InstantiationException, IllegalAccessException, InvocationTargetException */ + /* Deliberately do not embed ex.toString() in the message: the failure + kind (ClassNotFoundException vs. NoSuchMethodException vs. constructor + failure) would let a caller probing with attacker-controlled identity + strings fingerprint which classes exist on the classpath. The original + exception stays attached as the nested exception for diagnostics. + Control characters are stripped from the echoed input so crafted + identity strings cannot forge log lines. */ throw new JDOUserException( msg.msg( "EXC_ObjectIdentityStringConstruction", // NOI18N - new Object[] {ex.toString(), className, keyString}), + new Object[] {"", sanitized(className), sanitized(keyString)}), ex); } } + /** + * Replace ISO control characters (e.g. CR/LF) in untrusted text that is echoed into exception + * messages, so crafted input cannot forge additional log lines when the message is logged. + * + * @param text the untrusted text, possibly null + * @return the text with control characters replaced by spaces + */ + private static String sanitized(String text) { + if (text == null) { + return null; + } + StringBuilder buf = new StringBuilder(text.length()); + for (int i = 0; i < text.length(); i++) { + char c = text.charAt(i); + buf.append(Character.isISOControl(c) ? ' ' : c); + } + return buf.toString(); + } + /** * Get the DateFormat instance for the default locale from the VM. This requires the following * privileges for JDOImplHelper in the security permissions file: permission diff --git a/api/src/main/resources/javax/jdo/Bundle.properties b/api/src/main/resources/javax/jdo/Bundle.properties index 208ef4561..6c7ade8f6 100644 --- a/api/src/main/resources/javax/jdo/Bundle.properties +++ b/api/src/main/resources/javax/jdo/Bundle.properties @@ -66,7 +66,6 @@ EXC_StringWrongLength: There must be exactly one character in the id in the inpu EXC_IllegalEventType:The event type is outside the range of valid event types. EXC_SingleFieldIdentityNullParameter: The identity must not be null. EXC_ObjectIdentityStringConstruction: The identity instance could not be constructed. \ -\nThe exception thrown was: "{0}". \ \nParsed the class name as "{1}" and key as "{2}". EXC_ObjectIdentityStringConstructionNoDelimiter: Missing delimiter ":". EXC_ObjectIdentityStringConstructionTooShort: Parameter is too short. diff --git a/api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java b/api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java index 05e7bae0a..a1757bfe4 100644 --- a/api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java +++ b/api/src/test/java/javax/jdo/identity/ObjectIdentityTest.java @@ -221,6 +221,55 @@ void testBadStringConstructorNoPublicStringConstructor() { validateNestedException(ex, NoSuchMethodException.class); } + @Test + void testBadStringConstructorMessageUniformAcrossFailureKinds() { + // The construct-failure message must not reveal whether the named class + // exists on the classpath: ClassNotFoundException and NoSuchMethodException + // must produce the same text (modulo the echoed class name). The original + // exception remains available as the nested exception. + JDOUserException notFound = + Assertions.assertThrows( + JDOUserException.class, + () -> new ObjectIdentity(Object.class, "no.such.Clazz:yy"), + "Failed to catch expected exception."); + JDOUserException noCtor = + Assertions.assertThrows( + JDOUserException.class, + () -> + new ObjectIdentity( + Object.class, + "javax.jdo.identity.ObjectIdentityTest$BadIdClassNoStringConstructor:yy"), + "Failed to catch expected exception."); + String normalizedNotFound = notFound.getMessage().replace("no.such.Clazz", "CLASS"); + String normalizedNoCtor = + noCtor + .getMessage() + .replace( + "javax.jdo.identity.ObjectIdentityTest$BadIdClassNoStringConstructor", "CLASS"); + Assertions.assertEquals( + normalizedNotFound, + normalizedNoCtor, + "Exception message must not distinguish failure kinds (classpath oracle)."); + Assertions.assertFalse( + notFound.getMessage().contains("ClassNotFoundException"), + "Exception message must not embed the underlying exception type."); + } + + @Test + void testBadStringConstructorControlCharactersNotEchoed() { + JDOUserException ex = + Assertions.assertThrows( + JDOUserException.class, + () -> new ObjectIdentity(Object.class, "no.such.Clazz:evil\nFORGED LOG LINE"), + "Failed to catch expected exception."); + Assertions.assertFalse( + ex.getMessage().contains("evil\nFORGED"), + "Control characters from the identity string must not be echoed verbatim."); + Assertions.assertTrue( + ex.getMessage().contains("evil FORGED"), + "Sanitized identity string should still be echoed for diagnostics."); + } + @Test void testBadStringConstructorIllegalArgument() { JDOUserException ex =