Skip to content

Commit c98f1a4

Browse files
artem-smotrakovartem-smotrakov
committed
Better taint propagation in UnsafeTypeConfig
1 parent 476843a commit c98f1a4

File tree

3 files changed

+57
-10
lines changed

3 files changed

+57
-10
lines changed

‎java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import semmle.code.java.frameworks.YamlBeans
1010
import semmle.code.java.frameworks.HessianBurlap
1111
import semmle.code.java.frameworks.Castor
1212
import semmle.code.java.frameworks.apache.Lang
13+
import semmle.code.java.Reflection
1314

1415
class ObjectInputStreamReadObjectMethod extends Method {
1516
ObjectInputStreamReadObjectMethod() {
@@ -246,21 +247,47 @@ private class UnsafeTypeConfig extends TaintTracking2::Configuration {
246247
}
247248

248249
/**
249-
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
250-
*
251-
* Note any method that returns a `Class` or similar is assumed to propagate taint from all
252-
* of its arguments, so methods that accept user-controlled data but sanitize it or use it for some
253-
* completely different purpose before returning a `Class` could result in false positives.
250+
* Holds if `fromNode` to `toNode` is a dataflow step that resolves a class
251+
* or at least looks like resolving a class.
254252
*/
255253
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
256-
exists(MethodAccess ma, RefType returnType | returnType = ma.getMethod().getReturnType() |
257-
returnType instanceof JacksonTypeDescriptorType and
258-
ma.getAnArgument() = fromNode.asExpr() and
259-
ma = toNode.asExpr()
260-
)
254+
resolveClassStep(fromNode, toNode) or
255+
looksLikeResolveClassStep(fromNode, toNode)
261256
}
262257
}
263258

