Java: Unsafe RMI deserialization

java/ql/src/experimental/Security/CWE/CWE-502/RmiRemoteObjectWithFilter.java

@@ -0,0 +1,9 @@
+public void bindRemoteObject(Registry registry, int port) throws Exception {
+    ObjectInputFilter filter = info -> {
+        if (info.serialClass().getCanonicalName().startsWith("com.safe.package.")) {
+            return ObjectInputFilter.Status.ALLOWED;
+        }
+        return ObjectInputFilter.Status.REJECTED;
+    };
+    registry.bind("safer", UnicastRemoteObject.exportObject(new RemoteObjectImpl(), port, filter));
+}

java/ql/src/experimental/Security/CWE/CWE-502/RmiSafeRemoteObject.java

@@ -0,0 +1,14 @@
+public class Server {
+    public void bindRemoteObject(Registry registry) throws Exception {
+        registry.bind("safe", new RemoteObjectImpl());
+    }
+}
+
+interface RemoteObject extends Remote {
+    void calculate(int a, double b) throws RemoteException;
+    void save(String s) throws RemoteException;
+}
+
+class RemoteObjectImpl implements RemoteObject {
+    // ...
+}

java/ql/src/experimental/Security/CWE/CWE-502/RmiUnsafeRemoteObject.java

@@ -0,0 +1,13 @@
+public class Server {
+    public void bindRemoteObject(Registry registry) throws Exception {
+        registry.bind("unsafe", new RemoteObjectImpl());
+    }
+}
+
+interface RemoteObject extends Remote {
+    void action(Object obj) throws RemoteException;
+}
+
+class RemoteObjectImpl implements RemoteObject {
+    // ...
+}

java/ql/src/experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.qhelp

@@ -0,0 +1,81 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Java RMI uses the default Java serialization mechanism (in other words, <code>ObjectInputStream</code>)
+to pass parameters in remote method invocations. This mechanism is known to be unsafe when deserializing 
+untrusted data. If a registered remote object has a method that accepts a complex object,
+an attacker can take advantage of the unsafe deserialization mechanism.
+In the worst case, it results in remote code execution.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Use only strings and primitive types for parameters of remotely invokable methods.
+</p>
+<p>
+Set a filter for incoming serialized data by wrapping remote objects using either <code>UnicastRemoteObject.exportObject(Remote, int, ObjectInputFilter)</code>
+or <code>UnicastRemoteObject.exportObject(Remote, int, RMIClientSocketFactory, RMIServerSocketFactory, ObjectInputFilter)</code> methods.
+Those methods accept an <code>ObjectInputFilter</code> that decides which classes are allowed for deserialization.
+The filter should allow deserializing only safe classes.
+</p>
+<p>
+It is also possible to set a process-wide deserialization filter.
+The filter can be set by with <code>ObjectInputFilter.Config.setSerialFilter(ObjectInputFilter)</code> method,
+or by setting system or security property <code>jdk.serialFilter</code>.
+Make sure that you use the latest Java versions that include JEP 290.
+Please note that the query is not sensitive to this mitigation.
+</p>
+<p>
+If switching to the latest Java versions is not possible,
+consider using other implementations of remote procedure calls. For example, HTTP API with JSON.
+Make sure that the underlying deserialization mechanism is properly configured
+so that deserialization attacks are not possible.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following code registers a remote object
+with a vulnerable method that accepts a complex object:
+</p>
+<sample src="RmiUnsafeRemoteObject.java" />
+
+<p>
+The next example registers a safe remote object
+whose methods use only primitive types and strings:
+</p>
+<sample src="RmiSafeRemoteObject.java" />
+
+<p>
+The next example shows how to set a deserilization filter for a remote object:
+</p>
+<sample src="RmiRemoteObjectWithFilter.java" />
+
+</example>
+
+<references>
+<li>
+Oracle:
+<a href="https://www.oracle.com/java/technologies/javase/remote-method-invocation-home.html">Remote Method Invocation (RMI)</a>.
+</li>
+<li>
+ITNEXT:
+<a href="https://itnext.io/java-rmi-for-pentesters-part-two-reconnaissance-attack-against-non-jmx-registries-187a6561314d">Java RMI for pentesters part two - reconnaissance &amp; attack against non-JMX registries</a>.
+</li>
+<li>
+MOGWAI LABS:
+<a href="https://mogwailabs.de/en/blog/2019/03/attacking-java-rmi-services-after-jep-290">Attacking Java RMI services after JEP 290</a>
+</li>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Deserialization_of_untrusted_data">Deserialization of untrusted data</a>.
+</li>
+<li>
+OpenJDK:
+<a href="https://openjdk.java.net/jeps/290">JEP 290: Filter Incoming Serialization Data</a>
+</li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.ql

