diff --git a/java/ql/src/experimental/Security/CWE/CWE-502/RmiRemoteObjectWithFilter.java b/java/ql/src/experimental/Security/CWE/CWE-502/RmiRemoteObjectWithFilter.java new file mode 100644 index 000000000000..6d4d2a5357f7 --- /dev/null +++ b/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)); +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-502/RmiSafeRemoteObject.java b/java/ql/src/experimental/Security/CWE/CWE-502/RmiSafeRemoteObject.java new file mode 100644 index 000000000000..0c8836522ca9 --- /dev/null +++ b/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 { + // ... +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-502/RmiUnsafeRemoteObject.java b/java/ql/src/experimental/Security/CWE/CWE-502/RmiUnsafeRemoteObject.java new file mode 100644 index 000000000000..96d48e9c9af4 --- /dev/null +++ b/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 { + // ... +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.qhelp b/java/ql/src/experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.qhelp new file mode 100644 index 000000000000..02ee7d7dab10 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.qhelp @@ -0,0 +1,81 @@ + + + + +

+Java RMI uses the default Java serialization mechanism (in other words, ObjectInputStream) +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. +

+ + + +

+Use only strings and primitive types for parameters of remotely invokable methods. +

+

+Set a filter for incoming serialized data by wrapping remote objects using either UnicastRemoteObject.exportObject(Remote, int, ObjectInputFilter) +or UnicastRemoteObject.exportObject(Remote, int, RMIClientSocketFactory, RMIServerSocketFactory, ObjectInputFilter) methods. +Those methods accept an ObjectInputFilter that decides which classes are allowed for deserialization. +The filter should allow deserializing only safe classes. +

+

+It is also possible to set a process-wide deserialization filter. +The filter can be set by with ObjectInputFilter.Config.setSerialFilter(ObjectInputFilter) method, +or by setting system or security property jdk.serialFilter. +Make sure that you use the latest Java versions that include JEP 290. +Please note that the query is not sensitive to this mitigation. +

+

+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. +

+ + + +

+The following code registers a remote object +with a vulnerable method that accepts a complex object: +

+ + +

+The next example registers a safe remote object +whose methods use only primitive types and strings: +

+ + +

+The next example shows how to set a deserilization filter for a remote object: +

+ + + + + +
  • +Oracle: +Remote Method Invocation (RMI). +
    • +
  • +ITNEXT: +Java RMI for pentesters part two - reconnaissance & attack against non-JMX registries. +
    • +
  • +MOGWAI LABS: +Attacking Java RMI services after JEP 290 +
    • +
  • +OWASP: +Deserialization of untrusted data. +
    • +
  • +OpenJDK: +JEP 290: Filter Incoming Serialization Data +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.ql b/java/ql/src/experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.ql new file mode 100644 index 000000000000..f870d6f423e1 --- /dev/null +++ b/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." diff --git a/java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.expected b/java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.expected new file mode 100644 index 000000000000..188a8649d985 --- /dev/null +++ b/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. | diff --git a/java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.java b/java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.java new file mode 100644 index 000000000000..197a1c478435 --- /dev/null +++ b/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 +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.qlref b/java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.qlref new file mode 100644 index 000000000000..fce2e6c6a4a7 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-502/UnsafeDeserializationRmi.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-502/UnsafeDeserializationRmi.ql \ No newline at end of file