-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Java: Unsafe deserialization with Jackson #5900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
aschackmull
merged 26 commits into
github:main
from
artem-smotrakov:unsafe-jackson-deserialization
Aug 2, 2021
Merged
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
3eb2af1
First draft of sinks for unsafe deserialization with Jackson
artem-smotrakov 97fca62
Cover attacker-controlled types for deserialization with Jackson
artem-smotrakov ea0991c
Added Jackson to UnsafeDeserialization.qhelp
artem-smotrakov aefd210
Added tests for UnsafeDeserialization.ql and Jackson
artem-smotrakov 24e4b68
Removed getAnAccess() calls for Jackson
artem-smotrakov 704cc77
Added a change note for Jackson
artem-smotrakov e9731cd
Minor improvements for Jackson in UnsafeDeserialization.qll
artem-smotrakov 476843a
Added comments for Jackson in UnsafeDeserialization.qll
artem-smotrakov c98f1a4
Better taint propagation in UnsafeTypeConfig
artem-smotrakov 09ae779
Removed fromSource() check in looksLikeResolveClassStep()
artem-smotrakov 1b3516a
Apply suggestions from code review
artem-smotrakov 6d7cb48
Refactored the query for unsafe deserialization
artem-smotrakov 3856527
Refactored tests for unsafe deserialization
artem-smotrakov c367c7e
Merge branch 'unsafe-jackson-deserialization' of github.com:artem-smo…
artem-smotrakov e025307
Apply suggestions from code review
artem-smotrakov 035f7ac
Refactored libs for unsafe deserialization
artem-smotrakov 47e4cf4
Make UnsafeDeserializationSink public
artem-smotrakov 158a75e
Import UnsafeDeserializationQuery in unsafeDeserialization.ql
artem-smotrakov 7fec575
Simplify JsonTypeInfo stub
artem-smotrakov 50497eb
Make imports as private as possible
artem-smotrakov 893f84f
Merge branch 'unsafe-jackson-deserialization' of github.com:artem-smo…
artem-smotrakov 83a9b0e
Apply suggestions from code review
artem-smotrakov 1d3eb57
hasJsonTypeInfoAnnotation() should check fields recursively
artem-smotrakov a4b0041
Better looksLikeResolveClassStep() predicate
artem-smotrakov 7959e76
Better qldoc in UnsafeDeserializationQuery.qll
artem-smotrakov 6c973b5
Update java/ql/src/semmle/code/java/frameworks/Jackson.qll
aschackmull File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
lgtm,codescanning | ||
* The "Deserialization of user-controlled data" (`java/unsafe-deserialization`) query | ||
now recognizes `Jackson` deserialization. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
/** | ||
* Provides classes and predicates for working with the Jackson serialization framework. | ||
*/ | ||
|
||
import java | ||
private import semmle.code.java.Reflection | ||
private import semmle.code.java.dataflow.DataFlow | ||
|
||
private class ObjectMapper extends RefType { | ||
ObjectMapper() { | ||
getASupertype*().hasQualifiedName("com.fasterxml.jackson.databind", "ObjectMapper") | ||
} | ||
} | ||
|
||
/** A builder for building Jackson's `JsonMapper`. */ | ||
class MapperBuilder extends RefType { | ||
MapperBuilder() { | ||
hasQualifiedName("com.fasterxml.jackson.databind.cfg", "MapperBuilder<JsonMapper,Builder>") | ||
} | ||
} | ||
|
||
private class JsonFactory extends RefType { | ||
JsonFactory() { hasQualifiedName("com.fasterxml.jackson.core", "JsonFactory") } | ||
} | ||
|
||
private class JsonParser extends RefType { | ||
JsonParser() { hasQualifiedName("com.fasterxml.jackson.core", "JsonParser") } | ||
} | ||
|
||
/** A type descriptor in Jackson libraries. For example, `java.lang.Class`. */ | ||
class JacksonTypeDescriptorType extends RefType { | ||
JacksonTypeDescriptorType() { | ||
this instanceof TypeClass or | ||
hasQualifiedName("com.fasterxml.jackson.databind", "JavaType") or | ||
hasQualifiedName("com.fasterxml.jackson.core.type", "TypeReference") | ||
} | ||
} | ||
|
||
/** A method in `ObjectMapper` that deserialize data. */ | ||
class ObjectMapperReadMethod extends Method { | ||
ObjectMapperReadMethod() { | ||
this.getDeclaringType() instanceof ObjectMapper and | ||
this.hasName(["readValue", "readValues", "treeToValue"]) | ||
} | ||
} | ||
|
||
/** A call that enables the default typing in `ObjectMapper`. */ | ||
class EnableJacksonDefaultTyping extends MethodAccess { | ||
EnableJacksonDefaultTyping() { | ||
this.getMethod().getDeclaringType() instanceof ObjectMapper and | ||
this.getMethod().hasName("enableDefaultTyping") | ||
} | ||
} | ||
|
||
/** A qualifier of a call to one of the methods in `ObjectMapper` that deserialize data. */ | ||
class ObjectMapperReadQualifier extends DataFlow::ExprNode { | ||
ObjectMapperReadQualifier() { | ||
exists(MethodAccess ma | ma.getQualifier() = this.asExpr() | | ||
ma.getMethod() instanceof ObjectMapperReadMethod | ||
) | ||
} | ||
} | ||
|
||
/** A source that sets a type validator. */ | ||
class SetPolymorphicTypeValidatorSource extends DataFlow::ExprNode { | ||
SetPolymorphicTypeValidatorSource() { | ||
exists(MethodAccess ma, Method m | m = ma.getMethod() | | ||
( | ||
m.getDeclaringType() instanceof ObjectMapper and | ||
m.hasName("setPolymorphicTypeValidator") | ||
or | ||
m.getDeclaringType() instanceof MapperBuilder and | ||
m.hasName("polymorphicTypeValidator") | ||
) and | ||
this.asExpr() = ma.getQualifier() | ||
) | ||
} | ||
} | ||
|
||
/** Holds if `fromNode` to `toNode` is a dataflow step that resolves a class. */ | ||
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 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. | ||
*/ | ||
predicate createJacksonJsonParserStep(DataFlow::Node fromNode, DataFlow::Node toNode) { | ||
exists(MethodAccess ma, Method m | m = ma.getMethod() | | ||
(m.getDeclaringType() instanceof ObjectMapper or m.getDeclaringType() instanceof JsonFactory) and | ||
m.hasName("createParser") and | ||
ma.getArgument(0) = fromNode.asExpr() and | ||
ma = toNode.asExpr() | ||
) | ||
} | ||
|
||
/** | ||
* 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. | ||
*/ | ||
predicate createJacksonTreeNodeStep(DataFlow::Node fromNode, DataFlow::Node toNode) { | ||
exists(MethodAccess ma, Method m | m = ma.getMethod() | | ||
m.getDeclaringType() instanceof ObjectMapper and | ||
m.hasName("readTree") and | ||
ma.getArgument(0) = fromNode.asExpr() and | ||
ma = toNode.asExpr() | ||
) | ||
or | ||
exists(MethodAccess ma, Method m | m = ma.getMethod() | | ||
m.getDeclaringType() instanceof JsonParser and | ||
m.hasName("readValueAsTree") and | ||
ma.getQualifier() = fromNode.asExpr() and | ||
ma = toNode.asExpr() | ||
) | ||
} | ||
|
||
/** | ||
* Holds if `type` or one of its supertypes has a field with `JsonTypeInfo` annotation | ||
* that enables polymorphic type handling. | ||
*/ | ||
private predicate hasJsonTypeInfoAnnotation(RefType type) { | ||
hasFieldWithJsonTypeAnnotation(type.getASupertype*()) or | ||
hasJsonTypeInfoAnnotation(type.getAField().getType()) | ||
} | ||
|
||
/** | ||
* Holds if `type` has a field with `JsonTypeInfo` annotation | ||
* that enables polymorphic type handling. | ||
*/ | ||
private predicate hasFieldWithJsonTypeAnnotation(RefType type) { | ||
exists(Annotation a | | ||
type.getAField().getAnAnnotation() = a and | ||
a.getType().hasQualifiedName("com.fasterxml.jackson.annotation", "JsonTypeInfo") and | ||
a.getValue("use").(VarAccess).getVariable().hasName(["CLASS", "MINIMAL_CLASS"]) | ||
) | ||
} | ||
|
||
/** | ||
* Holds if `call` is a method call to a Jackson deserialization method such as `ObjectMapper.readValue(String, Class)`, | ||
* and the target deserialized class has a field with a `JsonTypeInfo` annotation that enables polymorphic typing. | ||
*/ | ||
predicate hasArgumentWithUnsafeJacksonAnnotation(MethodAccess call) { | ||
call.getMethod() instanceof ObjectMapperReadMethod and | ||
exists(RefType argType, int i | i > 0 and argType = call.getArgument(i).getType() | | ||
hasJsonTypeInfoAnnotation(argType.(ParameterizedType).getATypeArgument()) | ||
) | ||
} | ||
|
||
/** | ||
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class. | ||
* A method probably resolves a class if 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. | ||
*/ | ||
predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) { | ||
exists(MethodAccess ma, Method m, Expr arg | m = ma.getMethod() and arg = ma.getAnArgument() | | ||
m.getReturnType() instanceof JacksonTypeDescriptorType and | ||
m.getName().toLowerCase().regexpMatch("(.*)(resolve|load|class|type)(.*)") and | ||
arg.getType() instanceof TypeString and | ||
arg = fromNode.asExpr() and | ||
ma = toNode.asExpr() | ||
) | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.