It would be good if BasicPolymorphicTypeValidator blocked Object and Serializable by default.

[JoeBeeton]

First thank you for addressing the polymorphic deserialization vulnerabilities.
But in 2.10, by a dev doing something like
PolymorphicTypeValidator ptv = BasicPolymorphicTypeValidator.builder() .allowIfSubType(Object.class/Serializable.class) .build();
It is possible to enable all classes to be allowed to be deserialized. While it is not possible to completely block this, (someone could write their own validator ). In reality they are unlikely to and most will use your BasicPolymorphicTypeValidator. So to help make it more secure from developers that don't understand the potential vulnerabilities they are adding. I thought it might be useful to not allow the above whitelisting of everything, ( if that is set thrown an exception ). But allow it using it's own method call, something like
PolymorphicTypeValidator ptv = BasicPolymorphicTypeValidator.builder() .allowSubType_Insecure(Object.class/Serializable.class) .build();
That way it is flagging to the developer that they are doing something insecure and that they should think again.

[JoeBeeton]

I would be happy to raise a PR for the above if you think it is a good idea and likely to be included.

[cowtowncoder]

I appreciate the suggestion and agree with much of thinking. But I have couple of major concerns here:

1. Doing this in 2.10.x would break (for now) valid 2.10.0 usage
2. I suspect that those who want to allow any and all usage, will find the way.

So from versioning standpoint, if added, it should go in 2.11.

[JoeBeeton]

Putting it in 2.11 makes sense as you are right it would break functionality. Yes it would definitely be possible to bypass, but by making the developer acknowledge the fact they are doing something unsafe you make them think about what they are doing. As it is not immediately obvious to most developers that allowing subtype of Object will cause a vulnerability. It will also make it easer for SAST tooling to flag the usage as well. Do you have any kind of timeline for when 2.11 will be released?

[cowtowncoder]

I do not have firm deadline, but I plan for this to be much faster than 2.10. So -- if all goes well -- maybe release candidates by December, release in January 2020.

So: on check; I think check should be made when building BasicPolymorphicTypeValidator, as early as possible. While still backwards-incompatible, it would only be done in new methods so at least damage would be more contained.

I think it would also make sense to discuss this on jackson-dev, to see how others view the issue.
I am still going back and forth with this; but I do feel responsible for security so maybe this is worth doing.

[jamesrgrinter]

I think I'd like to see the actual functionality for Polymorphic Type handling somehow moved into a separate artifact that must be explicitly added as a dependency, so that I could be reassured that it is not present in my build or runtime environments and therefore cannot be used by any other bit of code, first or third party.

Whether that should then throw a runtime NoSuchMethodException or ClassNotFoundException, if they then attempt to use it, I don't know.

Obviously this approach wouldn't help existing deployed versions, but then neither does the current approach of adding blocked gadgets in release updates.

[cowtowncoder]

A big problem I have is that any code that can enable default typing can also implement and register custom deserializers, which allows implementing default typing and similar functionality.

But the biggest problem from my perspective is even simpler: changing functionality works ok for those case where developer is directly controlling inclusion and use of Jackson. But not so well for cases where usage is indirect, via transitive dependencies. In those cases pulling in of a newer version of Jackson, with suddenly breaking use of (safe) default typing, can break other parts of the system that use it.
I will get to hear about those cases because "change you made suddenly broke my system".

I think there is another issue for separating Default Typing functionality for 3.x which is related; and since it is a major version increase, these changes are acceptable; unsafe default typing functionality has been removed from master.

But I do not think extracting of Default Typing should be done for 2.x versions.

[JoeBeeton]

I have the commit for the above change ready to push and raise a PR. I guess I need to sign the CLA before I get access to push a branch? I'll send the CLA through tomorrow.

[cowtowncoder]

You should not need CLA to create PR. But you probably need to fork, make changes on your fork, then create PR.

[JoeBeeton]

Ahh, ok I was trying to push my branch directly into the jackson-databind repo. I'll fork instead.

[JoeBeeton]

PR created
#2533

[cowtowncoder]

Ok, so: I think the approach I prefer is via #2587: this is a MapperFeature that can be changed to enforce strict defaults to @JsonTypeInfo. Jackson 3.0 will default to non-permissive PTV for both cases, but with 2.x there are backwards compatibility issues to consider.

[JoeBeeton]

Yes I can understand you dont want to add this for backwards compatibility reasons.

[cowtowncoder]

I hope #2587 linked to above can achieve the same (and its equivalent which is the default for 3.0). Thank you @JoeBeeton for providing the original patch which lead to this addition for 2.11.0, to be released soon!