Java: Unsafe deserialization with Jackson #5900

artem-smotrakov · 2021-05-14T09:13:32Z

Deserialization of untrusted data with Jackson is known to be dangerous:

https://www.github.com/mbechler/marshalsec/blob/master/marshalsec.pdf?raw=true

I'd like to propose to add sinks for Jackson to UnsafeDeserialization.ql:

Added sinks for ObjectMapper.
Checked if polymorphic typing is enabled by ObjectMapper.enableDefaultTyping() call.
Checked if polymorphic typing is enabled by JsonTypeInfo annotation.
Checked if an attacker can control both data and type of deserialized object.

See https://cowtowncoder.medium.com/on-jackson-cves-dont-panic-here-is-what-you-need-to-know-54cd0d6e8062

There are multiples CVEs for deserialization gadgets for Jackson Databind

https://www.cvedetails.com/vulnerability-list/vendor_id-15866/product_id-42991/Fasterxml-Jackson-databind.html

Those CVEs create quite a lot of noise because many applications don't use polymorphic typing. The query would help check if applications are really affected by such CVEs. On the other hand, if an application actually turns on polymorphic typing, and deserializes data from remote peers, that would be a significant security risk that can be identified by this query.

The changeset is quite big and the query is in the supported query set. Would it be better to create a separate experimental query instead?

I'm going to add tests and update docs once we know whether this should be part of the supported query or a separate one. Until the questions above gets an answer, I'd like to keep this pull request in draft mode but earlier feedback is of course very welcome.

Results:

The query detects CVE-2016-8749 in Apache Camel 2.16.4.

smowton

Looks sensible to me. I've suggested removing the getAnAccess() trickery because there should be use-use flow from earlier to later uses of the same qualifier variable anyhow. Do you know of a particular case where this was necessary?

@aschackmull this will need your review as not targeting experimental

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

artem-smotrakov · 2021-05-14T16:04:59Z

@aschackmull this will need your review as not targeting experimental

@smowton @aschackmull As I mentioned in the description, I am not sure whether this should go to UnsafeDeserialization.ql or to a separate experimental query. Please let me know what would be the best option.

smowton · 2021-05-14T17:16:06Z

I favour inclusion in the existing query but the final call is @aschackmull

smowton · 2021-05-17T10:28:08Z

Also what of the getAnAccess() stuff, do you know of a case that makes this necessary for some reason? Or does the query work just as well without thanks to ordinary dataflow?

Either way you should add a change-note.

artem-smotrakov · 2021-05-17T18:44:57Z

Also what of the getAnAccess() stuff, do you know of a case that makes this necessary for some reason? Or does the query work just as well without thanks to ordinary dataflow?

I added the getAnAccess() stuff because the dataflow configs didn't work in some cases. I noticed that other configs in UnsafeDeserialization.qll cast qualifiers to VarAccess. That helped but I didn't really understand why. That would be interesting to look into it now but I can't reproduce that issue anymore. I've removed the getAnAccess() stuff. I wonder why SafeXStream and SafeKryo use getAnAccess(). Maybe it is not necessary anymore either.

Either way you should add a change-note.

I've added a change note. I've never done it before, so I used https://github.com/github/codeql/blob/main/java/change-notes/2020-10-07-fastjson-deserialization-sink.md as an example.

smowton

There are quite a few additional taint steps being introduced, which aren't easy for me to see the purpose of without an example. Could you write down a short example for each that uses one of the particular new steps?

In some cases these examples might be useful in the relevant qldoc too; in others they might just be to help me review.

smowton · 2021-05-18T07:44:00Z

java/change-notes/2021-05-17-jackson-deserialization-sink.md

+lgtm,codescanning
+* The "Deserialization of user-controlled data" (`java/unsafe-deserialization`) query
+  now recognizes `Jackson` deserialization.
+


Suggested change

smowton · 2021-05-18T07:44:57Z

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

@@ -18,6 +20,71 @@ class XMLDecoderReadObjectMethod extends Method {
  }
 }

