Skip to content

Commit 035f7ac

Browse files
artem-smotrakovartem-smotrakov
committed
Refactored libs for unsafe deserialization
1 parent e025307 commit 035f7ac

3 files changed

Lines changed: 99 additions & 91 deletions

File tree

java/ql/src/semmle/code/java/frameworks/JacksonQuery.qll renamed to java/ql/src/semmle/code/java/frameworks/Jackson.qll

Lines changed: 9 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ private class ObjectMapper extends RefType {
1313
}
1414
}
1515

16-
private class MapperBuilder extends RefType {
16+
/** A builder for building Jackson's `JsonMapper`. */
17+
class MapperBuilder extends RefType {
1718
MapperBuilder() {
1819
hasQualifiedName("com.fasterxml.jackson.databind.cfg", "MapperBuilder<JsonMapper,Builder>")
1920
}
@@ -27,7 +28,8 @@ private class JsonParser extends RefType {
2728
JsonParser() { hasQualifiedName("com.fasterxml.jackson.core", "JsonParser") }
2829
}
2930

30-
private class JacksonTypeDescriptorType extends RefType {
31+
/** Type descriptors in Jackson libraries. */
32+
class JacksonTypeDescriptorType extends RefType {
3133
JacksonTypeDescriptorType() {
3234
this instanceof TypeClass or
3335
hasQualifiedName("com.fasterxml.jackson.databind", "JavaType") or
@@ -44,15 +46,15 @@ class ObjectMapperReadMethod extends Method {
4446
}
4547

4648
/** A call that enables the default typing in `ObjectMapper`. */
47-
private class EnableJacksonDefaultTyping extends MethodAccess {
49+
class EnableJacksonDefaultTyping extends MethodAccess {
4850
EnableJacksonDefaultTyping() {
4951
this.getMethod().getDeclaringType() instanceof ObjectMapper and
5052
this.getMethod().hasName("enableDefaultTyping")
5153
}
5254
}
5355

5456
/** A qualifier of a call to one of the methods in `ObjectMapper` that deserialize data. */
55-
private class ObjectMapperReadQualifier extends DataFlow::ExprNode {
57+
class ObjectMapperReadQualifier extends DataFlow::ExprNode {
5658
ObjectMapperReadQualifier() {
5759
exists(MethodAccess ma | ma.getQualifier() = this.asExpr() |
5860
ma.getMethod() instanceof ObjectMapperReadMethod
@@ -61,7 +63,7 @@ private class ObjectMapperReadQualifier extends DataFlow::ExprNode {
6163
}
6264

6365
/** A source that sets a type validator. */
64-
private class SetPolymorphicTypeValidatorSource extends DataFlow::ExprNode {
66+
class SetPolymorphicTypeValidatorSource extends DataFlow::ExprNode {
6567
SetPolymorphicTypeValidatorSource() {
6668
exists(MethodAccess ma, Method m | m = ma.getMethod() |
6769
(
@@ -76,82 +78,8 @@ private class SetPolymorphicTypeValidatorSource extends DataFlow::ExprNode {
7678
}
7779
}
7880

79-
/**
80-
* Tracks flow from a remote source to a type descriptor (e.g. a `java.lang.Class` instance)
81-
* passed to a Jackson deserialization method.
82-
*
83-
* If this is user-controlled, arbitrary code could be executed while instantiating the user-specified type.
84-
*/
85-
class UnsafeTypeConfig extends TaintTracking2::Configuration {
86-
UnsafeTypeConfig() { this = "UnsafeTypeConfig" }
87-
88-
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
89-
90-
override predicate isSink(DataFlow::Node sink) {
91-
exists(MethodAccess ma, int i, Expr arg | i > 0 and ma.getArgument(i) = arg |
92-
ma.getMethod() instanceof ObjectMapperReadMethod and
93-
arg.getType() instanceof JacksonTypeDescriptorType and
94-
arg = sink.asExpr()
95-
)
96-
}
97-
98-
/**
99-
* Holds if `fromNode` to `toNode` is a dataflow step that resolves a class
100-
* or at least looks like resolving a class.
101-
*/
102-
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
103-
resolveClassStep(fromNode, toNode) or
104-
looksLikeResolveClassStep(fromNode, toNode)
105-
}
106-
}
107-
108-
/**
109-
* Tracks flow from `enableDefaultTyping` calls to a subsequent Jackson deserialization method call.
110-
*/
111-
class EnableJacksonDefaultTypingConfig extends DataFlow2::Configuration {
112-
EnableJacksonDefaultTypingConfig() { this = "EnableJacksonDefaultTypingConfig" }
113-
114-
override predicate isSource(DataFlow::Node src) {
115-
any(EnableJacksonDefaultTyping ma).getQualifier() = src.asExpr()
116-
}
117-
118-
override predicate isSink(DataFlow::Node sink) { sink instanceof ObjectMapperReadQualifier }
119-
}
120-
121-
/**
122-
* Tracks flow from calls which set a type validator to a subsequent Jackson deserialization method call,
123-
* including across builder method calls.
124-
*
125-
* Such a Jackson deserialization method call is safe because validation will likely prevent instantiating unexpected types.
126-
*/
127-
class SafeObjectMapperConfig extends DataFlow2::Configuration {
128-
SafeObjectMapperConfig() { this = "SafeObjectMapperConfig" }
129-
130-
override predicate isSource(DataFlow::Node src) {
131-
src instanceof SetPolymorphicTypeValidatorSource
132-
}
133-
134-
override predicate isSink(DataFlow::Node sink) { sink instanceof ObjectMapperReadQualifier }
135-
136-
/**
137-
* Holds if `fromNode` to `toNode` is a dataflow step
138-
* that configures or creates an `ObjectMapper` via a builder.
139-
*/
140-
override predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
141-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
142-
m.getDeclaringType() instanceof MapperBuilder and
143-
m.getReturnType()
144-
.(RefType)
145-
.hasQualifiedName("com.fasterxml.jackson.databind.json",
146-
["JsonMapper$Builder", "JsonMapper"]) and
147-
fromNode.asExpr() = ma.getQualifier() and
148-
ma = toNode.asExpr()
149-
)
150-
}
151-
}
152-
15381
/** Holds if `fromNode` to `toNode` is a dataflow step that resolves a class. s */
154-
private predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
82+
predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
15583
exists(ReflectiveClassIdentifierMethodAccess ma |
15684
ma.getArgument(0) = fromNode.asExpr() and
15785
ma = toNode.asExpr()
@@ -236,7 +164,7 @@ predicate hasArgumentWithUnsafeJacksonAnnotation(MethodAccess call) {
236164
* so methods that accept user-controlled data but sanitize it or use it for some
237165
* completely different purpose before returning a type descriptor could result in false positives.
238166
*/
239-
private predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
167+
predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
240168
exists(MethodAccess ma, Method m, int i, Expr arg |
241169
m = ma.getMethod() and arg = ma.getArgument(i)
242170
|

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

Lines changed: 89 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
/**
2+
* Provides classes and predicates for deserialization vulnerabilities.
3+
*/
4+
15
import semmle.code.java.dataflow.FlowSources
26
import semmle.code.java.frameworks.Kryo
37
import semmle.code.java.frameworks.XStream
@@ -8,25 +12,25 @@ import semmle.code.java.frameworks.JsonIo
812
import semmle.code.java.frameworks.YamlBeans
913
import semmle.code.java.frameworks.HessianBurlap
1014
import semmle.code.java.frameworks.Castor
11-
import semmle.code.java.frameworks.JacksonQuery
15+
import semmle.code.java.frameworks.Jackson
1216
import semmle.code.java.frameworks.apache.Lang
1317
import semmle.code.java.Reflection
1418

15-
class ObjectInputStreamReadObjectMethod extends Method {
19+
private class ObjectInputStreamReadObjectMethod extends Method {
1620
ObjectInputStreamReadObjectMethod() {
1721
this.getDeclaringType().getASourceSupertype*().hasQualifiedName("java.io", "ObjectInputStream") and
1822
(this.hasName("readObject") or this.hasName("readUnshared"))
1923
}
2024
}
2125

22-
class XMLDecoderReadObjectMethod extends Method {
26+
private class XMLDecoderReadObjectMethod extends Method {
2327
XMLDecoderReadObjectMethod() {
2428
this.getDeclaringType().hasQualifiedName("java.beans", "XMLDecoder") and
2529
this.hasName("readObject")
2630
}
2731
}
2832

29-
class SafeXStream extends DataFlow2::Configuration {
33+
private class SafeXStream extends DataFlow2::Configuration {
3034
SafeXStream() { this = "UnsafeDeserialization::SafeXStream" }
3135

3236
override predicate isSource(DataFlow::Node src) {
@@ -42,7 +46,7 @@ class SafeXStream extends DataFlow2::Configuration {
4246
}
4347
}
4448

45-
class SafeKryo extends DataFlow2::Configuration {
49+
private class SafeKryo extends DataFlow2::Configuration {
4650
SafeKryo() { this = "UnsafeDeserialization::SafeKryo" }
4751

4852
override predicate isSource(DataFlow::Node src) {
@@ -117,7 +121,7 @@ class SafeKryo extends DataFlow2::Configuration {
117121
}
118122
}
119123

120-
predicate unsafeDeserialization(MethodAccess ma, Expr sink) {
124+
private predicate unsafeDeserialization(MethodAccess ma, Expr sink) {
121125
exists(Method m | m = ma.getMethod() |
122126
m instanceof ObjectInputStreamReadObjectMethod and
123127
sink = ma.getQualifier() and
@@ -179,12 +183,16 @@ predicate unsafeDeserialization(MethodAccess ma, Expr sink) {
179183
)
180184
}
181185

182-
class UnsafeDeserializationSink extends DataFlow::ExprNode {
186+
private class UnsafeDeserializationSink extends DataFlow::ExprNode {
183187
UnsafeDeserializationSink() { unsafeDeserialization(_, this.getExpr()) }
184188

189+
/** Get a call that triggers unsafe deserialization. */
185190
MethodAccess getMethodAccess() { unsafeDeserialization(result, this.getExpr()) }
186191
}
187192

193+
/**
194+
* Tracks flows from remote user input to a deserialization sink.
195+
*/
188196
class UnsafeDeserializationConfig extends TaintTracking::Configuration {
189197
UnsafeDeserializationConfig() { this = "UnsafeDeserializationConfig" }
190198

@@ -229,3 +237,77 @@ class UnsafeDeserializationConfig extends TaintTracking::Configuration {
229237
)
230238
}
231239
}
240+
241+
/**
242+
* Tracks flow from a remote source to a type descriptor (e.g. a `java.lang.Class` instance)
243+
* passed to a Jackson deserialization method.
244+
*
245+
* If this is user-controlled, arbitrary code could be executed while instantiating the user-specified type.
246+
*/
247+
class UnsafeTypeConfig extends TaintTracking2::Configuration {
248+
UnsafeTypeConfig() { this = "UnsafeTypeConfig" }
249+
250+
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
251+
252+
override predicate isSink(DataFlow::Node sink) {
253+
exists(MethodAccess ma, int i, Expr arg | i > 0 and ma.getArgument(i) = arg |
254+
ma.getMethod() instanceof ObjectMapperReadMethod and
255+
arg.getType() instanceof JacksonTypeDescriptorType and
256+
arg = sink.asExpr()
257+
)
258+
}
259+
260+
/**
261+
* Holds if `fromNode` to `toNode` is a dataflow step that resolves a class
262+
* or at least looks like resolving a class.
263+
*/
264+
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
265+
resolveClassStep(fromNode, toNode) or
266+
looksLikeResolveClassStep(fromNode, toNode)
267+
}
268+
}
269+
270+
/**
271+
* Tracks flow from `enableDefaultTyping` calls to a subsequent Jackson deserialization method call.
272+
*/
273+
class EnableJacksonDefaultTypingConfig extends DataFlow2::Configuration {
274+
EnableJacksonDefaultTypingConfig() { this = "EnableJacksonDefaultTypingConfig" }
275+
276+
override predicate isSource(DataFlow::Node src) {
277+
any(EnableJacksonDefaultTyping ma).getQualifier() = src.asExpr()
278+
}
279+
280+
override predicate isSink(DataFlow::Node sink) { sink instanceof ObjectMapperReadQualifier }
281+
}
282+
283+
/**
284+
* Tracks flow from calls which set a type validator to a subsequent Jackson deserialization method call,
285+
* including across builder method calls.
286+
*
287+
* Such a Jackson deserialization method call is safe because validation will likely prevent instantiating unexpected types.
288+
*/
289+
class SafeObjectMapperConfig extends DataFlow2::Configuration {
290+
SafeObjectMapperConfig() { this = "SafeObjectMapperConfig" }
291+
292+
override predicate isSource(DataFlow::Node src) {
293+
src instanceof SetPolymorphicTypeValidatorSource
294+
}
295+
296+
override predicate isSink(DataFlow::Node sink) { sink instanceof ObjectMapperReadQualifier }
297+
298+
/**
299+
* Holds if `fromNode` to `toNode` is a dataflow step
300+
* that configures or creates an `ObjectMapper` via a builder.
301+
*/
302+
override predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
303+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
304+
m.getDeclaringType() instanceof MapperBuilder and
305+
m.getReturnType()
306+
.(RefType)
307+
.hasQualifiedName("com.fasterxml.jackson.databind.json",
308+
["JsonMapper$Builder", "JsonMapper"]) and
309+
fromNode.asExpr() = ma.getQualifier() and
310+
ma = toNode.asExpr()
311+
)
312+
}
313+
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@ class UnsafeDeserializationTest extends InlineExpectationsTest {
99

1010
override predicate hasActualResult(Location location, string element, string tag, string value) {
1111
tag = "unsafeDeserialization" and
12-
exists(DataFlow::Node sink, UnsafeDeserializationConfig conf |
13-
conf.hasFlowTo(sink)
14-
|
12+
exists(DataFlow::Node sink, UnsafeDeserializationConfig conf | conf.hasFlowTo(sink) |
1513
sink.getLocation() = location and
1614
element = sink.toString() and
1715
value = ""

0 commit comments

Comments
 (0)