From 291ca3830a4cccb768d7fee02133bf18bd2eeae0 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 23 Jul 2021 09:28:55 +0800 Subject: [PATCH] Modify according to suggestions --- .../CWE/CWE-470/UnsafeReflection.java | 47 ++----------------- .../CWE/CWE-470/UnsafeReflection.qhelp | 15 ++---- .../Security/CWE/CWE-470/UnsafeReflection.ql | 13 ++--- .../CWE/CWE-470/UnsafeReflectionLib.qll | 9 ---- .../CWE-470/UnsafeReflection.expected | 5 ++ 5 files changed, 20 insertions(+), 69 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 7ef6b70d8a8..02d3977daf1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java @@ -16,34 +16,8 @@ import org.springframework.web.multipart.MultipartFile; @Controller 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); - Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //bad - } catch (Exception e) { - e.printStackTrace(); - } - } - - @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); - 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 { + public Object bad1(@PathVariable("beanIdOrClassName") String beanIdOrClassName, @PathVariable("methodName") String methodName, @RequestBody Map body) throws Exception { List rawData = null; try { rawData = (List)body.get("methodInput"); @@ -53,7 +27,7 @@ public class UnsafeReflection { return invokeService(beanIdOrClassName, methodName, null, rawData); } - @GetMapping(value = "uf3") + @GetMapping(value = "uf1") public void good1(HttpServletRequest request) throws Exception { HashSet hashSet = new HashSet<>(); hashSet.add("com.example.test1"); @@ -71,7 +45,7 @@ public class UnsafeReflection { } } - @GetMapping(value = "uf4") + @GetMapping(value = "uf2") public void good2(HttpServletRequest request) throws Exception { String className = request.getParameter("className"); String parameterValue = request.getParameter("parameterValue"); @@ -86,21 +60,6 @@ public class UnsafeReflection { } } - @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); - } - 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 data) throws Exception { BeanFactory beanFactory = new BeanFactory(); try { 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 907d94b1f1a..b4c66b9ef00 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.qhelp @@ -3,26 +3,21 @@

-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. +Allowing users to freely choose the name of a class to instantiate could provide means to attack a vulnerable appplication.

-Create a list of classes that are allowed to load reflectively and strictly verify the input content to ensure that -users can only execute classes or codes that are running. +Create a list of classes that are allowed to load reflectively and strictly verify the input to ensure that +users can only instantiate classes or execute methods that ought to be allowed.

-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. +The bad method shown below illustrate class loading with Class.forName without any check on the particular class being instantiated. +The good methods illustrate some different ways to restrict which classes can be instantiated.

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 00e1bcfff75..2f7d6a4b482 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql @@ -55,10 +55,12 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration { ma = succ.asExpr() ) or - exists(MethodAccess ma | - ma.getMethod().getReturnType() instanceof TypeObject and - ma.getMethod().getAParamType() instanceof TypeClass and - ma.getAnArgument() = pred.asExpr() and + 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() ) or @@ -72,8 +74,7 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration { 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 + m.getReturnType() instanceof TypeClass and arg.getType() instanceof TypeString and arg = pred.asExpr() and ma = succ.asExpr() 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 d57e169c83a..eb8a6e33766 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll @@ -7,15 +7,6 @@ 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") - } -} - /** * A call to a Java standard library method which constructs or returns a `Class` from a `String`. * e.g `Class.forName(...)` or `ClassLoader.loadClass(...)` 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 c2a4102384a..0f84f6d90f3 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 @@ -7,7 +7,10 @@ edges | 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:119:44:119:52 | beanClass : Class | | UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:132:33:132:38 | method | +| UnsafeReflection.java:119:24:119:53 | getBean(...) : Object | UnsafeReflection.java:132:33:132:38 | method | +| UnsafeReflection.java:119:44:119:52 | beanClass : Class | UnsafeReflection.java:119:24:119:53 | getBean(...) : Object | nodes | UnsafeReflection.java:21:28:21:60 | getParameter(...) : String | semmle.label | getParameter(...) : String | | UnsafeReflection.java:25:29:25:59 | getDeclaredConstructors(...) : Constructor[] | semmle.label | getDeclaredConstructors(...) : Constructor[] | @@ -18,6 +21,8 @@ nodes | 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:119:24:119:53 | getBean(...) : Object | semmle.label | getBean(...) : Object | +| UnsafeReflection.java:119:44:119:52 | beanClass : Class | semmle.label | beanClass : Class | | UnsafeReflection.java:132:33:132:38 | method | semmle.label | method | #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 |