Fix

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

@@ -1,8 +1,17 @@
+import java.lang.reflect.Method;
+import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
 import javax.servlet.http.HttpServletRequest;
 import org.springframework.stereotype.Controller;
+import org.springframework.util.StringUtils;
 import org.springframework.web.bind.annotation.GetMapping;
-import org.springframework.web.bind.annotation.ResponseBody;
+import org.springframework.web.bind.annotation.PathVariable;
+import org.springframework.web.bind.annotation.RequestBody;
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.multipart.MultipartFile;
 
 @Controller
 public class UnsafeReflection {
@@ -10,9 +19,11 @@ public class UnsafeReflection {
     @GetMapping(value = "uf1")
     public void bad1(HttpServletRequest request) {
         String className = request.getParameter("className");
+        String parameterValue = request.getParameter("parameterValue");
         try {
-            Class clazz = Class.forName(className); //bad
-        } catch (ClassNotFoundException e) {
+            Class clazz = Class.forName(className);
+            Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //bad
+        } catch (Exception e) {
             e.printStackTrace();
         }
     }
@@ -20,44 +31,150 @@ public class UnsafeReflection {
     @GetMapping(value = "uf2")
     public void bad2(HttpServletRequest request) {
         String className = request.getParameter("className");
+        String parameterValue = request.getParameter("parameterValue");
         try {
             ClassLoader classLoader = ClassLoader.getSystemClassLoader();
-            Class clazz = classLoader.loadClass(className); //bad
-        } catch (ClassNotFoundException e) {
+            Class clazz = classLoader.loadClass(className);
+            Object object = clazz.newInstance();
+            clazz.getDeclaredMethods()[0].invoke(object, parameterValue); //bad
+        } catch (Exception e) {
             e.printStackTrace();
         }
     }
 
+    @RequestMapping(value = {"/service/{beanIdOrClassName}/{methodName}"}, method = {RequestMethod.POST}, consumes = {"application/json"}, produces = {"application/json"})
+    public Object bad3(@PathVariable("beanIdOrClassName") String beanIdOrClassName, @PathVariable("methodName") String methodName, @RequestBody Map<String, Object> body) throws Exception {
+        List<Object> rawData = null;
+        try {
+            rawData = (List<Object>)body.get("methodInput");
+        } catch (Exception e) {
+            return e;
+        }
+        return invokeService(beanIdOrClassName, methodName, null, rawData);
+    }
+
     @GetMapping(value = "uf3")
     public void good1(HttpServletRequest request) throws Exception {
         HashSet<String> hashSet = new HashSet<>();
         hashSet.add("com.example.test1");
         hashSet.add("com.example.test2");
         String className = request.getParameter("className");
-        if (hashSet.contains(className)){ //good
+        String parameterValue = request.getParameter("parameterValue");
+        if (!hashSet.contains(className)){ 
             throw new Exception("Class not valid: "  + className);
         }
-        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
-        Class clazz = classLoader.loadClass(className);
+        try {
+            Class clazz = Class.forName(className);
+            Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //good
+        } catch (Exception e) {
+            e.printStackTrace();
+        }
     }
 
     @GetMapping(value = "uf4")
     public void good2(HttpServletRequest request) throws Exception {
         String className = request.getParameter("className");
-        if (!"com.example.test1".equals(className)){ //good
+        String parameterValue = request.getParameter("parameterValue");
+        if (!"com.example.test1".equals(className)){
             throw new Exception("Class not valid: "  + className);
         }
-        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
-        Class clazz = classLoader.loadClass(className);
+        try {
+            Class clazz = Class.forName(className);
+            Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //good
+        } catch (Exception e) {
+            e.printStackTrace();
+        }
     }
 
     @GetMapping(value = "uf5")
     public void good3(HttpServletRequest request) throws Exception {
         String className = request.getParameter("className");
+        String parameterValue = request.getParameter("parameterValue");
         if (!className.equals("com.example.test1")){ //good
             throw new Exception("Class not valid: "  + className);
         }
-        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
-        Class clazz = classLoader.loadClass(className);
+        try {
+            Class clazz = Class.forName(className);
+            Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //good
+        } catch (Exception e) {
+            e.printStackTrace();
+        }
     }
-}
+
+    private Object invokeService(String beanIdOrClassName, String methodName, MultipartFile[] files, List<Object> data) throws Exception {
+        BeanFactory beanFactory = new BeanFactory();
+        try {
+            Object bean = null;
+            String beanName = null;
+            Class<?> beanClass = null;
+            try {
+                beanClass = Class.forName(beanIdOrClassName);
+                beanName = StringUtils.uncapitalize(beanClass.getSimpleName());
+            } catch (ClassNotFoundException classNotFoundException) {
+                beanName = beanIdOrClassName;
+            }
+            try {
+                bean = beanFactory.getBean(beanName);
+            } catch (BeansException beansException) {
+                bean = beanFactory.getBean(beanClass);
+            }
+            byte b;
+            int i;
+            Method[] arrayOfMethod;
+            for (i = (arrayOfMethod = bean.getClass().getMethods()).length, b = 0; b < i; ) {
+                Method method = arrayOfMethod[b];
+                if (!method.getName().equals(methodName)) {
+                    b++;
+                    continue;
+                }
+                ProxygenSerializer serializer = new ProxygenSerializer();
+                Object[] methodInput = serializer.deserializeMethodInput(data, files, method);
+                Object result = method.invoke(bean, methodInput);
+                Map<String, Object> map = new HashMap<>();
+                map.put("result", serializer.serialize(result));
+                return map;
+            }
+        } catch (Exception e) {
+            return e;
+        }
+        return null;
+    }
+}
+
+class BeansException extends Exception {
+
+}
+
+class BeanFactory {
+
+    private static HashMap<String, Object> classNameMap = new HashMap<>();
+
+    private static HashMap<Class<?>, Object> classMap = new HashMap<>();;
+
+    static {
+        classNameMap.put("xxxx", Runtime.getRuntime());
+        classMap.put(Runtime.class, Runtime.getRuntime());
+    }
+
+    public Object getBean(String className) throws BeansException {
+        if (classNameMap.get(className) == null) {
+            throw new BeansException();
+        }
+        return classNameMap.get(className);
+    }
+
+    public Object getBean(Class<?> clzz) {
+        return classMap.get(clzz);
+    }
+}
+
+class ProxygenSerializer {
+
+    public Object[] deserializeMethodInput(List<Object> data, MultipartFile[] files, Method method) {
+        return null;
+    }
+
+    public String serialize(Object result) {
+        return null;
+    }
+}

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

@@ -3,10 +3,8 @@
 
 <overview>
 <p>
-Allowing users to freely select a class to load can result in invocation of unexpected dangerous code.
-Dynamically loaded classes could contain dangerous code executed by a constructor or
-static class initializer, which means a vulnerability can rairse even without invoking methods 
-on such classes to be vulnerable to an attack.
+Allowing users to freely choose to call the constructor or method of the class through reflection. This means that 
+malicious users can carefully construct requests to create instances of any class or call methods of any class.
 </p>
 </overview>
 
@@ -19,9 +17,10 @@ users can only execute classes or codes that are running.
 
 <example>
 <p>
-The following examples show good examples and bad examples respectively. When using <code>Class.forName(...)</code> 
-or <code>ClassLoader.loadClass(...)</code> and not verifying user input, it is easy to cause security risks, for example: 
-<code>bad1</code>/<code>bad2</code>. When the user input is verified by <code>contains</code> or <code>equals</code> and then 
+The following examples are good examples and bad examples. When using <code>Class.forName(...)</code> or 
+<code>ClassLoader.loadClass(...)</code> to get the class, then create the class object or call the method of the class. 
+When the user is not authenticated Input, it is easy to cause security risks, for example: <code>bad1</code>/<code>bad2
+</code>/<code>bad3</code>. When the user input is verified by <code>contains</code> or <code>equals</code> and then 
 the reflection operation is performed, the system security can be well controlled, for example: 
 <code >good1</code>/<code>good2</code>/<code>good3</code>.
 </p>

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

@@ -11,7 +11,9 @@
  */
 
 import java
+import DataFlow
 import UnsafeReflectionLib
+import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.FlowSources
 import DataFlow::PathGraph
 
@@ -19,7 +21,7 @@ private class ContainsSanitizer extends DataFlow::BarrierGuard {
   ContainsSanitizer() { this.(MethodAccess).getMethod().hasName("contains") }
 
   override predicate checks(Expr e, boolean branch) {
-    e = this.(MethodAccess).getArgument(0) and branch = false
+    e = this.(MethodAccess).getArgument(0) and branch = true
   }
 }
 
@@ -39,6 +41,45 @@ 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 |
+      rcimac.getArgument(0) = pred.asExpr() and rcimac = succ.asExpr()
+    )
+    or
+    exists(MethodAccess ma |
+      (
+        ma instanceof ReflectiveConstructorsAccess or
+        ma instanceof ReflectiveMethodsAccess
+      ) and
+      ma.getQualifier() = pred.asExpr() and
+      ma = succ.asExpr()
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().getReturnType() instanceof TypeObject and
+      ma.getMethod().getAParamType() instanceof TypeClass and
+      ma.getAnArgument() = pred.asExpr() and
+      ma = succ.asExpr()
+    )
+    or
+    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 |
+      m = ma.getMethod() and arg = ma.getArgument(i)
+    |
+      m.getReturnType() instanceof JacksonTypeDescriptorType and
+      m.getName().toLowerCase().regexpMatch("resolve|load|class|type") and
+      arg.getType() instanceof TypeString and
+      arg = pred.asExpr() and
+      ma = succ.asExpr()
+    )
+  }
+
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof ContainsSanitizer or guard instanceof EqualsSanitizer
   }

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

