Java: Unsafe RMI deserialization

[artem-smotrakov]

RMI uses the default Java serialization mechanism (in other words, ObjectInputStream) to pass parameters in remote method invocations. If a remote method accepts complex parameters, then a remote attacker can send a malicious serialized object as one of the parameters. The malicious object gets deserialized without any check on the incoming data. In the worst case, it may let the attacker run arbitrary code remotely.

I'd like to propose a new experimental query that looks for deserialization vulnerabilities in remote objects registered in am RMI registry.

Here are several issue found by the query:

• CVE-2016-2170 in Apache OFBiz
• RMI deserialization vulnerability in PeerUnit/mock-dht
• RMI deserialization vulnerability in TubeDB
• RMI deserialization vulnerability in Weka
• Alerts on LGTM

[Marcono1234]

If I understand the documentation of Remote correctly, only methods declared in an interface which directly extends Remote are remote methods.
Therefore the following would probably not be vulnerable (though I am not sure):

interface RemoteInterface extends Remote {
  void doSomething(int);
}

class RemoteClass implements RemoteInterface {
  @Override
  public void doSomething(int) {}

  // Not vulnerable
  public void doSomething(Object o) {}
}

(So only methods declared on an Interface which has Remote as direct supertype might be vulnerable)

[artem-smotrakov]

Good catch! I'll update the query. Thanks!

[Marcono1234]

Could use more specific type here:

Suggested change

	private class ObjectInputStream extends RefType {
	private class ObjectInputStream extends Class {

[artem-smotrakov]

Sure, I can use Class here. Do you know whether it would make the query faster or not?

[Marcono1234]

Using the most specific types is one recommendation of the CodeQL documentation.
Though I am not sure if there is actually a notable performance improvement, or how big the improvement will be, might be best to start a discussion if you want to get more detailed information from the maintainers.

[artem-smotrakov]

@Marcono1234 @pwntester Thanks for the review! I'll address your comments soon. I am currently checking if a couple of other sinks and sanitizers from UnicastRemoteObject and Activatable.

[artem-smotrakov]

I found out that it is possible to set a deserialization filter by calling UnicastRemoteObject.exportObject(). I've updated the query to cover it. The query now use a taint-tracking config. The predicate isAdditionalTaintStep() breaks the data flow if exportObject() called without a filter.

I've updated tests and docs.

The query is ready for review.

[smowton]

Simpler to check the parameter type than to walk the class hierarchy from the actual argument type

[artem-smotrakov]

Good point. I'll update the query.

[smowton]

Note that this query doesn't check for either of these mitigations

[artem-smotrakov]

Correct, it doesn't. I thought about covering these mitigations in the query. To prevent deserialization attacks here, ObjectInputFilter.Config.setSerialFilter() should be called before actual RMI calls happen. I am not sure whether this check can be implemented with CodeQL or not. It is even more complicated with the jdk.serialFilter property. The property has to be set before the classes for deserialization mechanism (ObjectInputStream and others) are loaded because the property is read only once when the mechanism is accessed for the first time.

I'd prefer to keep these mitigations here because they are still valid if used correctly.

Please let me know if you have an idea how to reliably check there mitigations in the query.

[smowton]

Just noting in the qhelp that these are valid mitigations but the query isn't sensitive to them is enough

[artem-smotrakov]

Thanks for the comments @smowton ! I've addressed them and also fixed a few errors in the qhelp. codeql generate query-help now works fine.

[smowton]

Guh, sorry for the delay, didn't notice you apply that change.

[artem-smotrakov]

Guh, sorry for the delay, didn't notice you apply that change.

No problem. Thanks for the review!