Model written using smowton

java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql

@@ -42,10 +42,12 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeReflectionSink }
 
   override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(ReflectiveClassIdentifierMethodAccessCall rcimac |
+    // Argument -> return of Class.forName, ClassLoader.loadClass
+    exists(ReflectiveClassIdentifierMethodAccess rcimac |
       rcimac.getArgument(0) = pred.asExpr() and rcimac = succ.asExpr()
     )
     or
+    // Qualifier -> return of Class.getDeclaredConstructors/Methods and similar
     exists(MethodAccess ma |
       (
         ma instanceof ReflectiveConstructorsAccess or
@@ -55,31 +57,42 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration {
       ma = succ.asExpr()
     )
     or
-    exists(
+    // Qualifier -> return of Object.getClass
-      MethodAccess ma // Object.getClass()
+    exists(MethodAccess ma |
-    |
       ma.getMethod().hasName("getClass") and
       ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Object") and
       ma.getQualifier() = pred.asExpr() and
       ma = succ.asExpr()
     )
     or
-    exists(MethodAccess ma, Method m, int i, Expr arg |
+    // Argument -> return of methods that look like Class.forName
-      m = ma.getMethod() and arg = ma.getArgument(i)
+    looksLikeResolveClassStep(pred, succ)
-    |
+    or
-      m.getReturnType() instanceof TypeClass and
+    // Argument -> return of methods that look like `Object getInstance(Class c)`
-      arg.getType() instanceof TypeString and
+    looksLikeInstantiateClassStep(pred, succ)
-      arg = pred.asExpr() and
+    or
-      ma = succ.asExpr()
+    // Argument -> return of BeanFactory.getBean
+    exists(MethodAccess ma, Method getBean, Expr argument |
+      getBean.hasQualifiedName("org.springframework.beans.factory", "BeanFactory", "getBean") and
+      (
+        ma.getMethod().overrides(getBean)
+        or
+        ma.getMethod() = getBean
+      ) and
+      argument = ma.getAnArgument() and
+      (
+        argument.getType() instanceof TypeString
+        or
+        argument.getType() instanceof TypeClass
+      ) and
+      pred.asExpr() = argument and
+      succ.asExpr() = ma
     )
     or
-    exists(MethodAccess ma, Method m, int i, Expr arg |
+    // Qualifier -> return of Constructor.newInstance, Class.newInstance
-      m = ma.getMethod() and arg = ma.getArgument(i)
+    exists(NewInstance ni |
-    |
+      ni.getQualifier() = pred.asExpr() and
-      m.getReturnType() instanceof TypeObject and
+      ni = succ.asExpr()
-      arg.getType() instanceof TypeClass and
-      arg = pred.asExpr() and
-      ma = succ.asExpr()
     )
   }
 
@@ -88,6 +101,17 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration {
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeReflectionConfig conf
+private Expr getAMethodArgument(MethodAccess reflectiveCall) {
-where conf.hasFlowPath(source, sink)
+  result = reflectiveCall.(NewInstance).getAnArgument()
+  or
+  result = reflectiveCall.(MethodInvokeCall).getAnArgument()
+}
+
+from
+  DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeReflectionConfig conf,
+  MethodAccess reflectiveCall
+where
+  conf.hasFlowPath(source, sink) and
+  sink.getNode().asExpr() = reflectiveCall.getQualifier() and
+  conf.hasFlowToExpr(getAMethodArgument(reflectiveCall))
 select sink.getNode(), source, sink, "Unsafe reflection of $@.", source.getNode(), "user input"

java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll

@@ -1,83 +1,62 @@
 import java
 import DataFlow
 import semmle.code.java.Reflection
-import semmle.code.java.dataflow.DataFlow3
 import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.dataflow.TaintTracking2
 
 /**
- * A call to a Java standard library method which constructs or returns a `Class<T>` from a `String`.
+ * A call to `java.lang.reflect.Method.invoke`.
- * e.g `Class.forName(...)` or `ClassLoader.loadClass(...)`
  */
-class ReflectiveClassIdentifierMethodAccessCall extends MethodAccess {
+class MethodInvokeCall extends MethodAccess {
-  ReflectiveClassIdentifierMethodAccessCall() {
+  MethodInvokeCall() { this.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke") }
-    this instanceof ReflectiveClassIdentifierMethodAccess
-  }
 }
 
 /**
- * Unsafe reflection sink.
+ * Unsafe reflection sink (the qualifier or method arguments to `Constructor.newInstance(...)` or `Method.invoke(...)`)
- * e.g `Constructor.newInstance(...)` or `Method.invoke(...)` or `Class.newInstance()`.
  */
 class UnsafeReflectionSink extends DataFlow::ExprNode {
   UnsafeReflectionSink() {
     exists(MethodAccess ma |
       (
-        ma.getMethod().hasQualifiedName("java.lang.reflect", "Constructor<>", "newInstance")
+        ma.getMethod().hasQualifiedName("java.lang.reflect", "Constructor<>", "newInstance") or
-        or
+        ma instanceof MethodInvokeCall
-        ma.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke")
       ) and
-      ma.getQualifier() = this.asExpr() and
+      this.asExpr() = [ma.getQualifier(), ma.getAnArgument()]
-      exists(ReflectionArgsConfig rac | rac.hasFlowToExpr(ma.getAnArgument()))
     )
   }
 }
 
-/** Taint-tracking configuration tracing flow from remote sources to specifying the initialization parameters to the constructor or method. */
+/**
-class ReflectionArgsConfig extends TaintTracking2::Configuration {
+ * Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
-  ReflectionArgsConfig() { this = "ReflectionArgsConfig" }
+ * A method probably resolves a class if it takes a string, returns a Class
-
+ * and its name contains "resolve", "load", etc.
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+ */
-
+predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-  override predicate isSink(DataFlow::Node sink) {
+  exists(MethodAccess ma, Method m, int i, Expr arg |
-    exists(NewInstance ni | ni.getAnArgument() = sink.asExpr())
+    m = ma.getMethod() and arg = ma.getArgument(i)
-    or
+  |
-    exists(MethodAccess ma |
+    m.getReturnType() instanceof TypeClass and
-      ma.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke") and
+    m.getName().toLowerCase().regexpMatch("resolve|load|class|type") and
-      ma.getArgument(1) = sink.asExpr() and
+    arg.getType() instanceof TypeString and
-      exists(ReflectionInvokeObjectConfig rioc | rioc.hasFlowToExpr(ma.getArgument(0)))
+    arg = fromNode.asExpr() and
-    )
+    ma = toNode.asExpr()
-  }
+  )
 }
 
-/** A data flow configuration tracing flow from the class object associated with the class to specifying the initialization parameters. */
+/**
-class ReflectionInvokeObjectConfig extends DataFlow3::Configuration {
+ * Holds if `fromNode` to `toNode` is a dataflow step that looks like instantiating a class.
-  ReflectionInvokeObjectConfig() { this = "ReflectionInvokeObjectConfig" }
+ * A method probably instantiates a class if it is external, takes a Class, returns an Object
-
+ * and its name contains "instantiate" or similar terms.
-  override predicate isSource(DataFlow::Node source) {
+ */
-    exists(ReflectiveClassIdentifierMethodAccessCall rma | rma = source.asExpr())
+predicate looksLikeInstantiateClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-  }
+  exists(MethodAccess ma, Method m, int i, Expr arg |
-
+    m = ma.getMethod() and arg = ma.getArgument(i)
-  override predicate isSink(DataFlow::Node sink) {
+  |
-    exists(MethodAccess ma |
+    m.getReturnType() instanceof TypeObject and
-      ma.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke") and
+    m.getName()
-      ma.getArgument(0) = sink.asExpr()
+        .toLowerCase()
-    )
+        .regexpMatch("instantiate|instance|create|make|getbean|instantiateclass") and
-  }
+    arg.getType() instanceof TypeClass and
-
+    arg = fromNode.asExpr() and
-  override predicate isAdditionalFlowStep(Node pred, Node succ) {
+    ma = toNode.asExpr()
-    exists(NewInstance ni |
+  )
-      ni.getQualifier() = pred.asExpr() and
-      ni = succ.asExpr()
-    )
-    or
-    exists(MethodAccess ma, Method m, int i, Expr arg |
-      m = ma.getMethod() and arg = ma.getArgument(i)
-    |
-      m.getReturnType() instanceof TypeObject and
-      arg.getType() instanceof TypeClass and
-      arg = pred.asExpr() and
-      ma = succ.asExpr()
-    )
-  }
 }

java/ql/src/semmle/code/java/Reflection.qll

@@ -59,7 +59,7 @@ private class ReflectiveClassIdentifierLiteral extends ReflectiveClassIdentifier
 /**
  * A call to a Java standard library method which constructs or returns a `Class<T>` from a `String`.
  */
-library class ReflectiveClassIdentifierMethodAccess extends ReflectiveClassIdentifier, MethodAccess {
+class ReflectiveClassIdentifierMethodAccess extends ReflectiveClassIdentifier, MethodAccess {
   ReflectiveClassIdentifierMethodAccess() {
     // A call to `Class.forName(...)`, from which we can infer `T` in the returned type `Class<T>`.
     getCallee().getDeclaringType() instanceof TypeClass and getCallee().hasName("forName")

java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.expected

@@ -1,29 +1,56 @@
 edges
 | UnsafeReflection.java:21:28:21:60 | getParameter(...) : String | UnsafeReflection.java:25:29:25:59 | getDeclaredConstructors(...) : Constructor[] |
 | UnsafeReflection.java:21:28:21:60 | getParameter(...) : String | UnsafeReflection.java:25:29:25:62 | ...[...] |
+| UnsafeReflection.java:22:33:22:70 | getParameter(...) : String | UnsafeReflection.java:25:76:25:89 | parameterValue |
 | UnsafeReflection.java:25:29:25:59 | getDeclaredConstructors(...) : Constructor[] | UnsafeReflection.java:25:29:25:62 | ...[...] |
 | UnsafeReflection.java:33:28:33:60 | getParameter(...) : String | UnsafeReflection.java:39:13:39:38 | getDeclaredMethods(...) : Method[] |
 | UnsafeReflection.java:33:28:33:60 | getParameter(...) : String | UnsafeReflection.java:39:13:39:41 | ...[...] |
+| UnsafeReflection.java:33:28:33:60 | getParameter(...) : String | UnsafeReflection.java:39:50:39:55 | object |
+| UnsafeReflection.java:34:33:34:70 | getParameter(...) : String | UnsafeReflection.java:39:58:39:71 | parameterValue |
 | UnsafeReflection.java:39:13:39:38 | getDeclaredMethods(...) : Method[] | UnsafeReflection.java:39:13:39:41 | ...[...] |
 | UnsafeReflection.java:46:24:46:82 | beanIdOrClassName : String | UnsafeReflection.java:53:30:53:46 | beanIdOrClassName : String |
+| UnsafeReflection.java:46:132:46:168 | body : Map | UnsafeReflection.java:49:37:49:40 | body : Map |
+| UnsafeReflection.java:49:23:49:59 | (...)... : Object | UnsafeReflection.java:53:67:53:73 | rawData : Object |
+| UnsafeReflection.java:49:37:49:40 | body : Map | UnsafeReflection.java:49:37:49:59 | get(...) : Object |
+| UnsafeReflection.java:49:37:49:59 | get(...) : Object | UnsafeReflection.java:49:23:49:59 | (...)... : Object |
 | UnsafeReflection.java:53:30:53:46 | beanIdOrClassName : String | UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String |
-| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:109:31:109:39 | beanClass : Class |
+| UnsafeReflection.java:53:67:53:73 | rawData : Object | UnsafeReflection.java:104:102:104:118 | data : Object |
+| UnsafeReflection.java:62:33:62:70 | getParameter(...) : String | UnsafeReflection.java:68:76:68:89 | parameterValue |
+| UnsafeReflection.java:77:33:77:70 | getParameter(...) : String | UnsafeReflection.java:83:76:83:89 | parameterValue |
+| UnsafeReflection.java:92:33:92:70 | getParameter(...) : String | UnsafeReflection.java:98:76:98:89 | parameterValue |
 | UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:119:21:119:26 | method |
-| UnsafeReflection.java:109:11:109:40 | getBean(...) : Object | UnsafeReflection.java:119:21:119:26 | method |
+| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:119:35:119:38 | bean |
-| UnsafeReflection.java:109:31:109:39 | beanClass : Class | UnsafeReflection.java:109:11:109:40 | getBean(...) : Object |
+| UnsafeReflection.java:104:102:104:118 | data : Object | UnsafeReflection.java:119:41:119:44 | data |
 nodes
 | UnsafeReflection.java:21:28:21:60 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeReflection.java:22:33:22:70 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | UnsafeReflection.java:25:29:25:59 | getDeclaredConstructors(...) : Constructor[] | semmle.label | getDeclaredConstructors(...) : Constructor[] |
 | UnsafeReflection.java:25:29:25:62 | ...[...] | semmle.label | ...[...] |
+| UnsafeReflection.java:25:76:25:89 | parameterValue | semmle.label | parameterValue |
 | UnsafeReflection.java:33:28:33:60 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeReflection.java:34:33:34:70 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | UnsafeReflection.java:39:13:39:38 | getDeclaredMethods(...) : Method[] | semmle.label | getDeclaredMethods(...) : Method[] |
 | UnsafeReflection.java:39:13:39:41 | ...[...] | semmle.label | ...[...] |
+| UnsafeReflection.java:39:50:39:55 | object | semmle.label | object |
+| UnsafeReflection.java:39:58:39:71 | parameterValue | semmle.label | parameterValue |
 | UnsafeReflection.java:46:24:46:82 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String |
+| UnsafeReflection.java:46:132:46:168 | body : Map | semmle.label | body : Map |
+| UnsafeReflection.java:49:23:49:59 | (...)... : Object | semmle.label | (...)... : Object |
+| UnsafeReflection.java:49:37:49:40 | body : Map | semmle.label | body : Map |
+| UnsafeReflection.java:49:37:49:59 | get(...) : Object | semmle.label | get(...) : Object |
 | UnsafeReflection.java:53:30:53:46 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String |
+| UnsafeReflection.java:53:67:53:73 | rawData : Object | semmle.label | rawData : Object |
+| UnsafeReflection.java:62:33:62:70 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeReflection.java:68:76:68:89 | parameterValue | semmle.label | parameterValue |
+| UnsafeReflection.java:77:33:77:70 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeReflection.java:83:76:83:89 | parameterValue | semmle.label | parameterValue |
+| UnsafeReflection.java:92:33:92:70 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeReflection.java:98:76:98:89 | parameterValue | semmle.label | parameterValue |
 | UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String |
-| UnsafeReflection.java:109:11:109:40 | getBean(...) : Object | semmle.label | getBean(...) : Object |
+| UnsafeReflection.java:104:102:104:118 | data : Object | semmle.label | data : Object |
-| UnsafeReflection.java:109:31:109:39 | beanClass : Class | semmle.label | beanClass : Class |
 | UnsafeReflection.java:119:21:119:26 | method | semmle.label | method |
+| UnsafeReflection.java:119:35:119:38 | bean | semmle.label | bean |
+| UnsafeReflection.java:119:41:119:44 | data | semmle.label | data |
 #select
 | UnsafeReflection.java:25:29:25:62 | ...[...] | UnsafeReflection.java:21:28:21:60 | getParameter(...) : String | UnsafeReflection.java:25:29:25:62 | ...[...] | Unsafe reflection of $@. | UnsafeReflection.java:21:28:21:60 | getParameter(...) | user input |
 | UnsafeReflection.java:39:13:39:41 | ...[...] | UnsafeReflection.java:33:28:33:60 | getParameter(...) : String | UnsafeReflection.java:39:13:39:41 | ...[...] | Unsafe reflection of $@. | UnsafeReflection.java:33:28:33:60 | getParameter(...) | user input |