Skip to content

Commit fad1622

Browse files
smowtonsmowton
authored
Merge pull request github#5435 from haby0/DynamicallyLoadedClasses
Java: CWE-470 Use of Externally-Controlled Input to Select Classes or Code ('Unsafe Reflection')
2 parents bbbbeda + 09a8731 commit fad1622

File tree

11 files changed

+529
-2
lines changed

11 files changed

+529
-2
lines changed

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

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import java.lang.reflect.Method;
2+
import java.util.HashMap;
3+
import java.util.HashSet;
4+
import java.util.List;
5+
import java.util.Map;
6+
import javax.servlet.http.HttpServletRequest;
7+
import org.springframework.stereotype.Controller;
8+
import org.springframework.util.StringUtils;
9+
import org.springframework.web.bind.annotation.GetMapping;
10+
import org.springframework.web.bind.annotation.PathVariable;
11+
import org.springframework.web.bind.annotation.RequestBody;
12+
import org.springframework.web.bind.annotation.RequestMapping;
13+
import org.springframework.web.bind.annotation.RequestMethod;
14+
import org.springframework.web.multipart.MultipartFile;
15+
16+
@Controller
17+
public class UnsafeReflection {
18+
19+
@RequestMapping(value = {"/service/{beanIdOrClassName}/{methodName}"}, method = {RequestMethod.POST}, consumes = {"application/json"}, produces = {"application/json"})
20+
public Object bad1(@PathVariable("beanIdOrClassName") String beanIdOrClassName, @PathVariable("methodName") String methodName, @RequestBody Map<String, Object> body) throws Exception {
21+
List<Object> rawData = null;
22+
try {
23+
rawData = (List<Object>)body.get("methodInput");
24+
} catch (Exception e) {
25+
return e;
26+
}
27+
return invokeService(beanIdOrClassName, methodName, null, rawData);
28+
}
29+
30+
@GetMapping(value = "uf1")
31+
public void good1(HttpServletRequest request) throws Exception {
32+
HashSet<String> hashSet = new HashSet<>();
33+
hashSet.add("com.example.test1");
34+
hashSet.add("com.example.test2");
35+
String className = request.getParameter("className");
36+
String parameterValue = request.getParameter("parameterValue");
37+
if (!hashSet.contains(className)){
38+
throw new Exception("Class not valid: " + className);
39+
}
40+
try {
41+
Class clazz = Class.forName(className);
42+
Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //good
43+
} catch (Exception e) {
44+
e.printStackTrace();
45+
}
46+
}
47+
48+
@GetMapping(value = "uf2")
49+
public void good2(HttpServletRequest request) throws Exception {
50+
String className = request.getParameter("className");
51+
String parameterValue = request.getParameter("parameterValue");
52+
if (!"com.example.test1".equals(className)){
53+
throw new Exception("Class not valid: " + className);
54+
}
55+
try {
56+
Class clazz = Class.forName(className);
57+
Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //good
58+
} catch (Exception e) {
59+
e.printStackTrace();
60+
}
61+
}
62+
63+
private Object invokeService(String beanIdOrClassName, String methodName, MultipartFile[] files, List<Object> data) throws Exception {
64+
BeanFactory beanFactory = new BeanFactory();
65+
try {
66+
Object bean = null;
67+
Class<?> beanClass = Class.forName(beanIdOrClassName);
68+
bean = beanFactory.getBean(beanClass);
69+
byte b;
70+
int i;
71+
Method[] arrayOfMethod;
72+
for (i = (arrayOfMethod = bean.getClass().getMethods()).length, b = 0; b < i; ) {
73+
Method method = arrayOfMethod[b];
74+
if (!method.getName().equals(methodName)) {
75+
b++;
76+
continue;
77+
}
78+
Object result = method.invoke(bean, data);
79+
Map<String, Object> map = new HashMap<>();
80+
return map;
81+
}
82+
} catch (Exception e) {
83+
return e;
84+
}
85+
return null;
86+
}
87+
}
88+
89+
class BeanFactory {
90+
91+
private static HashMap<String, Object> classNameMap = new HashMap<>();
92+
93+
private static HashMap<Class<?>, Object> classMap = new HashMap<>();
94+
95+
static {
96+
classNameMap.put("xxxx", Runtime.getRuntime());
97+
classMap.put(Runtime.class, Runtime.getRuntime());
98+
}
99+
100+
public Object getBean(Class<?> clzz) {
101+
return classMap.get(clzz);
102+
}
103+
}

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Allowing users to freely choose the name of a class to instantiate could provide means to attack a vulnerable appplication.
7+
</p>
8+
</overview>
9+
10+
<recommendation>
11+
<p>
12+
Create a list of classes that are allowed to load reflectively and strictly verify the input to ensure that
13+
users can only instantiate classes or execute methods that ought to be allowed.
14+
</p>
15+
</recommendation>
16+
17+
<example>
18+
<p>
19+
The <code>bad</code> method shown below illustrate class loading with <code>Class.forName</code> without any check on the particular class being instantiated.
20+
The <code>good</code> methods illustrate some different ways to restrict which classes can be instantiated.
21+
</p>
22+
<sample src="UnsafeReflection.java" />
23+
24+
</example>
25+
26+
<references>
27+
28+
<li>
29+
Unsafe use of Reflection | OWASP:
30+
<a href="https://owasp.org/www-community/vulnerabilities/Unsafe_use_of_Reflection">Unsafe use of Reflection</a>.
31+
</li>
32+
<li>
33+
Java owasp: Classes should not be loaded dynamically:
34+
<a href="https://rules.sonarsource.com/java/tag/owasp/RSPEC-2658">Classes should not be loaded dynamically</a>.
35+
</li>
36+
</references>
37+
38+
</qhelp>

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/**
2+
* @name Use of externally-controlled input to select classes or code ('unsafe reflection')
3+
* @description Use external input with reflection function to select the class or code to
4+
* be used, which brings serious security risks.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id java/unsafe-reflection
9+
* @tags security
10+
* external/cwe/cwe-470
11+
*/
12+
13+
import java
14+
import DataFlow
15+
import UnsafeReflectionLib
16+
import semmle.code.java.dataflow.DataFlow
17+
import semmle.code.java.dataflow.FlowSources
18+
import DataFlow::PathGraph
19+
20+
private class ContainsSanitizer extends DataFlow::BarrierGuard {
21+
ContainsSanitizer() { this.(MethodAccess).getMethod().hasName("contains") }
22+
23+
override predicate checks(Expr e, boolean branch) {
24+
e = this.(MethodAccess).getArgument(0) and branch = true
25+
}
26+
}
27+
28+
private class EqualsSanitizer extends DataFlow::BarrierGuard {
29+
EqualsSanitizer() { this.(MethodAccess).getMethod().hasName("equals") }
30+
31+
override predicate checks(Expr e, boolean branch) {
32+
e = [this.(MethodAccess).getArgument(0), this.(MethodAccess).getQualifier()] and
33+
branch = true
34+
}
35+
}
36+
37+
class UnsafeReflectionConfig extends TaintTracking::Configuration {
38+
UnsafeReflectionConfig() { this = "UnsafeReflectionConfig" }
39+
40+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
41+
42+
override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeReflectionSink }
43+
44+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
45+
// Argument -> return of Class.forName, ClassLoader.loadClass
46+
exists(ReflectiveClassIdentifierMethodAccess rcimac |
47+
rcimac.getArgument(0) = pred.asExpr() and rcimac = succ.asExpr()
48+
)
49+
or
50+
// Qualifier -> return of Class.getDeclaredConstructors/Methods and similar
51+
exists(MethodAccess ma |
52+
(
53+
ma instanceof ReflectiveConstructorsAccess or
54+
ma instanceof ReflectiveMethodsAccess
55+
) and
56+
ma.getQualifier() = pred.asExpr() and
57+
ma = succ.asExpr()
58+
)
59+
or
60+
// Qualifier -> return of Object.getClass
61+
exists(MethodAccess ma |
62+
ma.getMethod().hasName("getClass") and
63+
ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Object") and
64+
ma.getQualifier() = pred.asExpr() and
65+
ma = succ.asExpr()
66+
)
67+
or
68+
// Argument -> return of methods that look like Class.forName
69+
looksLikeResolveClassStep(pred, succ)
70+
or
71+
// Argument -> return of methods that look like `Object getInstance(Class c)`
72+
looksLikeInstantiateClassStep(pred, succ)
73+
or
74+
// Qualifier -> return of Constructor.newInstance, Class.newInstance
75+
exists(NewInstance ni |
76+
ni.getQualifier() = pred.asExpr() and
77+
ni = succ.asExpr()
78+
)
79+
}
80+
81+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
82+
guard instanceof ContainsSanitizer or guard instanceof EqualsSanitizer
83+
}
84+
}
85+
86+
private Expr getAMethodArgument(MethodAccess reflectiveCall) {
87+
result = reflectiveCall.(NewInstance).getAnArgument()
88+
or
89+
result = reflectiveCall.(MethodInvokeCall).getAnArgument()
90+
}
91+
92+
from
93+
DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeReflectionConfig conf,
94+
MethodAccess reflectiveCall
95+
where
96+
conf.hasFlowPath(source, sink) and
97+
sink.getNode().asExpr() = reflectiveCall.getQualifier() and
98+
conf.hasFlowToExpr(getAMethodArgument(reflectiveCall))
99+
select sink.getNode(), source, sink, "Unsafe reflection of $@.", source.getNode(), "user input"

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import java
2+
import DataFlow
3+
import semmle.code.java.Reflection
4+
import semmle.code.java.dataflow.FlowSources
5+
6+
/**
7+
* A call to `java.lang.reflect.Method.invoke`.
8+
*/
9+
class MethodInvokeCall extends MethodAccess {
10+
MethodInvokeCall() { this.getMethod().hasQualifiedName("java.lang.reflect", "Method", "invoke") }
11+
}
12+
13+
/**
14+
* Unsafe reflection sink (the qualifier or method arguments to `Constructor.newInstance(...)` or `Method.invoke(...)`)
15+
*/
16+
class UnsafeReflectionSink extends DataFlow::ExprNode {
17+
UnsafeReflectionSink() {
18+
exists(MethodAccess ma |
19+
(
20+
ma.getMethod().hasQualifiedName("java.lang.reflect", "Constructor<>", "newInstance") or
21+
ma instanceof MethodInvokeCall
22+
) and
23+
this.asExpr() = [ma.getQualifier(), ma.getAnArgument()]
24+
)
25+
}
26+
}
27+
28+
/**
29+
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
30+
* A method probably resolves a class if it takes a string, returns a Class
31+
* and its name contains "resolve", "load", etc.
32+
*/
33+
predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
34+
exists(MethodAccess ma, Method m, int i, Expr arg |
35+
m = ma.getMethod() and arg = ma.getArgument(i)
36+
|
37+
m.getReturnType() instanceof TypeClass and
38+
m.getName().toLowerCase().regexpMatch("resolve|load|class|type") and
39+
arg.getType() instanceof TypeString and
40+
arg = fromNode.asExpr() and
41+
ma = toNode.asExpr()
42+
)
43+
}
44+
45+
/**
46+
* Holds if `fromNode` to `toNode` is a dataflow step that looks like instantiating a class.
47+
* A method probably instantiates a class if it is external, takes a Class, returns an Object
48+
* and its name contains "instantiate" or similar terms.
49+
*/
50+
predicate looksLikeInstantiateClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
51+
exists(MethodAccess ma, Method m, int i, Expr arg |
52+
m = ma.getMethod() and arg = ma.getArgument(i)
53+
|
54+
m.getReturnType() instanceof TypeObject and
55+
m.getName().toLowerCase().regexpMatch("instantiate|instance|create|make|getbean") and
56+
arg.getType() instanceof TypeClass and
57+
arg = fromNode.asExpr() and
58+
ma = toNode.asExpr()
59+
)
60+
}

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

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ private XMLElement elementReferencingType(RefType rt) {
4747
}
4848

