Added validation on Builder.build() to check if any matcher will #2533
Conversation
…w basetype/subtype of Serializable or Object.class If so an unchecked TypeMatchException is thrown. This can be bypassed by using build_insecure() instead which has no such checks. The aim of this change is to : A : Make developers realise what they are doing has serious security implications and hopefully accomplish their requirements in a different way. B : Make it easier for static code analysis to flag insecure usage of Jackson-Databind as those tools can assume that if a developer is calling build_insecure() they are doing that because they are allowing all or alot of classes on the classpath to be deserialized.
| * validates that each of the Matchers will not allow Serializable.class or Object.class. | ||
| * @throws InsecureTypeMatchException if an included Matcher will allow one or both of these classes through. | ||
| */ | ||
| private void validateTypeMatch() throws InsecureTypeMatchException { |
There was a problem hiding this comment.
Um, no. If user explicitly adds insecure base type, that should not be blocked.
| * so will leave your application open to vulnerabilities including remote code execution. | ||
| * @return the BasicPolymorphicTypeValidator. | ||
| */ | ||
| public BasicPolymorphicTypeValidator build_insecure() { |
There was a problem hiding this comment.
build_insecure() is not following any naming convention, but in general I'll add my notes on why I don't think this is the way to go.
|
First of all, thank you for proposing the patch. Instead of checking things at There is also no need to check subtype filtering methods: class name / class passed would have to be gadget, not But at the same time, there is then need to add something like On plus side, doing this would be bit simpler approach. |
|
Implemented slightly different solution, see #2587, to further improve safety. Closing this PR. |
allow basetype/subtype of Serializable or Object.class
If so an unchecked TypeMatchException is thrown.
This can be bypassed by using build_insecure() instead which has no such checks.
The aim of this change is to :
A : Make developers realise what they are doing has serious security implications and hopefully accomplish their requirements in a different way.
B : Make it easier for static code analysis to flag insecure usage of Jackson-Databind as those tools can assume that if a developer is calling
build_insecure() they are doing that because they are allowing all or alot of classes on the classpath to be deserialized.