This is an automated email from the ASF dual-hosted git repository. aherbert pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/commons-rng.git
commit f8edf66d33ef6bf7af7ca52224d940b583e7ad65 Author: Alex Herbert <[email protected]> AuthorDate: Tue Oct 1 00:06:42 2019 +0100 RNG-120: Fix security issues in serialization code for Random instances. --- .../commons/rng/core/source32/JDKRandom.java | 37 ++++++++++++- .../commons/rng/core/source32/JDKRandomTest.java | 63 ++++++++++++++++++++++ .../apache/commons/rng/simple/JDKRandomBridge.java | 17 ++++-- .../commons/rng/simple/JDKRandomBridgeTest.java | 2 +- 4 files changed, 114 insertions(+), 5 deletions(-) diff --git a/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/JDKRandom.java b/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/JDKRandom.java index e9245b1..c152c0a 100644 --- a/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/JDKRandom.java +++ b/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/JDKRandom.java @@ -18,7 +18,9 @@ package org.apache.commons.rng.core.source32; import java.util.Random; import java.io.IOException; +import java.io.InputStream; import java.io.ObjectOutputStream; +import java.io.ObjectStreamClass; import java.io.ObjectInputStream; import java.io.ByteArrayOutputStream; @@ -47,6 +49,38 @@ public class JDKRandom extends IntProvider { private Random delegate; /** + * An <code>ObjectInputStream</code> that's restricted to deserialize + * only {@link java.util.Random} using look-ahead deserialization. + * + * <p>Adapted from o.a.c.io.serialization.ValidatingObjectInputStream.</p> + * + * @see <a href="http://www.ibm.com/developerworks/library/se-lookahead/";> + * IBM DeveloperWorks Article: Look-ahead Java deserialization</a> + */ + private static class ValidatingObjectInputStream extends ObjectInputStream { + /** + * @param in Input stream + * @throws IOException Signals that an I/O exception has occurred. + */ + ValidatingObjectInputStream(final InputStream in) throws IOException { + super(in); + } + + /** {@inheritDoc} */ + @Override + protected Class<?> resolveClass(final ObjectStreamClass osc) throws IOException, + ClassNotFoundException { + // For legacy reasons the Random class is serialized using only primitives + // even though modern implementations use AtomicLong. + // The only expected class is java.util.Random. + if (!Random.class.getName().equals(osc.getName())) { + throw new IllegalStateException("Stream does not contain java.util.Random: " + osc.getName()); + } + return super.resolveClass(osc); + } + } + + /** * Creates an instance with the given seed. * * @param seed Initial seed. @@ -98,9 +132,10 @@ public class JDKRandom extends IntProvider { // Second obtain the state final byte[][] c = splitStateInternal(s2[1], stateSize); + // Use look-ahead deserialization to validate the state byte[] contains java.util.Random. try { final ByteArrayInputStream bis = new ByteArrayInputStream(c[0]); - final ObjectInputStream ois = new ObjectInputStream(bis); + final ObjectInputStream ois = new ValidatingObjectInputStream(bis); delegate = (Random) ois.readObject(); } catch (ClassNotFoundException e) { diff --git a/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/JDKRandomTest.java b/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/JDKRandomTest.java index 0e86baf..39baabd 100644 --- a/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/JDKRandomTest.java +++ b/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/JDKRandomTest.java @@ -16,13 +16,47 @@ */ package org.apache.commons.rng.core.source32; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; +import java.io.Serializable; import java.util.Random; import org.apache.commons.rng.RandomProviderState; +import org.apache.commons.rng.core.RandomProviderDefaultState; +import org.apache.commons.rng.core.util.NumberFactory; import org.junit.Assert; import org.junit.Test; public class JDKRandomTest { + /** + * A class that is Serializable. + * It contains member fields so there is something to serialize and malicious + * deserialization code. + */ + static class SerializableTestObject implements Serializable { + private static final long serialVersionUID = 1L; + + private int state0; + private double state1; + private long state2; + private boolean stte3; + + /** + * This simulates doing something malicious when deserializing. + * + * @param input Input stream. + * @throws IOException if an error occurs. + * @throws ClassNotFoundException if an error occurs. + */ + private void readObject(ObjectInputStream input) + throws IOException, + ClassNotFoundException { + Assert.fail("This should not be run during the test"); + } + } + @Test public void testReferenceCode() { final long refSeed = -1357111213L; @@ -64,4 +98,33 @@ public class JDKRandomTest { Assert.assertEquals(r + " nextInt", rng1.nextInt(), rng2.nextInt()); } } + + /** + * Test the deserialization code identifies bad states that do not contain a Random instance. + * This test exercises the code that uses a custom deserialization ObjectInputStream. + * + * @throws IOException Signals that an I/O exception has occurred. + */ + @Test(expected = IllegalStateException.class) + public void testRestoreWithInvalidClass() throws IOException { + // Serialize something + final ByteArrayOutputStream bos = new ByteArrayOutputStream(); + final ObjectOutputStream oos = new ObjectOutputStream(bos); + oos.writeObject(new SerializableTestObject()); + oos.close(); + + // Compose the size with the state. + // This is what is expected by the JDKRandom class. + final byte[] state = bos.toByteArray(); + final int stateSize = state.length; + final byte[] sizeAndState = new byte[4 + stateSize]; + System.arraycopy(NumberFactory.makeByteArray(stateSize), 0, sizeAndState, 0, 4); + System.arraycopy(state, 0, sizeAndState, 4, stateSize); + + final RandomProviderDefaultState dummyState = new RandomProviderDefaultState(sizeAndState); + + final JDKRandom rng = new JDKRandom(13L); + // This should throw + rng.restoreState(dummyState); + } } diff --git a/commons-rng-simple/src/main/java/org/apache/commons/rng/simple/JDKRandomBridge.java b/commons-rng-simple/src/main/java/org/apache/commons/rng/simple/JDKRandomBridge.java index 78fc60b..02b0f29 100644 --- a/commons-rng-simple/src/main/java/org/apache/commons/rng/simple/JDKRandomBridge.java +++ b/commons-rng-simple/src/main/java/org/apache/commons/rng/simple/JDKRandomBridge.java @@ -106,8 +106,13 @@ public final class JDKRandomBridge extends Random { // Write non-transient fields. output.defaultWriteObject(); - // Save current state. - output.writeObject(((RandomProviderDefaultState) delegate.saveState()).getState()); + // Save current state and size. + // Avoid the use of ObjectOutputStream.writeObject(Object) to save the state. + // This allows deserialization to avoid security issues in using readObject(). + final byte[] state = ((RandomProviderDefaultState) delegate.saveState()).getState(); + final int size = state.length; + output.writeInt(size); + output.write(state); } } @@ -125,6 +130,12 @@ public final class JDKRandomBridge extends Random { // Recreate the "delegate" from serialized info. delegate = RandomSource.create(source); // And restore its state. - delegate.restoreState(new RandomProviderDefaultState((byte[]) input.readObject())); + // Avoid the use of input.readObject() to deserialize by manually reading the byte[]. + // Note: ObjectInputStream.readObject() will execute the readObject() method of the named + // class in the stream which may contain potentially malicious code. + final int size = input.readInt(); + final byte[] state = new byte[size]; + input.readFully(state); + delegate.restoreState(new RandomProviderDefaultState(state)); } } diff --git a/commons-rng-simple/src/test/java/org/apache/commons/rng/simple/JDKRandomBridgeTest.java b/commons-rng-simple/src/test/java/org/apache/commons/rng/simple/JDKRandomBridgeTest.java index 70fa450..6e735c7 100644 --- a/commons-rng-simple/src/test/java/org/apache/commons/rng/simple/JDKRandomBridgeTest.java +++ b/commons-rng-simple/src/test/java/org/apache/commons/rng/simple/JDKRandomBridgeTest.java @@ -63,7 +63,7 @@ public class JDKRandomBridgeTest { ObjectInputStream ois = new ObjectInputStream(bis); final Random serialRng = (Random) (ois.readObject()); - // Check that the serialized data recreated the orginal state. + // Check that the serialized data recreated the original state. checkSameSequence(rng, serialRng); }
