Hi Alvaro,
Alvaro wrote:
> Lets say you have a Contact Interface and a ContactImpl class.
>
> Contact c = (Contact) xstream.fromXML(xml);
>
> and xml is:
>
> <dynamic-proxy>
> <interface>org.company.model.Contact</interface>
> <handler class="java.beans.EventHandler">
> <target class="java.lang.ProcessBuilder">
> <command>
> <string>calc.exe</string>
> </command>
> </target>
> <action>start</action>
> </handler>
> </dynamic-proxy>
>
> Then as soon as the code calls any method on the Contact instace, the
> payload gets executed (eg: contact.getFirstName() )
Ah, that one. Yes, I have seen it before, but I do not blame the dynamic
proxy by default. It's even questionable if this is a special problem with
XStream. While this *is* a security hole, it is made possible by
EventHandler's create method:
=============== %< ==============
public class Main {
public static void main(String[] args) {
Set<Comparable> set = new TreeSet<Comparable>();
set.add("foo");
set.add(EventHandler.create(Comparable.class, new
ProcessBuilder("gvim", "/home/joehni/tmp/ps-aux.txt"), "start"));
}
}
=============== %< ==============
While this is demonstrates the problem in pure Java, you will have the same
security hole in any *scripted* environment that can call the JVM. Like
XStream you can demonstrate this also with the Java ScriptEngine or quite
any Dependency Injection container that is configured from the outside. It
is a bit like SQL injection - if you accept input you have to ensure that it
cannot be abused.
However, it is unfortunate that the JRE itself already provides such an easy
way to redirect a method call to an arbitrary method of a target, but it is
explicitly documented:
<http://docs.oracle.com/javase/7/docs/api/java/beans/EventHandler.html#create(java.lang.Class,
java.lang.Object, java.lang.String)>
There's no general solution for every use case though, but it is quite easy
to write a converter for an EventHandler or a ProcessBuilder that will throw
an Exception at deserialization time. But it still depends on the use case.
XStream cannot say if the deserialization of such an object is wrong in
general or if it is a valid element of the object graph. In the latter case
an implementation of a EventHandlerConverter might only throw an exception
if the action name does not match a method of the proxied interface.
Nevertheless, there is a reason why this functionality made it into the JRE
and an object graph with GUI objects might contain such an element with much
higher probability than expected.
Therefore I was never sure if XStream should register such a converter by
default, but it is really frightening to have such an easy possibility for
abuse.
Actually you have 3 parts in the plain JDK itself that makes this possible:
- the dynamic proxy that can be used to insert a matching replacement
- the EventHandler that translates every call into an arbitrary action
- the ProcessBuilder that supports the call of arbitrary applications
And XStream provides with the reflection-based converters the 4th part by
deserializing arbitrary objects.
> SpringOXM has a wrapper for XStream
> (org.springframework.oxm.xstream.XStreamMarshaller) that enables the
> unmarshalling of objects from XML format.
> This SpringOXM module is used by SpringMVC when building RESTFul APIs.
> My concern is that an attacker can sends a malicious crafted XML that
> results in remote code execution in the case that the server is expecting
> an object that implements an interface.
>
> I would love to be able to disable the DynamicProxyconverter in simple
> fashion and expose that method to the SpringgOXM wrapper so it can be
> safely used for RESTFul APIs.
As said, I have no knowledge about the configuration possibility of XStream
within SpringOXM.
However, in case of untrusted input, you're only on the safe side, if you
prevent XStream from using the reflection converters although it is enough
to break the action in your example by suppressing one of the 4 involved
parts. But as long as the reflection-based converters work, an attacker mind
find other ways.
This implies that you use dedicated converters for all of your objects (you
might use as a more generic alternative the JavaBeanConverter) and register
a converter that claims to convert any type with low priority, but that will
throw an exception in unmarshal. Since the reflection-based converters have
even lower priority, they're never called.
Regards,
Jörg
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
