[jira] [Commented] (RNG-120) Fix security issues in serialization code for Random instances
[
https://issues.apache.org/jira/browse/RNG-120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16942109#comment-16942109
]
Alex Herbert commented on RNG-120:
--
{quote}But how can one be sure?
{quote}
I suppose you can be sure if you created the serialized form and know it could
not have been altered.
{quote}Doesn't your fixes imply that one should always perform validation
{quote}
It seems to be the case that serialization should not be done unchecked if you
really want to be safe. However added a checking guard in front of
ObjectInputStream is very easy. Perhaps this is why serialization is
out-of-favour nowadays.
{quote}that your SerializableTestObject would fail to be loaded if the data
were actually coming from an untrusted source (i.e. read from the network)?
{quote}
Yes.
The class loader has to be able to find the class. In this case the custom
class would not be found and ClassNotFoundException would be raised when
deserializing.
I did originally write the test using a String instead. However that has custom
deserialization and the test did not work (I found a note in the String.class
source code about custom serialization). So I created a dummy class. I've just
tried it with SerializableTestObject replaced with ArrayList and it still works
as expected.
The article states to the effect that the attacker has to have a guess at what
classes could be on the classpath of the server that will deserialise data. I
assume it wisely chooses to not propose good candidates for an attack.
The likelihood of this being a problem is low. It would require that:
# a system is reconstructing a RNG, specifically the JDKRandom which is not
recommended, using data sent to it in byte[] form and loaded via the
RestoreableUniformRandomProvider. That data may have been created with
malicious intent.
# a system is using JDKRandomBridge and restoring it from a serialized form
that is from an external source and may have been created with malicious intent.
However I do not see a problem with fixing the code. All the existing
functionality is maintained and serialization is made safer.
> 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)
[jira] [Commented] (RNG-120) Fix security issues in serialization code for Random instances
[
https://issues.apache.org/jira/browse/RNG-120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16942078#comment-16942078
]
Gilles Sadowski commented on RNG-120:
-
bq. Yes. You can use them if you are sure that the object that will be
deserialised is OK.
But how can one be sure?
Doesn't your fixes imply that one should always perform validation (even though
it does not seem obvious to me that "Commons RNG" is the right place to do so)?
Do I understand correctly this paragraph (from the article which you provided)
{noformat}
The attacker cannot simply send a serialized object of any class, because
the service will be unable to load the class.
{noformat}
that your {{SerializableTestObject}} would fail to be loaded if the data were
actually coming from an untrusted source (i.e. read from the network)?
> 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)
[jira] [Commented] (RNG-120) Fix security issues in serialization code for Random instances
[
https://issues.apache.org/jira/browse/RNG-120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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
[jira] [Commented] (RNG-120) Fix security issues in serialization code for Random instances
[
https://issues.apache.org/jira/browse/RNG-120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16941413#comment-16941413
]
Gilles Sadowski commented on RNG-120:
-
Is there any correct use for methods {{writeObject}} and {{readObject}}?
> 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)