+class ObjectMapperReadMethod extends Method {


Every class here: either make private or add qldoc

smowton · 2021-05-18T07:46:20Z

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

+  JsonParser() { hasQualifiedName("com.fasterxml.jackson.core", "JsonParser") }
+}
+
+class JacksonType extends RefType {


Suggested change

class JacksonType extends RefType {

class JacksonTypeDescriptorType extends RefType {

Existing name suggests any type in the com.fasterxml.jackson namespace

smowton · 2021-05-18T07:47:22Z

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

@@ -109,6 +176,119 @@ class SafeKryo extends DataFlow2::Configuration {
  }
 }

+class EnabledJacksonDefaultTyping extends DataFlow2::Configuration {


Suggested change

class EnabledJacksonDefaultTyping extends DataFlow2::Configuration {

class EnableJacksonDefaultTypingConfig extends DataFlow2::Configuration {

Less similar to existing class EnableJacksonDefaultTyping

smowton · 2021-05-18T07:50:35Z

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

+   * that configures or creates an `ObjectMapper` via a builder.
+   */
+  override predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    exists(MethodAccess ma, Method m, Expr q | m = ma.getMethod() and q = ma.getQualifier() |


Suggested change

exists(MethodAccess ma, Method m, Expr q | m = ma.getMethod() and q = ma.getQualifier() |

exists(MethodAccess ma, Method m | m = ma.getMethod() |

Inline the one use of ma.getQualifier()

smowton · 2021-05-18T07:54:38Z

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

+  override predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    exists(MethodAccess ma, Method m, Expr q | m = ma.getMethod() and q = ma.getQualifier() |
+      m.getDeclaringType() instanceof MapperBuilder and
+      m.getReturnType()


Could you give an end-to-end example that these steps are designed to address?

ObjectMapper may be created by JsonMapper.Builder that extends MapperBuilder and uses fluent API. In other words, its methods return this:

https://fasterxml.github.io/jackson-databind/javadoc/2.11/com/fasterxml/jackson/databind/cfg/MapperBuilder.html

JsonMapper$Builder has quite a lot of methods. Therefore, I thought that it maybe better to cover all methods that return the builder instead of listing all the available methods. Furthermore, the list should be updated once new methods are added.

Here is an example:

private static <T> T deserialize(/* tainted */ String string, Class<T> clazz) throws IOException { PolymorphicTypeValidator ptv = BasicPolymorphicTypeValidator.builder() .allowIfSubType("com.test.safe") .build(); // use a builder to create an ObjectMapper ObjectMapper mapper = JsonMapper.builder() .polymorphicTypeValidator(ptv) // sanitizer .build(); mapper.enableDefaultTyping(); return mapper.readValue(string, clazz); // deserialization happens here }

Builders need to be covered in the query because they have polymorphicTypeValidator() that is a sanitizer.

smowton · 2021-05-18T07:54:44Z

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

+   * Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
+   */
+  override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    exists(MethodAccess ma, RefType returnType | returnType = ma.getMethod().getReturnType() |


Could you give an end-to-end example that these steps are designed to address?

ObjectMapper.readValue() and other similar methods take a serialized object and a target object type. It is dangerous when an attacker can control both the data and the type. That's what happened in CVE-2016-8749.

The type maybe either Class, JavaType or TypeReference. This predicate checks if a method seems to take a class name as a string and convert it to one of these classes. Here is an example:

private static Object deserialize(/* tainted */ String string, /* tainted */ String className) throws IOException { ObjectMapper mapper = new ObjectMapper(); mapper.enableDefaultTyping(); return mapper.readValue(string, resolveType(className)); } private static Class resolveType(String className) { return Class.forName(className); }

Here, resolveType() is simple -- it just calls Class.forName(). But its logic might be more complex that could potentially result it false-negatives. The predicate looks for methods like resolveType() assuming that they create a type descriptor that is known to Jackson. Of course, that may result in false-positives. I thought that should be acceptable trade-off because deserialization vulnerabilities are quite dangerous.

smowton · 2021-05-18T08:02:13Z

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

+/**
+ * Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.
+ */
+predicate createJacksonJsonParserStep(DataFlow::Node fromNode, DataFlow::Node toNode) {


Could you give an end-to-end example that these steps are designed to address?

ObjectMapper has multiple readValue() and readValues() methods that take a JsonParser. In turn, JsonParser takes JSON that may come from untrusted sources.

This predicate defines steps that create a JsonParser. Here is an example:

private static <T> List<T> deserialize(/* tainted */ String string, Class<T> clazz) throws IOException { ObjectMapper mapper = new ObjectMapper(); mapper.enableDefaultTyping(); JsonParser parser = new JsonFactory().createParser(string); return mapper.readValues(parser, clazz).readAll(); }

Could the createParser be a sink, or could we still encounter sanitisers on the road to readValues?

Could the createParser be a sink,

I don't think so. JsonParser parses JSON and returns primitive values, strings, JSON tokens, etc. It can return a type id or descriptor but as far as I know the parser doesn't create instances of these types.

or could we still encounter sanitisers on the road to readValues?

I am aware of only two reliable sanitizers that are covered in the query. I was considering adding ObjectMapper.activateDefaultTyping() because it takes PolymorphicTypeValidator. But it turned out that in this case the validator is not always called. I reported this to the project, but they said that's expected. Please see FasterXML/jackson-databind#2524 . Therefore, I didn't add this method as a sanitizer.

smowton · 2021-05-18T08:02:16Z

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

+/**
+ * Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson `TreeNode`.
+ */
+predicate createJacksonTreeNodeStep(DataFlow::Node fromNode, DataFlow::Node toNode) {


Could you give an end-to-end example that these steps are designed to address?

ObjectMapper has treeToValue() method is sink that takes a TreeNode. Raw JSON data may be converted to a TreeNode and then supplied to treeToValue(). A TreeNode can be created by ObjectMapper.readTree() or ObjectMapper.readValueAsTree() methods. Here is an example:

private static <T> T deserialize(/* tainted */ String string, Class<T> clazz) throws IOException { ObjectMapper mapper = new ObjectMapper(); mapper.enableDefaultTyping(); TreeNode tree = mapper.readTree(string); return mapper.treeToValue(tree, clazz); }

artem-smotrakov · 2021-05-24T09:47:31Z

There are quite a few additional taint steps being introduced, which aren't easy for me to see the purpose of without an example. Could you write down a short example for each that uses one of the particular new steps?

In some cases these examples might be useful in the relevant qldoc too; in others they might just be to help me review.

@smowton Sure, I've added examples and some comments. Please let me know if you have questions. You can find complete examples in https://github.com/artem-smotrakov/ql-fun/tree/master/src/main/java/com/gypsyengineer/ql/fun/jackson

I noticed that UnsafeDeserialization.qhelp has only one example for ObjectInputStream but the query covers a lot of libraries. I was not sure those examples should be added to the qhelp file. I can add them though.

Thanks for the review. I've addressed other comments as well.

artem-smotrakov · 2021-05-24T09:58:27Z

There were several conflicts in the stub lib. I rebased the branch and had to force-push.

smowton

OK, this makes sense. The String -> Class heuristic is rather dubious; could you give an example of a Class.forName replacement that we found too difficult to model, and which led you to this overapproximation?

smowton · 2021-05-27T14:10:49Z

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

@@ -109,6 +176,119 @@ class SafeKryo extends DataFlow2::Configuration {
  }
 }

+private class EnableJacksonDefaultTypingConfig extends DataFlow2::Configuration {


Suggested change

private class EnableJacksonDefaultTypingConfig extends DataFlow2::Configuration {

/**

* Tracks flow from qualifiers to `enableDefaultTyping(...)` calls to a subsequent `readValue` or similar call.

*

* Such a `readValue` call could deserialize an arbitrary user-specified class.

*/

private class EnableJacksonDefaultTypingConfig extends DataFlow2::Configuration {

smowton · 2021-05-27T14:14:59Z

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

+  override predicate isSink(DataFlow::Node sink) { sink instanceof ObjectMapperReadSink }
+}
+
+private class SafeObjectMapperConfig extends DataFlow2::Configuration {


Suggested change

private class SafeObjectMapperConfig extends DataFlow2::Configuration {

/**

* Tracks flow from qualifiers to `[set]PolymorphicTypeValidator(...)` calls to a subsequent `readValue` or similar call, including across builder method calls.

*

* Such a `readValue` call is safe because validation will likely prevent instantiating unexpected types.

*/

private class SafeObjectMapperConfig extends DataFlow2::Configuration {

smowton · 2021-05-27T14:17:39Z

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

+  }
+}
+
+private class UnsafeTypeConfig extends TaintTracking2::Configuration {


Suggested change

private class UnsafeTypeConfig extends TaintTracking2::Configuration {

/**

* Tracks flow from a remote source to a type descriptor (e.g. a `java.lang.Class` instance) passed to a Jackson deserialization method.

*

* If this is user-controlled, arbitrary code could be executed while instantiating the user-specified type.

*/

private class UnsafeTypeConfig extends TaintTracking2::Configuration {

smowton · 2021-05-27T14:19:20Z

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

+  }
+
+  /**
+   * Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.


Suggested change

* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.

* 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.

*/

smowton · 2021-05-27T14:22:02Z

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

+/**
+ * Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.
+ */
+predicate createJacksonJsonParserStep(DataFlow::Node fromNode, DataFlow::Node toNode) {


Could the createParser be a sink, or could we still encounter sanitisers on the road to readValues?

smowton · 2021-05-27T14:23:23Z

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

+}
+
+/**
+ * Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.


Suggested change

* Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.

* Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.

*

* For example, a `createParser(userString)` call yields a `JsonParser` which becomes dangerous

* if passed to an unsafely-configured `ObjectMapper`'s `readValue` method.

smowton · 2021-05-27T14:29:17Z

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

+}
+
+/**
+ * Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson `TreeNode`.


Suggested change

* Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson `TreeNode`.

* Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson `TreeNode`.

*

* These are parse trees of user-supplied JSON, which may lead to arbitrary code execution

* if passed to an unsafely-configured `ObjectMapper`'s `treeToValue` method.

smowton · 2021-05-27T14:31:10Z

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

+      or
+      exists(EnableJacksonDefaultTypingConfig config | config.hasFlowToExpr(ma.getQualifier()))
+      or
+      exists(RefType argType, int i | i > 0 and argType = ma.getArgument(i).getType() |


Add a comment here giving an example of a sink method

artem-smotrakov · 2021-05-30T09:57:24Z

OK, this makes sense. The String -> Class heuristic is rather dubious; could you give an example of a Class.forName replacement that we found too difficult to model, and which led you to this overapproximation?

For example, you can look at how Apache Camel resolves classes

https://github.com/apache/camel/blob/881e5099f94316d4a66ffbff0a3e6915829d49d7/components/camel-jacksonxml/src/main/java/org/apache/camel/component/jacksonxml/JacksonXMLDataFormat.java#L170

Camel has an interface ClassResolver and one of the implementations is DefaultClassResolver. Its resolveClass() method in the end calls ObjectHelper.loadClass() with a selected classloader and then casts Class<?> to Class<T> using CastUtils.

https://github.com/apache/camel/blob/80b92e3624ae5db59a1a24a441f1b10b39eaa1a5/core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultClassResolver.java#L54

For some well-known or primitive classes, ObjectHelper.loadClass() just uses an if-else block to map a type to a Class:

https://github.com/apache/camel/blob/80b92e3624ae5db59a1a24a441f1b10b39eaa1a5/core/camel-util/src/main/java/org/apache/camel/util/ObjectHelper.java#L423

Otherwise, doLoadClass() finally calls ClassLoader.loadClass().

Technically, we can add a dataflow config that checks whether tainted data reaches one of the methods from the standard Java library which resolve a class. Unfortunately, such a config won't detect flows when an application uses a library or just a separate utility component that resolves classes (for example, imagine that ObjectHelper were in a separate repository). Then, we'll also need to add similar dataflow configs for JavaType and TypeReference.

I agree that the current String -> Class heuristic is overapproximation. The advantages are that it simplifies the query, probably makes it a bit faster (I hope), and reduces the risk of false positives when the logic for resolving classes is complex. The cost is higher risk of false positives.

smowton · 2021-06-01T09:21:17Z

How about as a compromise, the rule: it's probably a class-loading function if it (a) is external (otherwise we implicitly look inside it for libraries it in turn calls), (b) takes a String, (c) returns a Class, and (d) has a likely name: something containing resolve or load or forName or...

artem-smotrakov · 2021-06-01T09:26:29Z

How about as a compromise, the rule: it's probably a class-loading function if it

@smowton Sounds good to me. One question though.

(a) is external (otherwise we implicitly look inside it for libraries it in turn calls),

How would you prefer to check it? Should we call external everything that don't belong to java and javax packages?

smowton · 2021-06-01T10:06:02Z

Checking for not exists(Callable.getBody()) is a good start. We could be more thorough re: interface types, but assuming generally an interface might be implemented by something external is ok.

smowton · 2021-06-01T13:50:24Z

Actually just remembered Element.fromSource which is exactly what we want here

artem-smotrakov · 2021-06-01T20:10:10Z

Thanks for the suggestions @smowton ! I've updated UnsafeTypeConfig.isAdditionalTaintStep() and added a test case.

smowton · 2021-06-01T20:41:18Z

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

+  |
+    m.getReturnType() instanceof JacksonTypeDescriptorType and
+    m.getName().toLowerCase().regexpMatch("resolve|load|class|type") and
+    m.fromSource() and


Suggested change

m.fromSource() and

not m.fromSource() and

Methods that are from source should themselves call Class.forName or something else matching this heuristic

I think I didn't understand what you mean by "external". I'd like this predicate to cover cases explained by the example above

#5900 (comment)

Something like the following:

private static void deserialize(/* tainted */ String data, /* tainted */ String type) throws Exception { ObjectMapper mapper = new ObjectMapper(); mapper.readValue(data, resolveTypeImpl(type)); } // this method has complex logic for resolving types private static JavaType resolveTypeImpl(String type) throws Exception { ... }

I'd like the predicate to catch resolveTypeImpl() if it is in application's code, not only in an external library. Now, I'd rather drop m.fromSource() check. I've also updated the query to explicitly cover Class.forName() and ClassLoader.load() calls.

smowton · 2021-06-01T20:42:27Z

Over to @aschackmull as this is targeting the main query pack

artem-smotrakov · 2021-06-23T08:50:19Z

I resolved the merge conflicts, and also removed fromSource() check from looksLikeResolveClassStep(), please see #5900 (comment)

java/ql/src/semmle/code/java/frameworks/Jackson.qll

…trakov/ql into unsafe-jackson-deserialization

artem-smotrakov · 2021-07-28T16:30:46Z

@aschackmull I've updated the qldoc and tried to make imports as private as possible. Unfortunately, the docs don't say much about private import

https://codeql.github.com/docs/ql-language-reference/annotations/#private

I am wondering what kind of benefits private imports have besides limiting scope. Does it speed the query up, or maybe reduce memory consumption?

aschackmull · 2021-07-29T09:00:22Z

I am wondering what kind of benefits private imports have besides limiting scope. Does it speed the query up, or maybe reduce memory consumption?

It's primarily just to limit scope and help maintainability. I guess it might affect compilation speed slightly, but that's just speculation on my part.

java/ql/src/semmle/code/java/frameworks/Jackson.qll

Co-authored-by: Anders Schack-Mulligen <[email protected]>

aschackmull · 2021-07-29T09:07:53Z

java/ql/src/semmle/code/java/frameworks/Jackson.qll

+ */
+private predicate hasJsonTypeInfoAnnotation(RefType type) {
+  hasFieldWithJsonTypeAnnotation(type.getASupertype*()) or
+  hasFieldWithJsonTypeAnnotation(type.getAField().getType())


Should this line be deleted? The predicate hasFieldWithJsonTypeAnnotation already includes the .getAField() bit, so this line means that you allow jumping from a type to one of its fields exactly once or twice, which seems odd and inconsistent with the qldoc.

Hmm, it looks like the tests actually use this case? But then the restriction to only one or two .getAField() still seem arbitrary. Should it instead be the following?

Suggested change

hasFieldWithJsonTypeAnnotation(type.getAField().getType())

hasJsonTypeInfoAnnotation(type.getAField().getType())

Yes, you're right. I've updated the query.

aschackmull · 2021-07-29T09:31:27Z

java/ql/src/semmle/code/java/frameworks/Jackson.qll

+    m = ma.getMethod() and arg = ma.getArgument(i)
+  |
+    m.getReturnType() instanceof JacksonTypeDescriptorType and
+    m.getName().toLowerCase().regexpMatch("resolve|load|class|type") and


This doesn't quite do what you want, since regexpMatch matches the entire receiver.

Yup, I'll update the regex. Thanks!

java/ql/src/semmle/code/java/frameworks/Jackson.qll

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

aschackmull

I've looked through everything now, still a few inline comments, otherwise I think this is looking good.

Co-authored-by: Anders Schack-Mulligen <[email protected]>

artem-smotrakov · 2021-07-30T07:34:14Z

I've looked through everything now, still a few inline comments, otherwise I think this is looking good.

@aschackmull I've addressed your comments. Thanks!

java/ql/src/semmle/code/java/frameworks/Jackson.qll

github-actions bot added the Java label May 14, 2021

artem-smotrakov changed the title ~~Unsafe deserialization with Jackson~~ [Java] Unsafe deserialization with Jackson May 14, 2021

artem-smotrakov changed the title ~~[Java] Unsafe deserialization with Jackson~~ Java: Unsafe deserialization with Jackson May 14, 2021

smowton reviewed May 14, 2021

View reviewed changes

artem-smotrakov force-pushed the unsafe-jackson-deserialization branch from 7cd351d to 2faaa75 Compare May 15, 2021 09:38

github-actions bot added the documentation label May 15, 2021

artem-smotrakov marked this pull request as ready for review May 15, 2021 09:49

artem-smotrakov requested a review from a team as a code owner May 15, 2021 09:49

artem-smotrakov mentioned this pull request May 15, 2021

Java: Unsafe deserialization with Jackson github/securitylab#368

Closed

1 task

smowton reviewed May 18, 2021

View reviewed changes

artem-smotrakov force-pushed the unsafe-jackson-deserialization branch from c53305f to 2b16a8e Compare May 24, 2021 09:57

smowton reviewed May 27, 2021

View reviewed changes

smowton reviewed Jun 1, 2021

View reviewed changes

smowton assigned aschackmull Jun 1, 2021

artem-smotrakov added 2 commits July 9, 2021 10:16

First draft of sinks for unsafe deserialization with Jackson

3eb2af1

Cover attacker-controlled types for deserialization with Jackson

97fca62

artem-smotrakov dismissed smowton’s stale review via 7fec575 July 28, 2021 12:23

aschackmull reviewed Jul 28, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 28, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 28, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 28, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

artem-smotrakov added 2 commits July 28, 2021 18:25

Make imports as private as possible

50497eb

Merge branch 'unsafe-jackson-deserialization' of github.com:artem-smo…

893f84f

…trakov/ql into unsafe-jackson-deserialization

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

Apply suggestions from code review

83a9b0e

Co-authored-by: Anders Schack-Mulligen <[email protected]>

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/security/UnsafeDeserializationQuery.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/security/UnsafeDeserializationQuery.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/security/UnsafeDeserializationQuery.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 29, 2021

View reviewed changes

java/ql/src/semmle/code/java/security/UnsafeDeserializationQuery.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 29, 2021

View reviewed changes

artem-smotrakov and others added 3 commits July 30, 2021 08:30

hasJsonTypeInfoAnnotation() should check fields recursively

1d3eb57

Co-authored-by: Anders Schack-Mulligen <[email protected]>

Better looksLikeResolveClassStep() predicate

a4b0041

Better qldoc in UnsafeDeserializationQuery.qll

7959e76

Co-authored-by: Anders Schack-Mulligen <[email protected]>

aschackmull reviewed Aug 2, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Jackson.qll Outdated Show resolved Hide resolved

Update java/ql/src/semmle/code/java/frameworks/Jackson.qll

6c973b5

aschackmull approved these changes Aug 2, 2021

View reviewed changes

aschackmull merged commit 3b676d4 into github:main Aug 2, 2021

artem-smotrakov deleted the unsafe-jackson-deserialization branch August 2, 2021 10:46

	class JacksonType extends RefType {
	class JacksonTypeDescriptorType extends RefType {

	class EnabledJacksonDefaultTyping extends DataFlow2::Configuration {
	class EnableJacksonDefaultTypingConfig extends DataFlow2::Configuration {

	exists(MethodAccess ma, Method m, Expr q \| m = ma.getMethod() and q = ma.getQualifier() \|
	exists(MethodAccess ma, Method m \| m = ma.getMethod() \|

-   * Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
+   * 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.
+   */

	hasFieldWithJsonTypeAnnotation(type.getAField().getType())
	hasJsonTypeInfoAnnotation(type.getAField().getType())

Java: Unsafe deserialization with Jackson #5900

Java: Unsafe deserialization with Jackson #5900

Conversation

artem-smotrakov commented May 14, 2021 • edited Loading

smowton left a comment

Choose a reason for hiding this comment

artem-smotrakov commented May 14, 2021

smowton commented May 14, 2021

smowton commented May 17, 2021

artem-smotrakov commented May 17, 2021

smowton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-smotrakov May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-smotrakov May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-smotrakov commented May 24, 2021

artem-smotrakov commented May 24, 2021

smowton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-smotrakov commented May 30, 2021

smowton commented Jun 1, 2021 • edited Loading

artem-smotrakov commented Jun 1, 2021

smowton commented Jun 1, 2021

smowton commented Jun 1, 2021

artem-smotrakov commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smowton commented Jun 1, 2021

artem-smotrakov commented Jun 23, 2021

artem-smotrakov commented Jul 28, 2021

aschackmull commented Jul 29, 2021

aschackmull Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschackmull left a comment

Choose a reason for hiding this comment

artem-smotrakov commented Jul 30, 2021

artem-smotrakov commented May 14, 2021 •

edited

Loading

artem-smotrakov May 24, 2021 •

edited

Loading

artem-smotrakov May 24, 2021 •

edited

Loading

smowton commented Jun 1, 2021 •

edited

Loading

aschackmull Jul 29, 2021 •

edited

Loading