Skip to content
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

Allow customization of polymorphic deserialization black-list #2208

Closed
meeque opened this issue Dec 13, 2018 · 9 comments
Closed

Allow customization of polymorphic deserialization black-list #2208

meeque opened this issue Dec 13, 2018 · 9 comments

Comments

@meeque
Copy link

meeque commented Dec 13, 2018

In the past Jackson Databind has had numerous vulnerabilities that were related to polymorphic deserialization. In particular, many classes turn out to be insecure when deserialized from untrusted data. These classes are often called deserialization gadgets.

Jackson Databind has addressed such vulnerabilities by maintaining a black-list of classes that must not be deserialized. However, new classes are often identified to be deserialization gadgets, and the black-list needs to be extended frequently.

Right now (checked on 2.9.7 sources and on master) there is no convenient way for users of Jackson Databind to extend the black-list. In particular, users would have to either modify the Jackson Databind sources or re-implement several classes of Jackson Databind.

Instead, it would be great if users could extend the blacklist by means of configuration. This would have the following benefits:

  • Users can black-list their own classes that might be deserialization widgets.
    (If such classes are not published as open source, it is unlikely that they will ever make it on the built-in black-list that is maintained by Jackson Databind.)
  • When new classes are added to the built-in black-list that is maintained by Jackson Databind, users can adjust their own black-list accordingly.
    (This may be important for users who cannot readily upgrade to the latest Jackson Databind release. It can also help closing the time-gap between classes being proposed for the built-in black-list and an actual release that contains the updated black-list.)
@meeque
Copy link
Author

meeque commented Dec 13, 2018

Btw, I'm aware that a black-list is generally not a great security control. I highly appreciate the suggestion to provide white-listing facilities instead, as proposed in #2195. However, it seems that such white-listing would be optional, and I'm worried that many users might neglect it.

Thus I believe, that the black-listing mechanism should still be improved. IMHO allowing users to extend the black-list would be a step forward here.

If anyone agrees, I'll be happy to submit a PR for this. But I'd like to discuss first, how configuration should work from a user perspective. Some ideas:

  • Make a custom black-list (that would extend, rather than replace the built-in one) available in DeserializerFactoryConfig.
  • Allow users to inject their own implementation of SubTypeValidator. (Not sure where that should be configured?)

@meeque meeque changed the title Allow customization of polymorphic deserialization deserialization black-list Allow customization of polymorphic deserialization black-list Dec 13, 2018
@meeque
Copy link
Author

meeque commented Dec 28, 2018

Any thoughts on this, anyone?

@cowtowncoder
Copy link
Member

First of all, apologies for late response. I did notice issue but forgot to follow up.

This issue has been discussed on few occasions on mailing lists, which are better place to discuss things. But you can see my thinking on #2195 which I think is closer to second option you mention. Plan would be to add new abstraction/config hook in 2.10 as 3.0 is still quite a long way out.

Also: sort of related -- I am thinking of starting some sort of process for writing proposals (Jackson Enhancement Proposals?). See "Big Ideas" entry on:

https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress

and maybe wiki on:

https://github.com/FasterXML/jackson-future-ideas/wiki

Anyway: the idea would be to write simple (usually/ideally :) ) proposals of high-level change idea. Something bit more than an issue (since work may be done under multiple issues), as basis for discussion, collaboration.
Work for this particular issue does not have go that route, of course, but I mention it as things like this would probably benefit from such collaboration.

@meeque
Copy link
Author

meeque commented Jan 4, 2019

Sorry, I'm not on the mailing lists, and did not check the archives. I had read #2195, and I was under the impression that that one was more related to local measures. I.e. something that users would have to declare in @JsonTypeInfo or related annotations.

Now that I re-read #2195 and also jackson3-dev #21 it seems that you also plan to support the proposed handler API globally, i.e. at the level of a ObjectMapper. Can you confirm that?

Btw, I appreciate the idea of a new handler API that addresses these problem. However, I still think it would be worthwhile to simply improve configurability of the SubTypeValidator as a first step. I'll be mostly off the computer for one more week. But I'll try to play with the code afterwards, and see if I can come up with anythink that might be worth contributing. Ok?

@cowtowncoder
Copy link
Member

Yes, the idea would be to allow configuring that handler on ObjectMapper, using newly available "builder" construction being added in 2.10.
I am not against improved configurability of SubTypeValidator either, at general level (but specific approach does matter).

meeque pushed a commit to meeque/jackson-databind that referenced this issue Feb 28, 2019
meeque pushed a commit to meeque/jackson-databind that referenced this issue Feb 28, 2019
meeque pushed a commit to meeque/jackson-databind that referenced this issue Feb 28, 2019
meeque pushed a commit to meeque/jackson-databind that referenced this issue Mar 1, 2019
…implementation: ExtensibleSubTypeValidator.
@cowtowncoder
Copy link
Member

I should have read this through with more thought, and get back to my thinking.

So, I do not think configurability of SubTypeValidator in its current form makes sense, even for short-term.
I do plan on getting #2195 implemented for 2.10, which is the first version that could have API changes anyway.
Apologies for not stating this earlier and possibly causing you extra work here.

@artem-smotrakov
Copy link
Contributor

I like the idea to have a way to configure the blacklist. I am just wondering if it might make sense to introduce a system property (or security property) which contains a list of additional classes to SubTypeValidator.DEFAULT_NO_DESER_CLASS_NAMES in the static initialize. I think this is not the most elegant solution, maybe #2195 will then provide a better way to make the blacklist configurable. On the other hand, it seems to be a very simple solution which also allows a user to blacklist a new gadget right away without modifying the code and without upgrading Jackson Databind (well, the user still needs to start the JVM with the right value of the system property).

@meeque @cowtowncoder What do you think?

@cowtowncoder
Copy link
Member

I do not plan on configurability of the black list explicitly, although #2195 will allow users to use blocking over allowing if they want to.

Jackson does not use system properties, and this is a design principle: System properties are global configuration that works poorly with otherwise isolated instances of ObjectMapper, and are problematic when Jackson is used by different libraries, frameworks, and application: it should be possible to configure them differently as necessary (and when sharing of same setting needed, should use same shared mapper).
I am not against additional components that use system properties (they may make complete sense for some users), just think that it does not work well at core library level where actual usages vary a lot.

@meeque
Copy link
Author

meeque commented Apr 18, 2019

My PR #2269 and the related discussions did not seem to lead anywhere useful. Please feel free to close this issue without a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants