6559775: Race allows defaultReadObject to be invoked instead of readFields during deserialization

Reviewed-by: hawtin

make/java/java/FILES_java.gmk

@@ -400,6 +400,7 @@ JAVA_JAVA_java = \
     java/io/FilePermission.java \
     java/io/Serializable.java \
     java/io/Externalizable.java \
+    java/io/SerialCallbackContext.java \
 	java/io/Bits.java \
 	java/io/ObjectInput.java \
 	java/io/ObjectInputStream.java \

src/share/classes/java/io/ObjectInputStream.java

 /*
- * Copyright (c) 1996, 2008, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -265,7 +265,7 @@ public class ObjectInputStream
      * object currently being deserialized and descriptor for current class.
      * Null when not during readObject upcall.
      */
-    private CallbackContext curContext;
+    private SerialCallbackContext curContext;
 
     /**
      * Creates an ObjectInputStream that reads from the specified InputStream.
@@ -1798,7 +1798,7 @@ public class ObjectInputStream
     private void readExternalData(Externalizable obj, ObjectStreamClass desc)
         throws IOException
     {
-        CallbackContext oldContext = curContext;
+        SerialCallbackContext oldContext = curContext;
         curContext = null;
         try {
             boolean blocked = desc.hasBlockExternalData();
@@ -1857,10 +1857,10 @@ public class ObjectInputStream
                     slotDesc.hasReadObjectMethod() &&
                     handles.lookupException(passHandle) == null)
                 {
-                    CallbackContext oldContext = curContext;
+                    SerialCallbackContext oldContext = curContext;
 
                     try {
-                        curContext = new CallbackContext(obj, slotDesc);
+                        curContext = new SerialCallbackContext(obj, slotDesc);
 
                         bin.setBlockDataMode(true);
                         slotDesc.invokeReadObject(obj, this);
@@ -3505,42 +3505,4 @@ public class ObjectInputStream
         }
     }
 
-    /**
-     * Context that during upcalls to class-defined readObject methods; holds
-     * object currently being deserialized and descriptor for current class.
-     * This context keeps a boolean state to indicate that defaultReadObject
-     * or readFields has already been invoked with this context or the class's
-     * readObject method has returned; if true, the getObj method throws
-     * NotActiveException.
-     */
-    private static class CallbackContext {
-        private final Object obj;
-        private final ObjectStreamClass desc;
-        private final AtomicBoolean used = new AtomicBoolean();
-
-        public CallbackContext(Object obj, ObjectStreamClass desc) {
-            this.obj = obj;
-            this.desc = desc;
-        }
-
-        public Object getObj() throws NotActiveException {
-            checkAndSetUsed();
-            return obj;
-        }
-
-        public ObjectStreamClass getDesc() {
-            return desc;
-        }
-
-        private void checkAndSetUsed() throws NotActiveException {
-            if (!used.compareAndSet(false, true)) {
-                 throw new NotActiveException(
-                      "not in readObject invocation or fields already read");
-            }
-        }
-
-        public void setUsed() {
-            used.set(true);
-        }
-    }
 }

src/share/classes/java/io/ObjectOutputStream.java

 /*
- * Copyright (c) 1996, 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -35,6 +35,7 @@ import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import static java.io.ObjectStreamClass.processQueue;
+import java.io.SerialCallbackContext;
 
 /**
  * An ObjectOutputStream writes primitive data types and graphs of Java objects
@@ -191,10 +192,12 @@ public class ObjectOutputStream
     private boolean enableReplace;
 
     // values below valid only during upcalls to writeObject()/writeExternal()
-    /** object currently being serialized */
-    private Object curObj;
-    /** descriptor for current class (null if in writeExternal()) */
-    private ObjectStreamClass curDesc;
+    /**
+     * Context during upcalls to class-defined writeObject methods; holds
+     * object currently being serialized and descriptor for current class.
+     * Null when not during writeObject upcall.
+     */
+    private SerialCallbackContext curContext;
     /** current PutField object */
     private PutFieldImpl curPut;
 
