Java: Query for detecting unsafe deserialization with Spring exporters

[artem-smotrakov]

Spring Framework provides an abstract base class RemoteInvocationSerializingExporter for defining remote service exporters. A Spring exporter, which is based on this class, deserializes incoming data using ObjectInputStream. Deserializing untrusted data (CWE-502) is easily exploitable and in many cases allows an attacker to execute arbitrary code. Spring Framework also provides two classes that extend RemoteInvocationSerializingExporter:

• HttpInvokerServiceExporter
• SimpleHttpInvokerServiceExporter

CVE-2016-1000027 has been assigned to this issue in Spring Framework. There is no fix for that.

I'd like to propose a new experimental query that looks for unsafe deserialization with Spring exporters:

• The query looks for Spring configurations that define beans based on the vulnerable exporters.
• Added a qhelp file and an example of vulnerable code.
• Added tests.

Many projects have received alerts about CVE-2016-1000027 from security scanners. Since Spring Framework didn't address the issue, they can't just update Spring Framework. Instead, they have to understand the issue and check their code. This query can make their life easier and help them check the code. Moreover, the query can detect unsafe exporters even without scanners that look for known vulnerabilities.

Here are some true positives:

• Unsafe deserialization in HELIX-GR/lab
• Unsafe deserialization in YAMJ/yamj-v3

[intrigus-lgtm]

Those two lines should be equivalent to just

  isRemoteInvocationSerializingExporter(method.getReturnType().(RefType).getASupertype*())

The * says apply getASupertype() zero or more times.
And if you apply it zero times, you effectively get method.getReturnType() just as above.
It also looks like this is the only usage of isRemoteInvocationSerializingExporter, why not inline it?

[artem-smotrakov]

Thanks for the suggestion @intrigus-lgtm! You're right - I've removed that redundant check.

why not inline it?

I usually create predicates/classes/etc with some meaningful names even if they are used only once. I believe it makes code a bit easier to understand. I don't mind to inline it if there is a good reason for that (maybe performance?). Otherwise, I'd prefer to keep it as it is now.

[smowton]

How sure can we be that the exporter is exposed to untrusted data?

[smowton]

Suggested change

	Spring Framework provides an abstract base class <code>RemoteInvocationSerializingExporter</code>
	The Spring Framework provides an abstract base class <code>RemoteInvocationSerializingExporter</code>

[smowton]

Suggested change

	Spring Framework also provides two classes that extend <code>RemoteInvocationSerializingExporter</code>:
	The Spring Framework also provides two classes that extend <code>RemoteInvocationSerializingExporter</code>:

[smowton]

Suggested change

	CVE-2016-1000027 has been assigned to this issue in Spring Framework. There is no fix for that.
	CVE-2016-1000027 has been assigned to this issue in Spring Framework. It is regarded as a design limitation, and can be mitigated but not fixed outright.

[smowton]

Suggested change

	In general, avoid deserialization of untrusted data.
	In general, avoid using Java's built-in deserialization methods on untrusted data.

(to clarify vs. unexploitable formats)

[smowton]

Suggested change

	"Unasafe deserialization in a remote service exporter in '" + method.getName() + "' method"
	"Unsafe deserialization in a remote service exporter in '" + method.getName() + "' method"

[artem-smotrakov]

How sure can we be that the exporter is exposed to untrusted data?

The Spring Framework creates an HTTP endpoint if the exporter is defined as a Spring bean. Here is an example:

    @Bean(name = "/booking")
    HttpInvokerServiceExporter accountService() {
        HttpInvokerServiceExporter exporter = new HttpInvokerServiceExporter();
        exporter.setService(new CabBookingServiceImpl());
        exporter.setServiceInterface(CabBookingService.class);
        return exporter;
    }

A remote attacker can exploit this endpoint by sending an HTTP request with a malicious deserialization gadget. I tested it and wrote a simple PoC. Therefore, I think we can be quite sure that the exporter is exposed to untrusted data. Of course, we can not be 100% sure because there may be authentication/authorization checks, firewalls, filters, etc.

Thanks for the review @smowton !

[smowton]

Note you can use one qhelp file and include it from the other, like this: https://github.com/github/codeql/blob/aeb13146d2c786c7d8e69a52bc095fd78e2a1b29/python/ql/src/Lexical/CommentedOutCode.qhelp

[smowton]

(using naming scheme ".inc.qhelp" is required for qhelp fragments (qhelp documents that don't stand alone) is required to not trip up CI)

[artem-smotrakov]

Note you can use one qhelp file and include it from the other

Nice! I didn't know about it. I've updated the qhelp files -- thanks for the suggestion @smowton !

[pwntester]

We need to account for @Configuration meta-annotations such as @SpringBootApplication or maybe its enough to consider that @Bean annotations are going to be placed in the right classes and remove the check for @Configuration altogether

[artem-smotrakov]

That's a very good point @pwntester ! I've updated the query to take into account meta-annotations that has @Configuration.

[pwntester]

@artem-smotrakov just found a vulnerability on an application using the RMIServiceExporter and then remembered this PR. I think it would be nice to extend it so that it includes other vulnerable exporters such as the RmiBasedExporter or the HessianExporter

[artem-smotrakov]

Good point! Here you go #6142 Thanks!