From 2a50cf8244b0baf2fffcd9e89d4049da1b04d547 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 22 Jul 2021 22:24:09 +0800 Subject: [PATCH] Fix --- .../CWE/CWE-470/UnsafeReflection.java | 145 ++++++++++++++++-- .../CWE/CWE-470/UnsafeReflection.qhelp | 13 +- .../Security/CWE/CWE-470/UnsafeReflection.ql | 43 +++++- .../CWE/CWE-470/UnsafeReflectionLib.qll | 96 +++++++++++- java/ql/src/semmle/code/java/Reflection.qll | 20 +++ .../CWE-470/UnsafeReflection.expected | 30 +++- .../security/CWE-470/UnsafeReflection.java | 145 ++++++++++++++++-- .../query-tests/security/CWE-470/options | 2 +- .../web/bind/annotation/PathVariable.java | 1 + 9 files changed, 447 insertions(+), 48 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java index cf7ff8a71a1..7ef6b70d8a8 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java +++ b/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 body) throws Exception { + List rawData = null; + try { + rawData = (List)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 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(); + } } -} \ No newline at end of file + + private Object invokeService(String beanIdOrClassName, String methodName, MultipartFile[] files, List 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 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 classNameMap = new HashMap<>(); + + private static HashMap, 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 data, MultipartFile[] files, Method method) { + return null; + } + + public String serialize(Object result) { + return null; + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.qhelp b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.qhelp index 7a792caf229..907d94b1f1a 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.qhelp @@ -3,10 +3,8 @@

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

@@ -19,9 +17,10 @@ users can only execute classes or codes that are running.

-The following examples show good examples and bad examples respectively. When using Class.forName(...) -or ClassLoader.loadClass(...) and not verifying user input, it is easy to cause security risks, for example: -bad1/bad2. When the user input is verified by contains or equals and then +The following examples are good examples and bad examples. When using Class.forName(...) or +ClassLoader.loadClass(...) 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: bad1/bad2 +/bad3. When the user input is verified by contains or equals and then the reflection operation is performed, the system security can be well controlled, for example: good1/good2/good3.

diff --git a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql index 859e4df1012..00e1bcfff75 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql +++ b/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 } diff --git a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll index 79f1b21d4ab..d57e169c83a 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll +++ b/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` 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() + ) } } diff --git a/java/ql/src/semmle/code/java/Reflection.qll b/java/ql/src/semmle/code/java/Reflection.qll index 5a46fcb8be2..b58cc1e6692 100644 --- a/java/ql/src/semmle/code/java/Reflection.qll +++ b/java/ql/src/semmle/code/java/Reflection.qll @@ -314,6 +314,26 @@ class ClassMethodAccess extends MethodAccess { } } +/** + * A call to `Class.getConstructors(..)` or `Class.getDeclaredConstructors(..)`. + */ +class ReflectiveConstructorsAccess extends ClassMethodAccess { + ReflectiveConstructorsAccess() { + this.getCallee().hasName("getConstructors") or + this.getCallee().hasName("getDeclaredConstructors") + } +} + +/** + * A call to `Class.getMethods(..)` or `Class.getDeclaredMethods(..)`. + */ +class ReflectiveMethodsAccess extends ClassMethodAccess { + ReflectiveMethodsAccess() { + this.getCallee().hasName("getMethods") or + this.getCallee().hasName("getDeclaredMethods") + } +} + /** * A call to `Class.getMethod(..)` or `Class.getDeclaredMethod(..)`. */ diff --git a/java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.expected b/java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.expected index 0ec18ef5be9..c2a4102384a 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.expected @@ -1,11 +1,25 @@ edges -| UnsafeReflection.java:12:28:12:60 | getParameter(...) : String | UnsafeReflection.java:14:41:14:49 | className | -| UnsafeReflection.java:22:28:22:60 | getParameter(...) : String | UnsafeReflection.java:25:49:25:57 | className | +| 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: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: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:53:30:53:46 | beanIdOrClassName : String | UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | +| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:132:33:132:38 | method | nodes -| UnsafeReflection.java:12:28:12:60 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeReflection.java:14:41:14:49 | className | semmle.label | className | -| UnsafeReflection.java:22:28:22:60 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeReflection.java:25:49:25:57 | className | semmle.label | className | +| UnsafeReflection.java:21:28:21:60 | 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:33:28:33:60 | 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:46:24:46:82 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String | +| UnsafeReflection.java:53:30:53:46 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String | +| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String | +| UnsafeReflection.java:132:33:132:38 | method | semmle.label | method | #select -| UnsafeReflection.java:14:41:14:49 | className | UnsafeReflection.java:12:28:12:60 | getParameter(...) : String | UnsafeReflection.java:14:41:14:49 | className | Unsafe reflection of $@. | UnsafeReflection.java:12:28:12:60 | getParameter(...) | user input | -| UnsafeReflection.java:25:49:25:57 | className | UnsafeReflection.java:22:28:22:60 | getParameter(...) : String | UnsafeReflection.java:25:49:25:57 | className | Unsafe reflection of $@. | UnsafeReflection.java:22:28:22:60 | getParameter(...) | user input | +| 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 | +| UnsafeReflection.java:132:33:132:38 | method | UnsafeReflection.java:46:24:46:82 | beanIdOrClassName : String | UnsafeReflection.java:132:33:132:38 | method | Unsafe reflection of $@. | UnsafeReflection.java:46:24:46:82 | beanIdOrClassName | user input | diff --git a/java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.java b/java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.java index cf7ff8a71a1..7ef6b70d8a8 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.java +++ b/java/ql/test/experimental/query-tests/security/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 body) throws Exception { + List rawData = null; + try { + rawData = (List)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 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(); + } } -} \ No newline at end of file + + private Object invokeService(String beanIdOrClassName, String methodName, MultipartFile[] files, List 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 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 classNameMap = new HashMap<>(); + + private static HashMap, 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 data, MultipartFile[] files, Method method) { + return null; + } + + public String serialize(Object result) { + return null; + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-470/options b/java/ql/test/experimental/query-tests/security/CWE-470/options index a9289108747..ba166b547a0 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-470/options +++ b/java/ql/test/experimental/query-tests/security/CWE-470/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/ \ No newline at end of file +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/ \ No newline at end of file diff --git a/java/ql/test/stubs/springframework-5.3.8/org/springframework/web/bind/annotation/PathVariable.java b/java/ql/test/stubs/springframework-5.3.8/org/springframework/web/bind/annotation/PathVariable.java index 52411ebce00..1de6872d089 100644 --- a/java/ql/test/stubs/springframework-5.3.8/org/springframework/web/bind/annotation/PathVariable.java +++ b/java/ql/test/stubs/springframework-5.3.8/org/springframework/web/bind/annotation/PathVariable.java @@ -11,4 +11,5 @@ import java.lang.annotation.Target; @Documented public @interface PathVariable { + String value() default ""; } \ No newline at end of file