@@ -426,9 +429,11 @@ public class ObjectOutputStream
      *          <code>OutputStream</code>
      */
     public void defaultWriteObject() throws IOException {
-        if (curObj == null || curDesc == null) {
+        if ( curContext == null ) {
             throw new NotActiveException("not in call to writeObject");
         }
+        Object curObj = curContext.getObj();
+        ObjectStreamClass curDesc = curContext.getDesc();
         bout.setBlockDataMode(false);
         defaultWriteFields(curObj, curDesc);
         bout.setBlockDataMode(true);
@@ -446,9 +451,11 @@ public class ObjectOutputStream
      */
     public ObjectOutputStream.PutField putFields() throws IOException {
         if (curPut == null) {
-            if (curObj == null || curDesc == null) {
+            if (curContext == null) {
                 throw new NotActiveException("not in call to writeObject");
             }
+            Object curObj = curContext.getObj();
+            ObjectStreamClass curDesc = curContext.getDesc();
             curPut = new PutFieldImpl(curDesc);
         }
         return curPut;
@@ -1420,17 +1427,15 @@ public class ObjectOutputStream
      * writeExternal() method.
      */
     private void writeExternalData(Externalizable obj) throws IOException {
-        Object oldObj = curObj;
-        ObjectStreamClass oldDesc = curDesc;
         PutFieldImpl oldPut = curPut;
-        curObj = obj;
-        curDesc = null;
         curPut = null;
 
         if (extendedDebugInfo) {
             debugInfoStack.push("writeExternal data");
         }
+        SerialCallbackContext oldContext = curContext;
         try {
+            curContext = null;
             if (protocol == PROTOCOL_VERSION_1) {
                 obj.writeExternal(this);
             } else {
@@ -1440,13 +1445,12 @@ public class ObjectOutputStream
                 bout.writeByte(TC_ENDBLOCKDATA);
             }
         } finally {
+            curContext = oldContext;
             if (extendedDebugInfo) {
                 debugInfoStack.pop();
             }
         }
 
-        curObj = oldObj;
-        curDesc = oldDesc;
         curPut = oldPut;
     }
 
@@ -1461,12 +1465,9 @@ public class ObjectOutputStream
         for (int i = 0; i < slots.length; i++) {
             ObjectStreamClass slotDesc = slots[i].desc;
             if (slotDesc.hasWriteObjectMethod()) {
-                Object oldObj = curObj;
-                ObjectStreamClass oldDesc = curDesc;
                 PutFieldImpl oldPut = curPut;
-                curObj = obj;
-                curDesc = slotDesc;
                 curPut = null;
+                SerialCallbackContext oldContext = curContext;
 
                 if (extendedDebugInfo) {
                     debugInfoStack.push(
@@ -1474,18 +1475,19 @@ public class ObjectOutputStream
                         slotDesc.getName() + "\")");
                 }
                 try {
+                    curContext = new SerialCallbackContext(obj, slotDesc);
                     bout.setBlockDataMode(true);
                     slotDesc.invokeWriteObject(obj, this);
                     bout.setBlockDataMode(false);
                     bout.writeByte(TC_ENDBLOCKDATA);
                 } finally {
+                    curContext.setUsed();
+                    curContext = oldContext;
                     if (extendedDebugInfo) {
                         debugInfoStack.pop();
                     }
                 }
 
-                curObj = oldObj;
-                curDesc = oldDesc;
                 curPut = oldPut;
             } else {
                 defaultWriteFields(obj, slotDesc);

src/share/classes/java/io/SerialCallbackContext.java

+  /*
+   * %W% %E%
+   *
+   * Copyright (c) 2006, 2010  Oracle and/or its affiliates. All rights reserved.
+   * ORACLE PROPRIETARY/CONFIDENTIAL. Use is subject to license terms.
+   */
+
+  package java.io;
+
+  /**
+   * Context during upcalls from object stream to class-defined
+   * readObject/writeObject methods.
+   * Holds object currently being deserialized and descriptor for current class.
+   *
+   * This context keeps track of the thread it was constructed on, and allows
+   * only a single call of defaultReadObject, readFields, defaultWriteObject
+   * or writeFields which must be invoked on the same thread before the class's
+   * readObject/writeObject method has returned.
+   * If not set to the current thread, the getObj method throws NotActiveException.
+   */
+  final class SerialCallbackContext {
+      private final Object obj;
+      private final ObjectStreamClass desc;
+      /**
+       * Thread this context is in use by.
+       * As this only works in one thread, we do not need to worry about thread-safety.
+       */
+      private Thread thread;
+
+      public SerialCallbackContext(Object obj, ObjectStreamClass desc) {
+          this.obj = obj;
+          this.desc = desc;
+          this.thread = Thread.currentThread();
+      }
+
+      public Object getObj() throws NotActiveException {
+          checkAndSetUsed();
+          return obj;
+      }
+
+      public ObjectStreamClass getDesc() {
+          return desc;
+      }
+
+      private void checkAndSetUsed() throws NotActiveException {
+          if (thread != Thread.currentThread()) {
+               throw new NotActiveException(
+                "not in readObject invocation or fields already read");
+          }
+          thread = null;
+      }
+
+      public void setUsed() {
+          thread = null;
+      }
+  }
+
+

test/java/io/Serializable/6559775/README

+The testcase works well on dual core machines.
+The below output indicates a successful fix:
+
+Exception in thread "Thread-0" java.lang.NullPointerException
+        at java.io.ObjectInputStream.defaultReadObject(ObjectInputStream.java:476)
+        at SerialRace$1.run(SerialRace.java:33)
+        at java.lang.Thread.run(Thread.java:595)
+
+
+When the vulnerability exists, the output of the tescase is something like this:
+Available processors: 2
+Iteration 1
+java.io.NotActiveException: not in readObject invocation or fields already read
+        at java.io.ObjectInputStream$CallbackContext.checkAndSetUsed(ObjectInputStream.java:3437)
+        at java.io.ObjectInputStream$CallbackContext.getObj(ObjectInputStream.java:3427)
+        at java.io.ObjectInputStream.readFields(ObjectInputStream.java:514)
+        at SerialVictim.readObject(SerialVictim.java:19)
+        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
+        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
+        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
+        at java.lang.reflect.Method.invoke(Method.java:585)
+        at java.io.ObjectStreamClass.invokeReadObject(ObjectStreamClass.java:946)
+        at java.io.ObjectInputStream.readSerialData(ObjectInputStream.java:1809)
+        at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1719)
+        at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1305)
+        at java.io.ObjectInputStream.readObject(ObjectInputStream.java:348)
+        at SerialRace.main(SerialRace.java:65)
+Victim: ?
+Victim: $

test/java/io/Serializable/6559775/SerialRace.java

+/*
+ * @test %W% %E%
+ * @bug 6559775
+ * @summary  Race allows defaultReadObject to be invoked instead of readFields during deserialization
+ * @run shell Test6559775.sh
+*/
+
+import java.io.*;
+
+public class SerialRace {
+    public static void main(String[] args) throws Exception {
+        System.err.println(
+            "Available processors: "+
+            Runtime.getRuntime().availableProcessors()
+        );
+
+        final int perStream = 10000;
+
+        // Construct attack data.
+        ByteArrayOutputStream byteOut = new ByteArrayOutputStream();
+        {
+            ObjectOutputStream out = new ObjectOutputStream(byteOut);
+            char[] value = new char[] { '?' };
+            out.writeObject(value);
+            for (int i=0; i<perStream; ++i) {
+                SerialVictim orig = new SerialVictim(value);
+                out.writeObject(orig);
+            }
+            out.flush();
+        }
+        byte[] data = byteOut.toByteArray();
+
+        ByteArrayInputStream byteIn = new ByteArrayInputStream(data);
+        final ObjectInputStream in = new ObjectInputStream(byteIn);
+        final char[] value = (char[])in.readObject();
+        Thread thread = new Thread(new Runnable() { public void run() {
+                    for (;;) {
+                        try {
+                            // Attempt to interlope on other thread.
+                            in.defaultReadObject();
+                            // Got it.
+
+                            // Let other thread reach known state.
+                            Thread.sleep(1000);
+                            // This is the reference usually
+                            //   read in extended data.
+                            SerialVictim victim = (SerialVictim)
+                                in.readObject();
+                            System.err.println("Victim: "+victim);
+                            value[0] = '$';
+                            System.err.println("Victim: "+victim);
+                            return;
+                        } catch (java.io.NotActiveException exc) {
+                            // Not ready yet...
+                        } catch (java.lang.InterruptedException exc) {
+                            throw new Error(exc);
+                        } catch (IOException exc) {
+                            throw new Error(exc);
+                        } catch (ClassNotFoundException exc) {
+                            throw new Error(exc);
+                        }
+                    }
+        }});
+        thread.start();
+        Thread.yield();
+        // Normal reading from object stream.
+        // We hope the other thread catches us out between
+        //   setting up the call to SerialVictim.readObject and
+        //   the AtomicBoolean acquisition in readFields.
+        for (int i=0; i<perStream; ++i) {
+            try {
+                SerialVictim victim = (SerialVictim)in.readObject();
+            } catch (Exception exc) {
+                synchronized (System.err) {
+                    System.err.println("Iteration "+i);
+                    exc.printStackTrace();
+                }
+                // Allow atack thread to do it's business before close.
+                Thread.sleep(2000);
+                break;
+            }
+        }
+        // Stop the other thread.
+        in.close();
+    }
+}

test/java/io/Serializable/6559775/SerialVictim.java

+/**
+ * Instances of this class created through deserialization
+ *   should, in theory, be immutable.
+ * Instances created through the public constructor
+ *   are only to ease creation of the binary attack data.
+ */
+
+public class SerialVictim implements java.io.Serializable {
+    private char[] value;
+    public SerialVictim(char[] value) {
+        this.value = value;
+    }
+    //@Override
+    public String toString() {
+        return new String(value);
+    }
+    private void readObject(
+        java.io.ObjectInputStream in
+    ) throws java.io.IOException, java.lang.ClassNotFoundException {
+        java.io.ObjectInputStream.GetField fields = in.readFields();
+        // Clone before write should, in theory, make instance safe.
+        this.value = (char[])((char[])fields.get("value", null)).clone();
+        in.readObject();
+    }
+    private void writeObject(
+        java.io.ObjectOutputStream out
+    ) throws java.io.IOException {
+        out.defaultWriteObject();
+        out.writeObject(this);
+    }
+}

test/java/io/Serializable/6559775/Test6559775.sh

+#!/bin/sh
+
+if [ "${TESTSRC}" = "" ]
+then TESTSRC=.
+fi
+
+if [ "${TESTJAVA}" = "" ]
+then
+  PARENT=`dirname \`which java\``
+  TESTJAVA=`dirname ${PARENT}`
+  echo "TESTJAVA not set, selecting " ${TESTJAVA}
+  echo "If this is incorrect, try setting the variable manually."
+fi
+
+if [ "${TESTCLASSES}" = "" ]
+then
+  echo "TESTCLASSES not set.  Test cannot execute.  Failed."
+  exit 1
+fi
+
+BIT_FLAG=""
+
+# set platform-dependent variables
+OS=`uname -s`
+case "$OS" in
+  SunOS | Linux )
+    NULL=/dev/null
+    PS=":"
+    FS="/"
+    ## for solaris, linux it's HOME
+    FILE_LOCATION=$HOME
+    if [ -f ${FILE_LOCATION}${FS}JDK64BIT -a ${OS} = "SunOS" ]
+    then
+        BIT_FLAG=`cat ${FILE_LOCATION}${FS}JDK64BIT | grep -v '^#'`
+    fi
+    ;;
+  Windows_* )
+    NULL=NUL
+    PS=";"
+    FS="\\"
+    ;;
+  * )
+    echo "Unrecognized system!"
+    exit 1;
+    ;;
+esac
+
+JEMMYPATH=${CPAPPEND}
+CLASSPATH=.${PS}${TESTCLASSES}${PS}${JEMMYPATH} ; export CLASSPATH
+
+THIS_DIR=`pwd`
+
+${TESTJAVA}${FS}bin${FS}java ${BIT_FLAG} -version
+
+cp ${TESTSRC}${FS}*.java .
+chmod 777 *.java
+
+${TESTJAVA}${FS}bin${FS}javac SerialRace.java
+
+${TESTJAVA}${FS}bin${FS}java ${BIT_FLAG} SerialRace > test.out 2>&1
+
+cat test.out
+
+STATUS=0
+
+egrep "java.io.NotActiveException|not in readObject invocation or fields already read|^Victim" test.out
+
+if [ $? = 0 ]
+then
+    STATUS=1
+else
+    grep "java.lang.NullPointerException" test.out
+
+    if [ $? != 0 ]; then
+        STATUS=1
+    fi
+fi
+
+exit $STATUS