OpenJDK / jdk6 / jdk6 / jdk
changeset 448:57681551c11e
6559775: Race allows defaultReadObject to be invoked instead of readFields during deserialization
Reviewed-by: hawtin
author | skoppar |
---|---|
date | Tue, 28 Sep 2010 03:58:37 -0700 |
parents | bb1c74cae929 |
children | 6b2459c08142 |
files | make/java/java/FILES_java.gmk src/share/classes/java/io/ObjectInputStream.java src/share/classes/java/io/ObjectOutputStream.java src/share/classes/java/io/SerialCallbackContext.java test/java/io/Serializable/6559775/README test/java/io/Serializable/6559775/SerialRace.java test/java/io/Serializable/6559775/SerialVictim.java test/java/io/Serializable/6559775/Test6559775.sh |
diffstat | 8 files changed, 311 insertions(+), 63 deletions(-) [+] |
line wrap: on
line diff
--- a/make/java/java/FILES_java.gmk Tue Sep 28 18:59:04 2010 +0400 +++ b/make/java/java/FILES_java.gmk Tue Sep 28 03:58:37 2010 -0700 @@ -384,6 +384,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/src/share/classes/java/io/ObjectInputStream.java Tue Sep 28 18:59:04 2010 +0400 +++ b/src/share/classes/java/io/ObjectInputStream.java Tue Sep 28 03:58:37 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 @@ -264,7 +264,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. @@ -1797,7 +1797,7 @@ private void readExternalData(Externalizable obj, ObjectStreamClass desc) throws IOException { - CallbackContext oldContext = curContext; + SerialCallbackContext oldContext = curContext; curContext = null; try { boolean blocked = desc.hasBlockExternalData(); @@ -1856,10 +1856,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); @@ -3504,42 +3504,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/src/share/classes/java/io/ObjectOutputStream.java Tue Sep 28 18:59:04 2010 +0400 +++ b/src/share/classes/java/io/ObjectOutputStream.java Tue Sep 28 03:58:37 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; - + SerialCallbackContext oldContext = curContext; + if (extendedDebugInfo) { debugInfoStack.push("writeExternal data"); } 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/src/share/classes/java/io/SerialCallbackContext.java Tue Sep 28 03:58:37 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/test/java/io/Serializable/6559775/README Tue Sep 28 03:58:37 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/test/java/io/Serializable/6559775/SerialRace.java Tue Sep 28 03:58:37 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/test/java/io/Serializable/6559775/SerialVictim.java Tue Sep 28 03:58:37 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/test/java/io/Serializable/6559775/Test6559775.sh Tue Sep 28 03:58:37 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