Add additional checks in ObjectInputStreamreplicant-4.2-0004replicant-4.2

Thanks to Jann Horn for reporting a bug in ObjectInputStream
and sending the initial patch.

Add some checks that the class of an object
being deserialized still conforms to the requirements
for serialization.

Add some checks that the class being deserialized matches
the type information (enum, serializable, externalizable)
held in the stream.

Delayed static initialization of classes until the
type of the class has been validated against the stream
content in some cases.

Added more tests.

Bug: 15874291
(cherry picked from commit 738c833d38d41f8f76eb7e77ab39add82b1ae1e2)

Change-Id: I9f5437ed60936882de56589537176466624e631d

Signed-off-by: Neil Fuller <nfuller@google.com>
Tested-by: Moritz Bandemer <replicant@posteo.mx>

3 files changed, 91 insertions, 19 deletions

diff --git a/luni/src/main/java/java/io/ObjectInputStream.java b/luni/src/main/java/java/io/ObjectInputStream.java
index 0476901..963b7e9 100644
--- a/luni/src/main/java/java/io/ObjectInputStream.java
+++ b/luni/src/main/java/java/io/ObjectInputStream.java
@@ -1078,7 +1078,8 @@ public class ObjectInputStream extends InputStream implements ObjectInput, Objec
      * @see #readFields
      * @see #readObject()
      */
