commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Herbert (Jira)" <j...@apache.org>
Subject [jira] [Commented] (RNG-120) Fix security issues in serialization code for Random instances
Date Tue, 01 Oct 2019 11:52:00 GMT

    [ https://issues.apache.org/jira/browse/RNG-120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16941766#comment-16941766
] 

Alex Herbert commented on RNG-120:
----------------------------------

{quote}Is there any correct use for methods writeObject and readObject?
{quote}
Yes. You can use them if you are sure that the object that will be deserialised is OK. 

The deserialization lifecycle first reads the name of the class from the input stream. It
then has to find the class and load it, running any object specific deserialization code for
the class. It then passes the class back to the caller. The caller can then determine if this
is a class they want. But by then the malicious code (if any) will have already been run.

If the binary data handed to you is from an unverified source it can contain anything. So
running readObject() on the binary data may load unwanted code during deserialization. 

So the solution is [Look-ahead deserialisation|https://www.ibm.com/developerworks/library/se-lookahead/index.html] that
first checks the class to be loaded before loading it. This uses a class that extends ObjectInputStream
and overrides:
{code:java}
protected Class<?> resolveClass(ObjectStreamClass desc);
{code}
This method is called during the deserialization lifecycle to determine how to resolve a named
class that is in the object input stream. The name of the class is in the description parameter.

So in this case you can check the class name before any loading of class files occurs. The
PR contains code to just check that the class to be deserialized is {{java.util.Random}}.
I added a test case that attempts to pass in a malicious serialized object:
{code:java}
/**
 * A class that is Serializable.
 * It contains member fields so there is something to serialize.
 */
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 {
        throw new AssertionError("This should not be run during the test");
    }
}
{code}
Without the patch to {{JDKRandom}} the test fails as the custom deserialisation code is run
and the AssertionError is thrown.

With the patch for look-ahead deserialization all is OK.

The examples in the SonarCloud warning suggest three solutions that all extend ObjectInputStream
and provide various configuration options on what to deserialize:

[contrast-rO0's SafeObjectInputStream|https://github.com/Contrast-Security-OSS/contrast-rO0]
[ikkisoft's SerialKiller|https://github.com/ikkisoft/SerialKiller]
Commons IO ValidatingObjectInputStream

I picked to adapt Commons IO ValidatingObjectInputStream with the simple solution to only
allow deserialising Random:
{code:java}
@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 Random.
    if (!Random.class.getName().equals(osc.getName())) {
        throw new IllegalStateException("Stream does not contain java.util.Random: " + osc.getName());
    }
    return super.resolveClass(osc);
}
{code}

The other use of readObject can be replaced by raw byte[] write and read. So this is not an
issue.


> Fix security issues in serialization code for Random instances
> --------------------------------------------------------------
>
>                 Key: RNG-120
>                 URL: https://issues.apache.org/jira/browse/RNG-120
>             Project: Commons RNG
>          Issue Type: Improvement
>          Components: core, simple
>    Affects Versions: 1.3
>            Reporter: Alex Herbert
>            Assignee: Alex Herbert
>            Priority: Minor
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> SonarCloud has highlighted security issues in the use of serialization to save and restore
the state of java.util.Random instances.
> When reading objects using ObjectInputStream.readObject() the class is first identified
and the private readObject() method of the class type is executed (if it is present). If the
class is a malicious class then potentially malicious code can be executed.
> h2. JDKRandom
> Uses serialisation to save the {{java.util.Random}} instance to the RandomProviderState.
> The code requires that {{java.util.Random}} is read using ObjectInputStream.readObject().
To ensure the code only allows {{java.util.Random}} to be read the code can adapt the [ValidatingObjectInputStream|https://commons.apache.org/proper/commons-io/javadocs/api-2.6/org/apache/commons/io/serialization/ValidatingObjectInputStream.html]
idea from Commons IO to prevent malicious code execution.
> h2. JDKRandomBridge
> This writes and reads a byte[] using the writeObject and readObject methods of ObjectOutput/InputStream.
To avoid use of readObject() the code can be refactored to write the byte[] using the write(byte[])
method of ObjectOutputStream and the readFully(byte[]) method of ObjectInputStream.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Mime
View raw message