Skip to content

Commit 291ca38

Browse files
haby0haby0
committed
Modify according to suggestions
1 parent 2a50cf8 commit 291ca38

File tree

5 files changed

+20
-69
lines changed

5 files changed

+20
-69
lines changed

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

Lines changed: 3 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -16,34 +16,8 @@
1616
@Controller
1717
public class UnsafeReflection {
1818

19-
@GetMapping(value = "uf1")
20-
public void bad1(HttpServletRequest request) {
21-
String className = request.getParameter("className");
22-
String parameterValue = request.getParameter("parameterValue");
23-
try {
24-
Class clazz = Class.forName(className);
25-
Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //bad
26-
} catch (Exception e) {
27-
e.printStackTrace();
28-
}
29-
}
30-
31-
@GetMapping(value = "uf2")
32-
public void bad2(HttpServletRequest request) {
33-
String className = request.getParameter("className");
34-
String parameterValue = request.getParameter("parameterValue");
35-
try {
36-
ClassLoader classLoader = ClassLoader.getSystemClassLoader();
37-
Class clazz = classLoader.loadClass(className);
38-
Object object = clazz.newInstance();
39-
clazz.getDeclaredMethods()[0].invoke(object, parameterValue); //bad
40-
} catch (Exception e) {
41-
e.printStackTrace();
42-
}
43-
}
44-
4519
@RequestMapping(value = {"/service/{beanIdOrClassName}/{methodName}"}, method = {RequestMethod.POST}, consumes = {"application/json"}, produces = {"application/json"})
46-
public Object bad3(@PathVariable("beanIdOrClassName") String beanIdOrClassName, @PathVariable("methodName") String methodName, @RequestBody Map<String, Object> body) throws Exception {
20+
public Object bad1(@PathVariable("beanIdOrClassName") String beanIdOrClassName, @PathVariable("methodName") String methodName, @RequestBody Map<String, Object> body) throws Exception {
4721
List<Object> rawData = null;
4822
try {
4923
rawData = (List<Object>)body.get("methodInput");
@@ -53,7 +27,7 @@ public Object bad3(@PathVariable("beanIdOrClassName") String beanIdOrClassName,
5327
return invokeService(beanIdOrClassName, methodName, null, rawData);
5428
}
5529

56-
@GetMapping(value = "uf3")
30+
@GetMapping(value = "uf1")
5731
public void good1(HttpServletRequest request) throws Exception {
5832
HashSet<String> hashSet = new HashSet<>();
5933
hashSet.add("com.example.test1");
@@ -71,7 +45,7 @@ public void good1(HttpServletRequest request) throws Exception {
7145
}
7246
}
7347

74-
@GetMapping(value = "uf4")
48+
@GetMapping(value = "uf2")
7549
public void good2(HttpServletRequest request) throws Exception {
7650
String className = request.getParameter("className");
7751
String parameterValue = request.getParameter("parameterValue");
@@ -86,21 +60,6 @@ public void good2(HttpServletRequest request) throws Exception {
8660
}
8761
}
8862

89-
@GetMapping(value = "uf5")
90-
public void good3(HttpServletRequest request) throws Exception {
91-
String className = request.getParameter("className");
92-
String parameterValue = request.getParameter("parameterValue");
93-
if (!className.equals("com.example.test1")){ //good
94-
throw new Exception("Class not valid: " + className);
95-
}
96-
try {
97-
Class clazz = Class.forName(className);
98-
Object object = clazz.getDeclaredConstructors()[0].newInstance(parameterValue); //good
99-
} catch (Exception e) {
100-
e.printStackTrace();
101-
}
102-
}
103-
10463
private Object invokeService(String beanIdOrClassName, String methodName, MultipartFile[] files, List<Object> data) throws Exception {
10564
BeanFactory beanFactory = new BeanFactory();
10665
try {

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

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,26 +3,21 @@
33

44
<overview>
55
<p>
6-
Allowing users to freely choose to call the constructor or method of the class through reflection. This means that
7-
malicious users can carefully construct requests to create instances of any class or call methods of any class.
6+
Allowing users to freely choose the name of a class to instantiate could provide means to attack a vulnerable appplication.
87
</p>
98
</overview>
109

1110
<recommendation>
1211
<p>
13-
Create a list of classes that are allowed to load reflectively and strictly verify the input content to ensure that
14-
users can only execute classes or codes that are running.
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.
1514
</p>
1615
</recommendation>
1716

1817
<example>
1918
<p>
20-
The following examples are good examples and bad examples. When using <code>Class.forName(...)</code> or
21-
<code>ClassLoader.loadClass(...)</code> to get the class, then create the class object or call the method of the class.
22-
When the user is not authenticated Input, it is easy to cause security risks, for example: <code>bad1</code>/<code>bad2
23-
</code>/<code>bad3</code>. When the user input is verified by <code>contains</code> or <code>equals</code> and then
24-
the reflection operation is performed, the system security can be well controlled, for example:
25-
<code >good1</code>/<code>good2</code>/<code>good3</code>.
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.
2621
</p>
2722
<sample src="UnsafeReflection.java" />
2823

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,12 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration {
5555
ma = succ.asExpr()
5656
)
5757
or
58-
exists(MethodAccess ma |
59-
ma.getMethod().getReturnType() instanceof TypeObject and
60-
ma.getMethod().getAParamType() instanceof TypeClass and
61-
ma.getAnArgument() = pred.asExpr() and
58+
exists(MethodAccess ma, Method m, int i, Expr arg |
59+
m = ma.getMethod() and arg = ma.getArgument(i)
60+
|
61+
m.getReturnType() instanceof TypeObject and
62+
arg.getType() instanceof TypeClass and
63+
arg = pred.asExpr() and
6264
ma = succ.asExpr()
6365
)
6466
or
@@ -72,8 +74,7 @@ class UnsafeReflectionConfig extends TaintTracking::Configuration {
7274
exists(MethodAccess ma, Method m, int i, Expr arg |
7375
m = ma.getMethod() and arg = ma.getArgument(i)
7476
|
75-
m.getReturnType() instanceof JacksonTypeDescriptorType and
76-
m.getName().toLowerCase().regexpMatch("resolve|load|class|type") and
77+
m.getReturnType() instanceof TypeClass and
7778
arg.getType() instanceof TypeString and
7879
arg = pred.asExpr() and
7980
ma = succ.asExpr()

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,6 @@ import semmle.code.java.dataflow.FlowSources
77
import semmle.code.java.dataflow.TaintTracking
88
import semmle.code.java.dataflow.TaintTracking2
99

10-
/** Type descriptors in Jackson libraries. */
11-
class JacksonTypeDescriptorType extends RefType {
12-
JacksonTypeDescriptorType() {
13-
this instanceof TypeClass or
14-
hasQualifiedName("com.fasterxml.jackson.databind", "JavaType") or
15-
hasQualifiedName("com.fasterxml.jackson.core.type", "TypeReference")
16-
}
17-
}
18-
1910
/**
2011
* A call to a Java standard library method which constructs or returns a `Class<T>` from a `String`.
2112
* e.g `Class.forName(...)` or `ClassLoader.loadClass(...)`

‎java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@ edges
77
| UnsafeReflection.java:39:13:39:38 | getDeclaredMethods(...) : Method[] | UnsafeReflection.java:39:13:39:41 | ...[...] |
88
| UnsafeReflection.java:46:24:46:82 | beanIdOrClassName : String | UnsafeReflection.java:53:30:53:46 | beanIdOrClassName : String |
99
| UnsafeReflection.java:53:30:53:46 | beanIdOrClassName : String | UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String |
10+
| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:119:44:119:52 | beanClass : Class |
1011
| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | UnsafeReflection.java:132:33:132:38 | method |
12+
| UnsafeReflection.java:119:24:119:53 | getBean(...) : Object | UnsafeReflection.java:132:33:132:38 | method |
13+
| UnsafeReflection.java:119:44:119:52 | beanClass : Class | UnsafeReflection.java:119:24:119:53 | getBean(...) : Object |
1114
nodes
1215
| UnsafeReflection.java:21:28:21:60 | getParameter(...) : String | semmle.label | getParameter(...) : String |
1316
| UnsafeReflection.java:25:29:25:59 | getDeclaredConstructors(...) : Constructor[] | semmle.label | getDeclaredConstructors(...) : Constructor[] |
@@ -18,6 +21,8 @@ nodes
1821
| UnsafeReflection.java:46:24:46:82 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String |
1922
| UnsafeReflection.java:53:30:53:46 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String |
2023
| UnsafeReflection.java:104:34:104:57 | beanIdOrClassName : String | semmle.label | beanIdOrClassName : String |
24+
| UnsafeReflection.java:119:24:119:53 | getBean(...) : Object | semmle.label | getBean(...) : Object |
25+
| UnsafeReflection.java:119:44:119:52 | beanClass : Class | semmle.label | beanClass : Class |
2126
| UnsafeReflection.java:132:33:132:38 | method | semmle.label | method |
2227
#select
2328
| 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 |

0 commit comments

Comments
 (0)