hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 19:26:02 +02:00
Code Issues Packages Projects Releases Wiki Activity

Better taint propagation in UnsafeTypeConfig

Browse Source
This commit is contained in:
Artem Smotrakov
2021-06-01 22:07:27 +02:00
parent 476843a278
commit c98f1a479e
3 changed files with 57 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

47
java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll
View File

@@ -10,6 +10,7 @@ import semmle.code.java.frameworks.YamlBeans
import semmle.code.java.frameworks.HessianBurlap
import semmle.code.java.frameworks.Castor
import semmle.code.java.frameworks.apache.Lang
import semmle.code.java.Reflection
class ObjectInputStreamReadObjectMethod extends Method {
ObjectInputStreamReadObjectMethod() {
@@ -246,21 +247,47 @@ private class UnsafeTypeConfig extends TaintTracking2::Configuration {
}
/**
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
*
* Note any method that returns a `Class` or similar is assumed to propagate taint from all
* of its arguments, so methods that accept user-controlled data but sanitize it or use it for some
* completely different purpose before returning a `Class` could result in false positives.
* Holds if `fromNode` to `toNode` is a dataflow step that resolves a class
* or at least looks like resolving a class.
*/
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
exists(MethodAccess ma, RefType returnType | returnType = ma.getMethod().getReturnType() |
returnType instanceof JacksonTypeDescriptorType and
ma.getAnArgument() = fromNode.asExpr() and
ma = toNode.asExpr()
)
resolveClassStep(fromNode, toNode) or
looksLikeResolveClassStep(fromNode, toNode)
}
}
/**
* Holds if `fromNode` to `toNode` is a dataflow step that resolves a class.
*/
private predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
exists(ReflectiveClassIdentifierMethodAccess ma |
ma.getArgument(0) = fromNode.asExpr() and
ma = toNode.asExpr()
)
}
/**
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
* A method probably resolves a class if it is external, takes a string, returns a type descriptor
* and its name contains "resolve", "load", etc.
*
* Any method call that satisfies the rule above is assumed to propagate taint from its string arguments,
* so methods that accept user-controlled data but sanitize it or use it for some
* completely different purpose before returning a type descriptor could result in false positives.
*/
private predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
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.fromSource() and
arg.getType() instanceof TypeString and
arg = fromNode.asExpr() and
ma = toNode.asExpr()
)
}
/**
* Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.
*

15
java/ql/test/query-tests/security/CWE-502/JacksonTest.java
View File

@@ -171,6 +171,21 @@ class UnsafeCatDeserialization {
mapper.readValue(data, clazz);
});
}
// BAD: an attacker can control both data and type of deserialized object
private static void testUnsafeDeserializationWithUnsafeClassAndCustomTypeResolver() throws Exception {
JacksonTest.withSocket(input -> {
String[] parts = input.split(";");
String data = parts[0];
String type = parts[1];
ObjectMapper mapper = new ObjectMapper();
mapper.readValue(data, resolveTypeImpl(type));
});
}
private static Class resolveTypeImpl(String type) throws Exception {
return Class.forName(type);
}
}
class SaferCatDeserialization {

5
java/ql/test/query-tests/security/CWE-502/UnsafeDeserialization.expected
View File

@@ -89,6 +89,7 @@ edges
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:147:32:147:37 | string : String |
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:156:32:156:37 | string : String |
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:165:32:165:36 | input : String |
| JacksonTest.java:21:28:21:35 | jexlExpr : String | JacksonTest.java:177:32:177:36 | input : String |
| JacksonTest.java:73:32:73:37 | string : String | JacksonTest.java:75:30:75:35 | string |
| JacksonTest.java:82:32:82:37 | string : String | JacksonTest.java:84:30:84:35 | string |
| JacksonTest.java:91:32:91:37 | string : String | JacksonTest.java:93:30:93:35 | string |
@@ -96,6 +97,7 @@ edges
| JacksonTest.java:147:32:147:37 | string : String | JacksonTest.java:150:31:150:68 | createParser(...) |
| JacksonTest.java:156:32:156:37 | string : String | JacksonTest.java:159:32:159:54 | readTree(...) |
| JacksonTest.java:165:32:165:36 | input : String | JacksonTest.java:171:30:171:33 | data |
| JacksonTest.java:177:32:177:36 | input : String | JacksonTest.java:182:30:182:33 | data |
| TestMessageBodyReader.java:20:55:20:78 | entityStream : InputStream | TestMessageBodyReader.java:22:18:22:52 | new ObjectInputStream(...) |
| TestMessageBodyReader.java:20:55:20:78 | entityStream : InputStream | TestMessageBodyReader.java:22:40:22:51 | entityStream : InputStream |
| TestMessageBodyReader.java:22:40:22:51 | entityStream : InputStream | TestMessageBodyReader.java:22:18:22:52 | new ObjectInputStream(...) |
@@ -209,6 +211,8 @@ nodes
| JacksonTest.java:159:32:159:54 | readTree(...) | semmle.label | readTree(...) |
| JacksonTest.java:165:32:165:36 | input : String | semmle.label | input : String |
| JacksonTest.java:171:30:171:33 | data | semmle.label | data |
| JacksonTest.java:177:32:177:36 | input : String | semmle.label | input : String |
| JacksonTest.java:182:30:182:33 | data | semmle.label | data |
| TestMessageBodyReader.java:20:55:20:78 | entityStream : InputStream | semmle.label | entityStream : InputStream |
| TestMessageBodyReader.java:22:18:22:52 | new ObjectInputStream(...) | semmle.label | new ObjectInputStream(...) |
| TestMessageBodyReader.java:22:40:22:51 | entityStream : InputStream | semmle.label | entityStream : InputStream |
@@ -266,4 +270,5 @@ nodes
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |