Java Object Serialization and Untrusted Code Execution

This past week we had a mini-fire-drill at work, in response to a CERT vulnerability note titled “Apache Commons Collections Java library insecurely deserializes data.” We dutifully updated our commons-collections dependencies, but something didn't quite smell right to me, so I did some digging. And after a few hours of research and experimentation, I realized that commons-collection was only a convenient tool to exploit the real weakness.

Yes, if you use commons-collection, you should update it to the latest version. Today. Now.

But more important, you need to review your codebase for any code that might deserialize untrusted data. This includes references to ObjectInputStream, but it also includes any RMI servers that you expose to the outside world. And don't stop at your code, but also look at your dependencies to make sure that they don't deserialize untrusted data. Once you've done that, read on for an explanation.

Actually, there are two things you should read first. The first is the slide deck from “Marshalling Pickles,” which describes how this exploit works (for multiple languages, not just Java). The second is my article on Java serialization, especially the section on readObject().

Now that you've read all that, I'm going to summarize the exploit:

• The Java serialization protocol is public and easy to read. Anybody can craft an arbitrary object graph and write it to a file.
• To deserialize an object, that object's classfile must be on the classpath. This means that an attacker can't simply write a malicious class, compile it, and expect your system to execute it (unless they can also get the bytecode into your system, in which case you've already lost). This, however, is a false sense of security.
• Apache Commons Collections provides a number of functor objects that perform invocation of Java methods using reflection. This means that, if Commons-Collections is on your classpath, the attacker can execute arbitrary code, by providing it as serialized data. And because it's so useful, Commons-Collections is on many classpaths, including many app-servers.
• Commons-Collection also provides a LazyMap class that invokes user-defined functors via its get(). This means that, if you can get the target JVM to accept and access such a map, you can execute arbitrary code.
• The JRE internal class sun.reflect.annotation.AnnotationInvocationHandler has a member variable as a Map (rather than, say, a HashMap). Moreover, it has a readObject() method that accesses this map as part of restoring the object's state in its readObject() method.

Putting all these pieces together, the exploit is a serialized AnnotationInvocationHandler that contains a LazyMap that invokes functors that execute arbitrary code. When this graph is deserialized, the functors are executed, and you're pwned.

There are a few points to take away from this description. The first, of course, is that uncontrolled deserialization is a Bad Idea. That doesn't mean that serialization is itself a bad idea; feel free to serialize and deserialize your internal data. Just be sure that you're not deserializing something that came from outside your application.

The second take-away is that code-as-data can be dangerous. As I said above, there's no way to put arbitrary code on your classpath, but the exploit doesn't rely on that: it relies on passing arbitrary data into your application. Clojure fans might not like the implications.

For that matter, pay attention to what goes onto your classpath! Apache Commons is an incredibly useful collection of utility classes; if you don't use them, it's likely that one of your dependencies does. Or you have a dependency that offers features that are just waiting to be exploited. My KDGCommons library, for example, has a DefaultMap; all it's missing is a reflection-based functor (and a much wider usage).

The final — and most important — take-away is to pay attention to the variables in your serialized classes. The attack vector here is not commons-collections, it's AnnotationInvocationHandler and its Map. If, instead, this member was defined as a concrete class such as HashMap, the exploit would not have worked: attempting to deserialize a LazyMap when a Hashmap is expected would cause an exception. I know, using a concrete class goes against everything that we're taught about choosing the least-specific type for a variable, but security takes precedence over purity (and you don't need to expose the concrete type via the object's API).

If you're not scared enough already, I'll leave you with this: in the src.zip for JDK 1.7.0_79, there are a little over 600 classes that implement Serializable. And there are a bunch of these that define a private Map. Any of those could be a vector for the same attack.

When Immutability Gets In The Way

Here's a recent hack that I perpetrated:

(defn- redact-headers-from-exception-info
  [ei]
  (if-not (get-in (ex-data ei) [:request :headers])
    ei)

  (let [orig-trace (.getStackTrace ei)]
    (doto (ex-info (.getMessage ei)
                   (update-in (ex-data ei) [:request :headers] dissoc "Authorization")
                   (.getCause ei))
          (.setStackTrace orig-trace))))

Clojure provides the ex-info function, which creates a RuntimeException that has an attached map of data. This is quite useful, as you can provide detailed information at the exception site without creating your own exception subclass.

However, it can provide too much information. In this case, the exception was being thrown from a web-service call, and, to assist debugging, contained the entire request. Including the Authorization header, which contains client credentials. Not something that you want to write to a logfile.

And here's how immutability got in our way. Clojure maps are immutable: you can't update them in-place. And the ExceptionInfo object doesn't provide any way to replace the map. So we were faced with a choice: either pass the exception to the logger and have sensitive data end up in the log; or extract the map from the exception, redact the sensitive data, but lose the stack trace. I don't know about you, but I consider stack traces to be one of the most important clues to debugging errors; we make web-service calls all over the place, and need to know which one is failing.

Fortunately, java.lang.Throwable is not immutable: it provides methods to get and set the stack trace. And thus the hack: I extract the stack trace and data map from the existing exception, remove the fields that I don't want to log, and create a new exception from those components. Then I explicitly replace the stack trace in the new exception. It's ugly, but it works: we log the information we need to debug the problem, but not the sensitive information that was present in the original info map.

This post is not intended as a slam on Clojure, or on immutability in general. Instead, it's a reminder that a rigid adherence to immutability can have unintended consequences.