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 02d3977daf1..a6129ff8615 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java @@ -62,78 +62,42 @@ public class UnsafeReflection { 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; + try { + Object bean = null; + Class beanClass = Class.forName(beanIdOrClassName); + 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; + } + Object result = method.invoke(bean, data); + Map map = new HashMap<>(); + return map; + } + } catch (Exception e) { + return e; + } + return null; } } -class BeansException extends Exception { - -} - class BeanFactory { - private static HashMap classNameMap = new HashMap<>(); + private static HashMap classNameMap = new HashMap<>(); - private static HashMap, Object> classMap = new HashMap<>();; + private static HashMap, Object> classMap = new HashMap<>(); - static { - classNameMap.put("xxxx", Runtime.getRuntime()); - classMap.put(Runtime.class, Runtime.getRuntime()); - } + 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; - } + public Object getBean(Class clzz) { + return classMap.get(clzz); + } } 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 2f7d6a4b482..a6346879c2d 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql @@ -55,16 +55,9 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration { ma = succ.asExpr() ) or - exists(MethodAccess ma, Method m, int i, Expr arg | - m = ma.getMethod() and arg = ma.getArgument(i) + exists( + MethodAccess ma // Object.getClass() | - m.getReturnType() instanceof TypeObject and - arg.getType() instanceof TypeClass and - arg = 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 @@ -79,6 +72,15 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration { arg = 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 TypeObject and + arg.getType() instanceof TypeClass and + arg = pred.asExpr() and + ma = succ.asExpr() + ) } override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { 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 eb8a6e33766..17f44e977bd 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll @@ -1,10 +1,8 @@ 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 /** @@ -44,18 +42,10 @@ class ReflectionArgsConfig extends TaintTracking2::Configuration { override predicate isSink(DataFlow::Node sink) { exists(NewInstance ni | ni.getAnArgument() = sink.asExpr()) or - exists(MethodAccess ma, ReflectionInvokeObjectConfig rioc | + exists(MethodAccess ma | 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() + exists(ReflectionInvokeObjectConfig rioc | rioc.hasFlowToExpr(ma.getArgument(0))) ) } } @@ -81,13 +71,12 @@ class ReflectionInvokeObjectConfig extends DataFlow3::Configuration { 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 + 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() ) } 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 0f84f6d90f3..543948067c2 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,10 +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 | +| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:109:31:109:39 | beanClass : Class | +| 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:109:31:109:39 | beanClass : Class | UnsafeReflection.java:109:11:109:40 | 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[] | @@ -21,10 +21,10 @@ 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 | +| UnsafeReflection.java:109:11:109:40 | getBean(...) : Object | semmle.label | getBean(...) : Object | +| UnsafeReflection.java:109:31:109:39 | beanClass : Class | semmle.label | beanClass : Class | +| UnsafeReflection.java:119:21:119:26 | 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 | | 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 | +| UnsafeReflection.java:119:21:119:26 | method | UnsafeReflection.java:46:24:46:82 | beanIdOrClassName : String | UnsafeReflection.java:119:21:119:26 | 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 7ef6b70d8a8..d9dc0573660 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 @@ -103,78 +103,42 @@ public class UnsafeReflection { 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; + try { + Object bean = null; + Class beanClass = Class.forName(beanIdOrClassName); + 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; + } + Object result = method.invoke(bean, data); + Map map = new HashMap<>(); + return map; + } + } catch (Exception e) { + return e; + } + return null; } } -class BeansException extends Exception { - -} - class BeanFactory { - private static HashMap classNameMap = new HashMap<>(); + private static HashMap classNameMap = new HashMap<>(); - private static HashMap, Object> classMap = new HashMap<>();; + private static HashMap, Object> classMap = new HashMap<>(); - static { - classNameMap.put("xxxx", Runtime.getRuntime()); - classMap.put(Runtime.class, Runtime.getRuntime()); - } + 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; - } + public Object getBean(Class clzz) { + return classMap.get(clzz); + } }