4949
abstract private class ReflectiveClassIdentifier extends Expr {
50+
/**
51+
* Gets the type of a class identified by this expression.
52+
*/
5053
abstract RefType getReflectivelyIdentifiedClass();
5154
}
5255

@@ -59,7 +62,7 @@ private class ReflectiveClassIdentifierLiteral extends ReflectiveClassIdentifier
5962
/**
6063
* A call to a Java standard library method which constructs or returns a `Class<T>` from a `String`.
6164
*/
62-
library class ReflectiveClassIdentifierMethodAccess extends ReflectiveClassIdentifier, MethodAccess {
65+
class ReflectiveClassIdentifierMethodAccess extends ReflectiveClassIdentifier, MethodAccess {
6366
ReflectiveClassIdentifierMethodAccess() {
6467
// A call to `Class.forName(...)`, from which we can infer `T` in the returned type `Class<T>`.
6568
getCallee().getDeclaringType() instanceof TypeClass and getCallee().hasName("forName")
@@ -314,6 +317,26 @@ class ClassMethodAccess extends MethodAccess {
314317
}
315318
}
316319

320+
/**
321+
* A call to `Class.getConstructors(..)` or `Class.getDeclaredConstructors(..)`.
322+
*/
323+
class ReflectiveConstructorsAccess extends ClassMethodAccess {
324+
ReflectiveConstructorsAccess() {
325+
this.getCallee().hasName("getConstructors") or
326+
this.getCallee().hasName("getDeclaredConstructors")
327+
}
328+
}
329+
330+
/**
331+
* A call to `Class.getMethods(..)` or `Class.getDeclaredMethods(..)`.
332+
*/
333+
class ReflectiveMethodsAccess extends ClassMethodAccess {
334+
ReflectiveMethodsAccess() {
335+
this.getCallee().hasName("getMethods") or
336+
this.getCallee().hasName("getDeclaredMethods")
337+
}
338+
}
339+
317340
/**
318341
* A call to `Class.getMethod(..)` or `Class.getDeclaredMethod(..)`.
319342
*/

0 commit comments

Comments
 (0)