@@ -1,13 +1,103 @@
 import java
+import DataFlow
 import semmle.code.java.Reflection
 import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.DataFlow3
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.TaintTracking2
+
+/** Type descriptors in Jackson libraries. */
+class JacksonTypeDescriptorType extends RefType {
+  JacksonTypeDescriptorType() {
+    this instanceof TypeClass or
+    hasQualifiedName("com.fasterxml.jackson.databind", "JavaType") or
+    hasQualifiedName("com.fasterxml.jackson.core.type", "TypeReference")
+  }
+}
 
 /**
- * Unsafe reflection sink,
- * e.g `Class.forName(...)` or `ClassLoader.loadClass(...)`.
+ * A call to a Java standard library method which constructs or returns a `Class<T>` from a `String`.
+ * e.g `Class.forName(...)` or `ClassLoader.loadClass(...)`
+ */
+class ReflectiveClassIdentifierMethodAccessCall extends MethodAccess {
+  ReflectiveClassIdentifierMethodAccessCall() {
+    this instanceof ReflectiveClassIdentifierMethodAccess
+  }
+}
+
+/**
+ * Unsafe reflection sink.
+ * e.g `Constructor.newInstance(...)` or `Method.invoke(...)` or `Class.newInstance()`.
  */
 class UnsafeReflectionSink extends DataFlow::ExprNode {
   UnsafeReflectionSink() {
-    exists(ReflectiveClassIdentifierMethodAccess rcima | rcima.getArgument(0) = this.getExpr())
+    exists(MethodAccess ma |
+      (
+        ma.getMethod().hasQualifiedName("java.lang.reflect", "Constructor<>", "newInstance")
+        or
+        ma.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke")
+      ) and
+      ma.getQualifier() = this.asExpr() and
+      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 {
+  ReflectionArgsConfig() { this = "ReflectionArgsConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(NewInstance ni | ni.getAnArgument() = sink.asExpr())
+    or
+    exists(MethodAccess ma, ReflectionInvokeObjectConfig rioc |
+      ma.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke") and
+      ma.getArgument(1) = sink.asExpr() and
+      rioc.hasFlow(_, DataFlow::exprNode(ma.getArgument(0)))
+    )
+  }
+
+  override predicate isAdditionalTaintStep(Node pred, Node succ) {
+    exists(MethodAccess ma |
+      ma.getMethod().getReturnType().hasName("Object[]") and
+      ma.getAnArgument() = pred.asExpr() and
+      ma = succ.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 {
+  ReflectionInvokeObjectConfig() { this = "ReflectionInvokeObjectConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(ReflectiveClassIdentifierMethodAccessCall rma | rma = source.asExpr())
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma |
+      ma.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke") and
+      ma.getArgument(0) = sink.asExpr()
+    )
+  }
+
+  override predicate isAdditionalFlowStep(Node pred, Node succ) {
+    exists(NewInstance ni |
+      ni.getQualifier() = pred.asExpr() and
+      ni = succ.asExpr()
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().getReturnType() instanceof TypeObject and
+      (
+        ma.getMethod().getAParamType() instanceof TypeString or
+        ma.getMethod().getAParamType() instanceof TypeClass
+      ) and
+      ma.getAnArgument() = pred.asExpr() and
+      ma = succ.asExpr()
+    )
   }
 }