259+
/**
260+
* Holds if `fromNode` to `toNode` is a dataflow step that resolves a class.
261+
*/
262+
private predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
263+
exists(ReflectiveClassIdentifierMethodAccess ma |
264+
ma.getArgument(0) = fromNode.asExpr() and
265+
ma = toNode.asExpr()
266+
)
267+
}
268+
269+
/**
270+
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
271+
* A method probably resolves a class if it is external, takes a string, returns a type descriptor
272+
* and its name contains "resolve", "load", etc.
273+
*
274+
* Any method call that satisfies the rule above is assumed to propagate taint from its string arguments,
275+
* so methods that accept user-controlled data but sanitize it or use it for some
276+
* completely different purpose before returning a type descriptor could result in false positives.
277+
*/
278+
private predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
279+
exists(MethodAccess ma, Method m, int i, Expr arg |
280+
m = ma.getMethod() and arg = ma.getArgument(i)
281+
|
282+
m.getReturnType() instanceof JacksonTypeDescriptorType and
283+
m.getName().toLowerCase().regexpMatch("resolve|load|class|type") and
284+
m.fromSource() and
285+
arg.getType() instanceof TypeString and
286+
arg = fromNode.asExpr() and
287+
ma = toNode.asExpr()
288+
)
289+
}
290+
264291
/**
265292
* Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.
266293
*

‎java/ql/test/query-tests/security/CWE-502/JacksonTest.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,21 @@ private static void testUnsafeDeserializationWithUnsafeClass() throws Exception
171171
mapper.readValue(data, clazz);
172172
});
173173
}
174+
175+
// BAD: an attacker can control both data and type of deserialized object
176+
private static void testUnsafeDeserializationWithUnsafeClassAndCustomTypeResolver() throws Exception {
177+
JacksonTest.withSocket(input -> {
178+
String[] parts = input.split(";");
179+
String data = parts[0];
180+
String type = parts[1];
181+
ObjectMapper mapper = new ObjectMapper();
182+
mapper.readValue(data, resolveTypeImpl(type));
183+
});
184+
}
185+
186+
private static Class resolveTypeImpl(String type) throws Exception {
187+
return Class.forName(type);
188+
}
174189
}
175190

176191
class SaferCatDeserialization {

‎java/ql/test/query-tests/security/CWE-502/UnsafeDeserialization.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,13 +89,15 @@ edges
8989
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:147:32:147:37 | string : String |
9090
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:156:32:156:37 | string : String |
9191
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:165:32:165:36 | input : String |
92+
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:177:32:177:36 | input : String |
9293
| JacksonTest.java:73:32:73:37 | string : String | JacksonTest.java:75:30:75:35 | string |
9394
| JacksonTest.java:82:32:82:37 | string : String | JacksonTest.java:84:30:84:35 | string |
9495
| JacksonTest.java:91:32:91:37 | string : String | JacksonTest.java:93:30:93:35 | string |
9596
| JacksonTest.java:138:32:138:37 | string : String | JacksonTest.java:141:30:141:35 | string |
9697
| JacksonTest.java:147:32:147:37 | string : String | JacksonTest.java:150:31:150:68 | createParser(...) |
9798
| JacksonTest.java:156:32:156:37 | string : String | JacksonTest.java:159:32:159:54 | readTree(...) |
9899
| JacksonTest.java:165:32:165:36 | input : String | JacksonTest.java:171:30:171:33 | data |
100+
| JacksonTest.java:177:32:177:36 | input : String | JacksonTest.java:182:30:182:33 | data |
99101
| TestMessageBodyReader.java:20:55:20:78 | entityStream : InputStream | TestMessageBodyReader.java:22:18:22:52 | new ObjectInputStream(...) |
100102
| TestMessageBodyReader.java:20:55:20:78 | entityStream : InputStream | TestMessageBodyReader.java:22:40:22:51 | entityStream : InputStream |
101103
| TestMessageBodyReader.java:22:40:22:51 | entityStream : InputStream | TestMessageBodyReader.java:22:18:22:52 | new ObjectInputStream(...) |
@@ -209,6 +211,8 @@ nodes
209211
| JacksonTest.java:159:32:159:54 | readTree(...) | semmle.label | readTree(...) |
210212
| JacksonTest.java:165:32:165:36 | input : String | semmle.label | input : String |
211213
| JacksonTest.java:171:30:171:33 | data | semmle.label | data |
214+
| JacksonTest.java:177:32:177:36 | input : String | semmle.label | input : String |
215+
| JacksonTest.java:182:30:182:33 | data | semmle.label | data |
212216
| TestMessageBodyReader.java:20:55:20:78 | entityStream : InputStream | semmle.label | entityStream : InputStream |
213217
| TestMessageBodyReader.java:22:18:22:52 | new ObjectInputStream(...) | semmle.label | new ObjectInputStream(...) |
214218
| TestMessageBodyReader.java:22:40:22:51 | entityStream : InputStream | semmle.label | entityStream : InputStream |
@@ -266,4 +270,5 @@ nodes
266270
| JacksonTest.java:150:13:150:80 | readValues(...) | JacksonTest.java:19:25:19:47 | getInputStream(...) : InputStream | JacksonTest.java:150:31:150:68 | createParser(...) | Unsafe deserialization of $@. | JacksonTest.java:19:25:19:47 | getInputStream(...) | user input |
267271
| JacksonTest.java:159:13:159:66 | treeToValue(...) | JacksonTest.java:19:25:19:47 | getInputStream(...) : InputStream | JacksonTest.java:159:32:159:54 | readTree(...) | Unsafe deserialization of $@. | JacksonTest.java:19:25:19:47 | getInputStream(...) | user input |
268272
| JacksonTest.java:171:13:171:41 | readValue(...) | JacksonTest.java:19:25:19:47 | getInputStream(...) : InputStream | JacksonTest.java:171:30:171:33 | data | Unsafe deserialization of $@. | JacksonTest.java:19:25:19:47 | getInputStream(...) | user input |
273+
| JacksonTest.java:182:13:182:57 | readValue(...) | JacksonTest.java:19:25:19:47 | getInputStream(...) : InputStream | JacksonTest.java:182:30:182:33 | data | Unsafe deserialization of $@. | JacksonTest.java:19:25:19:47 | getInputStream(...) | user input |
269274
| TestMessageBodyReader.java:22:18:22:65 | readObject(...) | TestMessageBodyReader.java:20:55:20:78 | entityStream : InputStream | TestMessageBodyReader.java:22:18:22:52 | new ObjectInputStream(...) | Unsafe deserialization of $@. | TestMessageBodyReader.java:20:55:20:78 | entityStream | user input |

0 commit comments

Comments
 (0)