Harden readObject(ObjectInputStream) method argument checks (#4098)

Signed-off-by: SunWeb3Sec <infosecpt@gmail.com>
Co-authored-by: Volkan Yazıcı <volkan@yazi.ci>

Author: SunWeb3Sec

log4j-1.2-api/src/main/java/org/apache/log4j/Level.java

@@ -25,6 +25,7 @@
 import java.io.Serializable;
 import org.apache.log4j.helpers.OptionConverter;
 import org.apache.logging.log4j.util.Strings;
+import org.apache.logging.log4j.util.internal.SerializationUtil;
 
 /**
  * Defines the minimum set of levels recognized by the system, that is
@@ -214,6 +215,7 @@ public static Level toLevel(final String sArg, final Level defaultLevel) {
      * @throws ClassNotFoundException if class not found.
      */
     private void readObject(final ObjectInputStream s) throws IOException, ClassNotFoundException {
+        SerializationUtil.assertFiltered(s);
         s.defaultReadObject();
         level = s.readInt();
         syslogEquivalent = s.readInt();

log4j-1.2-api/src/test/java/org/apache/log4j/util/SerializationTestHelper.java

@@ -24,9 +24,14 @@
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
+import java.util.Arrays;
+import java.util.Collection;
 import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.util.Constants;
+import org.apache.logging.log4j.util.FilteredObjectInputStream;
 
 /**
  * Utiities for serialization tests.
@@ -103,11 +108,23 @@ public static void assertStreamEquals(
      * @throws Exception thrown on IO or deserialization exception.
      */
     public static Object deserializeStream(final String witness) throws Exception {
-        try (final ObjectInputStream objIs = new ObjectInputStream(new FileInputStream(witness))) {
+        try (final ObjectInputStream objIs = newObjectInputStream(new FileInputStream(witness))) {
             return objIs.readObject();
         }
     }
 
+    private static ObjectInputStream newObjectInputStream(final InputStream in) throws IOException {
+        if (Constants.JAVA_MAJOR_VERSION == 8) {
+            // FilteredObjectInputStream's default allow-list covers `org.apache.logging.log4j.` but
+            // not the `org.apache.log4j.` 1.2-compatibility namespace, so we have to enumerate the
+            // 1.2 classes that the tests in this module deserialize on Java 8.
+            final Collection<String> allowedLog4j12Classes =
+                    Arrays.asList("org.apache.log4j.Level", "org.apache.log4j.LevelTest$CustomLevel");
+            return new FilteredObjectInputStream(in, allowedLog4j12Classes);
+        }
+        return new ObjectInputStream(in);
+    }
+
     /**
      * Creates a clone by serializing object and deserializing byte stream.
      *
@@ -123,7 +140,7 @@ public static Object serializeClone(final Object obj) throws IOException, ClassN
         }
 
         final ByteArrayInputStream src = new ByteArrayInputStream(memOut.toByteArray());
-        final ObjectInputStream objIs = new ObjectInputStream(src);
+        final ObjectInputStream objIs = newObjectInputStream(src);
 
         return objIs.readObject();
     }

log4j-api-test/src/main/java/org/apache/logging/log4j/test/SerializableMatchers.java

@@ -20,7 +20,9 @@
 import static org.hamcrest.core.IsInstanceOf.any;
 
 import java.io.Serializable;
-import org.apache.commons.lang3.SerializationUtils;
+import java.util.Collection;
+import java.util.Collections;
+import org.apache.logging.log4j.test.junit.SerialUtil;
 import org.hamcrest.FeatureMatcher;
 import org.hamcrest.Matcher;
 
@@ -32,10 +34,19 @@
 public final class SerializableMatchers {
 
     public static <T extends Serializable> Matcher<T> serializesRoundTrip(final Matcher<T> matcher) {
+        return serializesRoundTrip(matcher, Collections.emptySet());
+    }
+
+    /**
+     * Same as {@link #serializesRoundTrip(Matcher)} but extends the default deserialization
+     * allow-list on Java 8 (see {@link SerialUtil#deserialize(byte[], Collection)}).
+     */
+    public static <T extends Serializable> Matcher<T> serializesRoundTrip(
+            final Matcher<T> matcher, final Collection<String> allowedExtraClasses) {
         return new FeatureMatcher<T, T>(matcher, "serializes round trip", "serializes round trip") {
             @Override
             protected T featureValueOf(final T actual) {
-                return SerializationUtils.roundtrip(actual);
+                return SerialUtil.deserialize(SerialUtil.serialize(actual), allowedExtraClasses);
             }
         };
     }
@@ -52,5 +63,13 @@ public static Matcher<? super Serializable> serializesRoundTrip() {
         return serializesRoundTrip(any(Serializable.class));
     }
 
+    /**
+     * Same as {@link #serializesRoundTrip()} but extends the default deserialization allow-list on
+     * Java 8 (see {@link SerialUtil#deserialize(byte[], Collection)}).
+     */
+    public static Matcher<? super Serializable> serializesRoundTrip(final Collection<String> allowedExtraClasses) {
+        return serializesRoundTrip(any(Serializable.class), allowedExtraClasses);
+    }
+
     private SerializableMatchers() {}
 }

log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/SerialUtil.java

@@ -24,6 +24,8 @@
 import java.io.ObjectOutput;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
+import java.util.Collection;
+import java.util.Collections;
 import org.apache.logging.log4j.test.internal.annotation.SuppressFBWarnings;
 import org.apache.logging.log4j.util.Constants;
 import org.apache.logging.log4j.util.FilteredObjectInputStream;
@@ -68,11 +70,25 @@ public static byte[] serialize(final Serializable... objs) {
      * @param data byte array representing the serialized object
      * @return the deserialized object
      */
-    @SuppressWarnings("unchecked")
     @SuppressFBWarnings("OBJECT_DESERIALIZATION")
     public static <T> T deserialize(final byte[] data) {
+        return deserialize(data, Collections.emptySet());
+    }
+
+    /**
+     * Deserialize an object from the specified byte array using a {@link FilteredObjectInputStream}
+     * extended with the supplied allow-list (Java 8 only — Java 9+ uses the JVM's serialization
+     * filter, so the allow-list is ignored).
+     * @param data byte array representing the serialized object
+     * @param allowedExtraClasses fully-qualified class names to add to {@link
+     *     FilteredObjectInputStream}'s default allow-list on Java 8
+     * @return the deserialized object
+     */
+    @SuppressWarnings("unchecked")
+    @SuppressFBWarnings("OBJECT_DESERIALIZATION")
+    public static <T> T deserialize(final byte[] data, final Collection<String> allowedExtraClasses) {
         try {
-            final ObjectInputStream ois = getObjectInputStream(data);
+            final ObjectInputStream ois = getObjectInputStream(data, allowedExtraClasses);
             return (T) ois.readObject();
         } catch (final Exception ex) {
             throw new IllegalStateException("Could not deserialize", ex);
@@ -86,8 +102,18 @@ public static <T> T deserialize(final byte[] data) {
      */
     @SuppressFBWarnings("OBJECT_DESERIALIZATION")
     public static ObjectInputStream getObjectInputStream(final byte[] data) throws IOException {
+        return getObjectInputStream(data, Collections.emptySet());
+    }
+
+    /**
+     * Creates an {@link ObjectInputStream} adapted to the current Java version, extended with the
+     * supplied allow-list on Java 8.
+     */
+    @SuppressFBWarnings("OBJECT_DESERIALIZATION")
+    public static ObjectInputStream getObjectInputStream(
+            final byte[] data, final Collection<String> allowedExtraClasses) throws IOException {
         final ByteArrayInputStream bas = new ByteArrayInputStream(data);
-        return getObjectInputStream(bas);
+        return getObjectInputStream(bas, allowedExtraClasses);
     }
 
     /**
@@ -97,8 +123,18 @@ public static ObjectInputStream getObjectInputStream(final byte[] data) throws I
      */
     @SuppressFBWarnings("OBJECT_DESERIALIZATION")
     public static ObjectInputStream getObjectInputStream(final InputStream stream) throws IOException {
+        return getObjectInputStream(stream, Collections.emptySet());
+    }
+
+    /**
+     * Creates an {@link ObjectInputStream} adapted to the current Java version, extended with the
+     * supplied allow-list on Java 8.
+     */
+    @SuppressFBWarnings("OBJECT_DESERIALIZATION")
+    public static ObjectInputStream getObjectInputStream(
+            final InputStream stream, final Collection<String> allowedExtraClasses) throws IOException {
         return Constants.JAVA_MAJOR_VERSION == 8
-                ? new FilteredObjectInputStream(stream)
+                ? new FilteredObjectInputStream(stream, allowedExtraClasses)
                 : new ObjectInputStream(stream);
     }
 }

log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/package-info.java

@@ -15,7 +15,7 @@
  * limitations under the license.
  */
 @Export
-@Version("2.25.3")
+@Version("2.27.0")
 package org.apache.logging.log4j.test.junit;
 
 import org.osgi.annotation.bundle.Export;

log4j-api-test/src/main/java/org/apache/logging/log4j/test/package-info.java

@@ -15,7 +15,7 @@
  * limitations under the license.
  */
 @Export
-@Version("2.25.3")
+@Version("2.27.0")
 package org.apache.logging.log4j.test;
 
 import org.osgi.annotation.bundle.Export;

log4j-api-test/src/test/java/org/apache/logging/log4j/message/FormattedMessageTest.java

@@ -19,13 +19,9 @@
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
-import java.io.IOException;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
 import java.util.Locale;
 import org.apache.logging.log4j.test.junit.Mutable;
+import org.apache.logging.log4j.test.junit.SerialUtil;
 import org.apache.logging.log4j.util.Constants;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.parallel.ResourceAccessMode;
@@ -158,15 +154,9 @@ void testSafeAfterGetFormattedMessageIsCalled() { // LOG4J2-763
     }
 
     @Test
-    void testSerialization() throws IOException, ClassNotFoundException {
+    void testSerialization() {
         final FormattedMessage expected = new FormattedMessage("Msg", "a", "b", "c");
-        final ByteArrayOutputStream baos = new ByteArrayOutputStream();
-        try (final ObjectOutputStream out = new ObjectOutputStream(baos)) {
-            out.writeObject(expected);
-        }
-        final ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
-        final ObjectInputStream in = new ObjectInputStream(bais);
-        final FormattedMessage actual = (FormattedMessage) in.readObject();
+        final FormattedMessage actual = SerialUtil.deserialize(SerialUtil.serialize(expected));
         assertEquals(expected, actual);
         assertEquals(expected.getFormat(), actual.getFormat());
         assertEquals(expected.getFormattedMessage(), actual.getFormattedMessage());

log4j-api-test/src/test/java/org/apache/logging/log4j/message/LocalizedMessageTest.java

@@ -18,10 +18,9 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
-import java.io.Serializable;
 import java.util.Locale;
-import org.apache.commons.lang3.SerializationUtils;
 import org.apache.logging.log4j.test.junit.Mutable;
+import org.apache.logging.log4j.test.junit.SerialUtil;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.parallel.ResourceAccessMode;
 import org.junit.jupiter.api.parallel.ResourceLock;
@@ -33,8 +32,8 @@
 @ResourceLock(value = Resources.LOCALE, mode = ResourceAccessMode.READ)
 class LocalizedMessageTest {
 
-    private <T extends Serializable> T roundtrip(final T msg) {
-        return SerializationUtils.roundtrip(msg);
+    private LocalizedMessage roundtrip(final LocalizedMessage msg) {
+        return SerialUtil.deserialize(SerialUtil.serialize(msg));
     }
 
     @Test

log4j-api-test/src/test/java/org/apache/logging/log4j/message/ObjectArrayMessageTest.java

@@ -19,6 +19,7 @@
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 
+import org.apache.logging.log4j.test.junit.SerialUtil;
 import org.junit.jupiter.api.Test;
 
 /**
@@ -38,4 +39,16 @@ void testGetParameters() {
     void testGetThrowable() {
         assertNull(OBJECT_ARRAY_MESSAGE.getThrowable());
     }
+
+    /**
+     * Round-trips through a filtered stream (see {@link SerialUtil#getObjectInputStream})
+     * to verify that {@code readObject}'s new {@code SerializationUtil.assertFiltered}
+     * check accepts streams that carry a filter.
+     */
+    @Test
+    void testSerializableRoundTripThroughFilteredStream() {
+        final ObjectArrayMessage original = new ObjectArrayMessage("A", "B", "C");
+        final ObjectArrayMessage restored = SerialUtil.deserialize(SerialUtil.serialize(original));
+        assertArrayEquals(original.getParameters(), restored.getParameters());
+    }
 }

log4j-api-test/src/test/java/org/apache/logging/log4j/message/StringFormattedMessageTest.java

@@ -20,13 +20,9 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
-import java.io.IOException;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
 import java.util.Locale;
 import org.apache.logging.log4j.test.junit.Mutable;
+import org.apache.logging.log4j.test.junit.SerialUtil;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.parallel.ResourceAccessMode;
 import org.junit.jupiter.api.parallel.ResourceLock;
@@ -115,15 +111,9 @@ void testSafeAfterGetFormattedMessageIsCalled() { // LOG4J2-763
     }
 
     @Test
-    void testSerialization() throws IOException, ClassNotFoundException {
+    void testSerialization() {
         final StringFormattedMessage expected = new StringFormattedMessage("Msg", "a", "b", "c");
-        final ByteArrayOutputStream baos = new ByteArrayOutputStream();
-        try (final ObjectOutputStream out = new ObjectOutputStream(baos)) {
-            out.writeObject(expected);
-        }
-        final ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
-        final ObjectInputStream in = new ObjectInputStream(bais);
-        final StringFormattedMessage actual = (StringFormattedMessage) in.readObject();
+        final StringFormattedMessage actual = SerialUtil.deserialize(SerialUtil.serialize(expected));
         assertEquals(expected, actual);
         assertEquals(expected.getFormat(), actual.getFormat());
         assertEquals(expected.getFormattedMessage(), actual.getFormattedMessage());