@@ -0,0 +1,78 @@
+/**
+ * @name Unsafe deserialization in a remotely callable method.
+ * @description If a registered remote object has a method that accepts a complex object,
+ *              an attacker can take advantage of the unsafe deserialization mechanism
+ *              which is used to pass parameters in RMI.
+ *              In the worst case, it results in remote code execution.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/unsafe-deserialization-rmi
+ * @tags security
+ *       external/cwe/cwe-502
+ */
+
+import java
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.frameworks.Rmi
+import DataFlow::PathGraph
+
+/**
+ * A method that binds a name to a remote object.
+ */
+private class BindMethod extends Method {
+  BindMethod() {
+    (
+      getDeclaringType().hasQualifiedName("java.rmi", "Naming") or
+      getDeclaringType().hasQualifiedName("java.rmi.registry", "Registry")
+    ) and
+    hasName(["bind", "rebind"])
+  }
+}
+
+/**
+ * Holds if `type` has an vulnerable remote method.
+ */
+private predicate hasVulnerableMethod(RefType type) {
+  exists(RemoteCallableMethod m, Type parameterType |
+    m.getDeclaringType() = type and parameterType = m.getAParamType()
+  |
+    not parameterType instanceof PrimitiveType and
+    not parameterType instanceof TypeString and
+    not parameterType.(RefType).hasQualifiedName("java.io", "ObjectInputStream")
+  )
+}
+
+/**
+ * A taint-tracking configuration for unsafe remote objects
+ * that are vulnerable to deserialization attacks.
+ */
+private class BindingUnsafeRemoteObjectConfig extends TaintTracking::Configuration {
+  BindingUnsafeRemoteObjectConfig() { this = "BindingUnsafeRemoteObjectConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(ConstructorCall cc | cc = source.asExpr() |
+      hasVulnerableMethod(cc.getConstructedType().getASupertype*())
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma | ma.getArgument(1) = sink.asExpr() |
+      ma.getMethod() instanceof BindMethod
+    )
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    exists(MethodAccess ma, Method m | m = ma.getMethod() |
+      m.getDeclaringType().hasQualifiedName("java.rmi.server", "UnicastRemoteObject") and
+      m.hasName("exportObject") and
+      not m.getParameterType([2, 4]).(RefType).hasQualifiedName("java.io", "ObjectInputFilter") and
+      ma.getArgument(0) = fromNode.asExpr() and
+      ma = toNode.asExpr()
+    )
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, BindingUnsafeRemoteObjectConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe deserialization in a remote object."

java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.expected

@@ -0,0 +1,15 @@
+edges
+| UnsafeDeserializationRmi.java:17:68:17:95 | new UnsafeRemoteObjectImpl(...) : UnsafeRemoteObjectImpl | UnsafeDeserializationRmi.java:17:35:17:96 | exportObject(...) |
+nodes
+| UnsafeDeserializationRmi.java:15:33:15:60 | new UnsafeRemoteObjectImpl(...) | semmle.label | new UnsafeRemoteObjectImpl(...) |
+| UnsafeDeserializationRmi.java:16:35:16:62 | new UnsafeRemoteObjectImpl(...) | semmle.label | new UnsafeRemoteObjectImpl(...) |
+| UnsafeDeserializationRmi.java:17:35:17:96 | exportObject(...) | semmle.label | exportObject(...) |
+| UnsafeDeserializationRmi.java:17:68:17:95 | new UnsafeRemoteObjectImpl(...) : UnsafeRemoteObjectImpl | semmle.label | new UnsafeRemoteObjectImpl(...) : UnsafeRemoteObjectImpl |
+| UnsafeDeserializationRmi.java:29:31:29:58 | new UnsafeRemoteObjectImpl(...) | semmle.label | new UnsafeRemoteObjectImpl(...) |
+| UnsafeDeserializationRmi.java:30:33:30:60 | new UnsafeRemoteObjectImpl(...) | semmle.label | new UnsafeRemoteObjectImpl(...) |
+#select
+| UnsafeDeserializationRmi.java:15:33:15:60 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:15:33:15:60 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:15:33:15:60 | new UnsafeRemoteObjectImpl(...) | Unsafe deserialization in a remote object. |
+| UnsafeDeserializationRmi.java:16:35:16:62 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:16:35:16:62 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:16:35:16:62 | new UnsafeRemoteObjectImpl(...) | Unsafe deserialization in a remote object. |
+| UnsafeDeserializationRmi.java:17:35:17:96 | exportObject(...) | UnsafeDeserializationRmi.java:17:68:17:95 | new UnsafeRemoteObjectImpl(...) : UnsafeRemoteObjectImpl | UnsafeDeserializationRmi.java:17:35:17:96 | exportObject(...) | Unsafe deserialization in a remote object. |
+| UnsafeDeserializationRmi.java:29:31:29:58 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:29:31:29:58 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:29:31:29:58 | new UnsafeRemoteObjectImpl(...) | Unsafe deserialization in a remote object. |
+| UnsafeDeserializationRmi.java:30:33:30:60 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:30:33:30:60 | new UnsafeRemoteObjectImpl(...) | UnsafeDeserializationRmi.java:30:33:30:60 | new UnsafeRemoteObjectImpl(...) | Unsafe deserialization in a remote object. |

java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.java

@@ -0,0 +1,73 @@
+import java.io.ObjectInputFilter;
+import java.io.ObjectInputStream;
+import java.rmi.Naming;
+import java.rmi.Remote;
+import java.rmi.RemoteException;
+import java.rmi.registry.LocateRegistry;
+import java.rmi.registry.Registry;
+import java.rmi.server.UnicastRemoteObject;
+
+public class UnsafeDeserializationRmi {
+
+    // BAD (bind a remote object that has a vulnerable method)
+    public static void testRegistryBindWithObjectParameter() throws Exception {
+        Registry registry = LocateRegistry.createRegistry(1099);
+        registry.bind("unsafe", new UnsafeRemoteObjectImpl());
+        registry.rebind("unsafe", new UnsafeRemoteObjectImpl());
+        registry.rebind("unsafe", UnicastRemoteObject.exportObject(new UnsafeRemoteObjectImpl()));
+    }
+
+    // GOOD (bind a remote object that has methods that takes safe parameters)
+    public static void testRegistryBindWithIntParameter() throws Exception {
+        Registry registry = LocateRegistry.createRegistry(1099);
+        registry.bind("safe", new SafeRemoteObjectImpl());
+        registry.rebind("safe", new SafeRemoteObjectImpl());
+    }
+
+    // BAD (bind a remote object that has a vulnerable method)
+    public static void testNamingBindWithObjectParameter() throws Exception {
+        Naming.bind("unsafe", new UnsafeRemoteObjectImpl());
+        Naming.rebind("unsafe", new UnsafeRemoteObjectImpl());
+    }
+
+    // GOOD (bind a remote object that has methods that takes safe parameters)
+    public static void testNamingBindWithIntParameter() throws Exception {
+        Naming.bind("safe", new SafeRemoteObjectImpl());
+        Naming.rebind("safe", new SafeRemoteObjectImpl());
+    }
+
+    // GOOD (bind a remote object with a deserialization filter)
+    public static void testRegistryBindWithDeserializationFilter() throws Exception {
+        Registry registry = LocateRegistry.createRegistry(1099);
+        ObjectInputFilter filter = info -> {
+            if (info.serialClass().getCanonicalName().startsWith("com.safe.package.")) {
+                return ObjectInputFilter.Status.ALLOWED;
+            }
+            return ObjectInputFilter.Status.REJECTED;
+        };
+        registry.rebind("safe", UnicastRemoteObject.exportObject(new UnsafeRemoteObjectImpl(), 12345, filter));
+    }
+}
+
+interface UnsafeRemoteObject extends Remote {
+    void take(Object obj) throws RemoteException;
+}
+
+class UnsafeRemoteObjectImpl implements UnsafeRemoteObject {
+    public void take(Object obj) throws RemoteException {}
+}
+
+interface SafeRemoteObject extends Remote {
+    void take(int n) throws RemoteException;
+    void take(double n) throws RemoteException;
+    void take(String s) throws RemoteException;
+    void take(ObjectInputStream ois) throws RemoteException;
+}
+
+class SafeRemoteObjectImpl implements SafeRemoteObject {
+    public void take(int n) throws RemoteException {}
+    public void take(double n) throws RemoteException {}
+    public void take(String s) throws RemoteException {}
+    public void take(ObjectInputStream ois) throws RemoteException {}
+    public void safeMethod(Object object) {} // this method is not declared in SafeRemoteObject
+}

java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.ql