Add additional checks in ObjectInputStream

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 738c833)

Change-Id: I9f5437ed60936882de56589537176466624e631d

Author: nfuller

luni/src/main/java/java/io/ObjectInputStream.java

@@ -1052,7 +1052,8 @@ private void readFieldValues(EmulatedFieldsForLoading emulatedFields)
      * @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;
@@ -1574,6 +1575,9 @@ private Object readEnum(boolean unshared) throws OptionalDataException,
             ClassNotFoundException, IOException {
         // read classdesc for Enum first
         ObjectStreamClass classDesc = readEnumDesc();
+
+        Class enumType = classDesc.checkAndGetTcEnumClass();
+
         int newHandle = nextHandle();
         // read name after class desc
         String name;
@@ -1595,9 +1599,11 @@ private Object readEnum(boolean unshared) throws OptionalDataException,
 
         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;
@@ -1781,9 +1787,10 @@ private Object readNewObject(boolean unshared)
             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
@@ -2055,8 +2062,7 @@ public short readShort() throws IOException {
      *             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;
@@ -2256,7 +2262,7 @@ protected Class<?> resolveClass(ObjectStreamClass osClass)
                 // 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;
@@ -2330,8 +2336,7 @@ private void verifyAndInit(ObjectStreamClass loadedStreamClass)
             throws InvalidClassException {
 
         Class<?> localClass = loadedStreamClass.forClass();
-        ObjectStreamClass localStreamClass = ObjectStreamClass
-                .lookupStreamClass(localClass);
+        ObjectStreamClass localStreamClass = ObjectStreamClass.lookupStreamClass(localClass);
 
         if (loadedStreamClass.getSerialVersionUID() != localStreamClass
                 .getSerialVersionUID()) {

luni/src/main/java/java/io/ObjectStreamClass.java

@@ -1088,7 +1088,6 @@ static ObjectStreamClass lookupStreamClass(Class<?> cl) {
             tlc.put(cl, cachedValue);
         }
         return cachedValue;
-
     }
 
     /**
@@ -1318,4 +1317,72 @@ private int primitiveSize(Class<?> type) {
     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();
+    }
 }

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;