-    private void readFieldValues(Object obj, ObjectStreamClass classDesc) throws OptionalDataException, ClassNotFoundException, IOException {
+    private void readFieldValues(Object obj, ObjectStreamClass classDesc)
+            throws OptionalDataException, ClassNotFoundException, IOException {
         // Now we must read all fields and assign them to the receiver
         ObjectStreamField[] fields = classDesc.getLoadFields();
         fields = (fields == null) ? ObjectStreamClass.NO_FIELDS : fields;
@@ -1602,6 +1603,9 @@ public class ObjectInputStream extends InputStream implements ObjectInput, Objec
             ClassNotFoundException, IOException {
         // read classdesc for Enum first
         ObjectStreamClass classDesc = readEnumDesc();
+
+        Class enumType = classDesc.checkAndGetTcEnumClass();
+
         int newHandle = nextHandle();
         // read name after class desc
         String name;
@@ -1623,9 +1627,11 @@ public class ObjectInputStream extends InputStream implements ObjectInput, Objec
 
         Enum<?> result;
         try {
-            result = Enum.valueOf((Class) classDesc.forClass(), name);
+            result = Enum.valueOf(enumType, name);
         } catch (IllegalArgumentException e) {
-            throw new InvalidObjectException(e.getMessage());
+            InvalidObjectException ioe = new InvalidObjectException(e.getMessage());
+            ioe.initCause(e);
+            throw ioe;
         }
         registerObjectRead(result, newHandle, unshared);
         return result;
@@ -1809,9 +1815,10 @@ public class ObjectInputStream extends InputStream implements ObjectInput, Objec
             throw missingClassDescriptor();
         }
 
+        Class<?> objectClass = classDesc.checkAndGetTcObjectClass();
+
         int newHandle = nextHandle();
-        Class<?> objectClass = classDesc.forClass();
-        Object result = null;
+        Object result;
         Object registeredResult = null;
         if (objectClass != null) {
             // Now we know which class to instantiate and which constructor to
@@ -2100,8 +2107,7 @@ public class ObjectInputStream extends InputStream implements ObjectInput, Objec
      *             if the source stream does not contain readable serialized
      *             objects.
      */
-    protected void readStreamHeader() throws IOException,
-            StreamCorruptedException {
+    protected void readStreamHeader() throws IOException, StreamCorruptedException {
         if (input.readShort() == STREAM_MAGIC
                 && input.readShort() == STREAM_VERSION) {
             return;
@@ -2301,7 +2307,7 @@ public class ObjectInputStream extends InputStream implements ObjectInput, Objec
                 // not primitive class
                 // Use the first non-null ClassLoader on the stack. If null, use
                 // the system class loader
-                cls = Class.forName(className, true, callerClassLoader);
+                cls = Class.forName(className, false, callerClassLoader);
             }
         }
         return cls;
@@ -2375,8 +2381,7 @@ public class ObjectInputStream extends InputStream implements ObjectInput, Objec
             throws InvalidClassException {
 
         Class<?> localClass = loadedStreamClass.forClass();
-        ObjectStreamClass localStreamClass = ObjectStreamClass
-                .lookupStreamClass(localClass);
+        ObjectStreamClass localStreamClass = ObjectStreamClass.lookupStreamClass(localClass);
 
         if (loadedStreamClass.getSerialVersionUID() != localStreamClass
                 .getSerialVersionUID()) {
diff --git a/luni/src/main/java/java/io/ObjectStreamClass.java b/luni/src/main/java/java/io/ObjectStreamClass.java
index a28489a..79a04e5 100644
--- a/luni/src/main/java/java/io/ObjectStreamClass.java
+++ b/luni/src/main/java/java/io/ObjectStreamClass.java
@@ -1068,7 +1068,6 @@ public class ObjectStreamClass implements Serializable {
             tlc.put(cl, cachedValue);
         }
         return cachedValue;
-
     }
 
     /**
@@ -1298,4 +1297,72 @@ public class ObjectStreamClass implements Serializable {
     public String toString() {
         return getName() + ": static final long serialVersionUID =" + getSerialVersionUID() + "L;";
     }
+
+    /**
+     * Checks the local class to make sure it is valid for {@link ObjectStreamConstants#TC_OBJECT}
+     * deserialization. Also performs some sanity checks of the stream data. This method is used
+     * during deserialization to confirm the local class is likely to be compatible with the coming
+     * stream data, but before an instance is instantiated.
+     *
+     * @hide used internally during deserialization
+     */
+    public Class<?> checkAndGetTcObjectClass() throws InvalidClassException {
+        // We check some error possibilities that might cause problems later.
+        boolean wasSerializable = (flags & ObjectStreamConstants.SC_SERIALIZABLE) != 0;
+        boolean wasExternalizable = (flags & ObjectStreamConstants.SC_EXTERNALIZABLE) != 0;
+        if (wasSerializable == wasExternalizable) {
+            throw new InvalidClassException(
+                    getName() + " stream data is corrupt: SC_SERIALIZABLE=" + wasSerializable
+                            + " SC_EXTERNALIZABLE=" + wasExternalizable
+                            + ", classDescFlags must have one or the other");
+        }
+
+        // TC_ENUM is handled elsewhere. See checkAndGetTcEnumClass().
+        if (isEnum()) {
+            throw new InvalidClassException(
+                    getName() + " local class is incompatible: Local class is an enum, streamed"
+                            + " data is tagged with TC_OBJECT");
+        }
+
+        // isSerializable() is true if the local class implements Serializable. Externalizable
+        // classes are also Serializable via inheritance.
+        if (!isSerializable()) {
+            throw new InvalidClassException(getName() + " local class is incompatible: Not"
+                    + " Serializable");
+        }
+
+        // The stream class was externalizable, but is only serializable locally.
+        if (wasExternalizable != isExternalizable()) {
+            throw new InvalidClassException(
+                    getName() + " local class is incompatible: Local class is Serializable, stream"
+                            + " data requires Externalizable");
+        }
+
+        // The following are left unchecked and thus are treated leniently at this point.
+        // SC_BLOCK_DATA may be set iff SC_EXTERNALIZABLE is set AND version 2 of the protocol is in
+        // use.
+        // SC_ENUM should not be set.
+
+        return forClass();
+    }
+
+    /**
+     * Checks the local class to make sure it is valid for {@link ObjectStreamConstants#TC_ENUM}
+     * deserialization. This method is used during deserialization to confirm the local class is
+     * likely to be compatible with the coming stream data, but before an instance is instantiated.
+     *
+     * @hide used internally during deserialization
+     */
+    public Class<?> checkAndGetTcEnumClass() throws InvalidClassException {
+        if (!isEnum()) {
+            throw new InvalidClassException(
+                    getName() + " local class is incompatible: Local class is not an enum,"
+                            + " streamed data is tagged with TC_ENUM");
+        }
+
+        // The stream flags are expected to be SC_SERIALIZABLE | SC_ENUM but these and the
+        // other flags are not used when reading enum data so they are treated leniently.
+
+        return forClass();
+    }
 }
diff --git a/luni/src/main/java/java/io/ObjectStreamConstants.java b/luni/src/main/java/java/io/ObjectStreamConstants.java
index 8228b33..95f8b03 100644
--- a/luni/src/main/java/java/io/ObjectStreamConstants.java
+++ b/luni/src/main/java/java/io/ObjectStreamConstants.java
@@ -149,25 +149,25 @@ public abstract interface ObjectStreamConstants {
     // Flags that indicate if the object was serializable, externalizable
     // and had a writeObject method when dumped.
     /**
-     * Bit mask for the {@code flag} field in ObjectStreamClass. Indicates
-     * that a serializable class has its own {@code writeObject} method.
+     * Bit mask for the {@code flag} field in {@link ObjectStreamClass}. Indicates
+     * that a {@link Serializable} class has its own {@code writeObject} method.
      */
     public static final byte SC_WRITE_METHOD = 0x01; // If SC_SERIALIZABLE
 
     /**
-     * Bit mask for the {@code flag} field in ObjectStreamClass. Indicates
-     * that a class is serializable.
+     * Bit mask for the {@code flag} field in {@link ObjectStreamClass}. Indicates
+     * that a class implements {@link Serializable} but not {@link Externalizable}.
      */
     public static final byte SC_SERIALIZABLE = 0x02;
 
     /**
-     * Bit mask for the {@code flag} field in ObjectStreamClass. Indicates
-     * that a class is externalizable.
+     * Bit mask for the {@code flag} field in {@link ObjectStreamClass}. Indicates
+     * that a class implements {@link Externalizable}.
      */
     public static final byte SC_EXTERNALIZABLE = 0x04;
 
     /**
-     * Bit mask for the {@code flag} field in ObjectStreamClass. Indicates
+     * Bit mask for the {@code flag} field in {@link ObjectStreamClass}. Indicates
      * that an externalizable class is written in block data mode.
      */
     public static final byte SC_BLOCK_DATA = 0x08; // If SC_EXTERNALIZABLE
@@ -178,7 +178,7 @@ public abstract interface ObjectStreamConstants {
     public static final byte TC_ENUM = 0x7E;
 
     /**
-     * Bit mask for the {@code flag} field in ObjectStreamClass. Indicates
+     * Bit mask for the {@code flag} field in {@link ObjectStreamClass}. Indicates
      * that a class is an enum type.
      */
     public static final byte SC_ENUM = 0x10;