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

Consider making @JacksonInject useInput=false the default #186

Open
Marcono1234 opened this issue Apr 5, 2021 · 0 comments
Open

Consider making @JacksonInject useInput=false the default #186

Marcono1234 opened this issue Apr 5, 2021 · 0 comments

Comments

@Marcono1234
Copy link

Marcono1234 commented Apr 5, 2021

For the recently found vulnerability CVE-2021-25646 in Apache Druid a flaw with @JacksonInject was abused, that is by default it uses (and overwrites injected data with) deserialized data. For Apache Druid the injected data was suposed to contain a security configuration, but due to using the deserialized data, the user could easily disable the security configuration.
This has been mentioned in GitHub's "LiveQL Episode 2 - The Rhino in the room.".

Part of the problem is also that the current Jackson documentation is incorrect, or incomplete:

  • The wiki says:

    @JacksonInject: annotation to indicate that property should get its value via "injection", and not from data (JSON).

  • The class javadoc currently says:

    Jackson-specific annotation used for indicating that value of annotated property will be "injected", i.e. set based on value configured by ObjectMapper (usually on per-call basis). Usually property is not deserialized from JSON, although it possible to have injected value as default and still allow optional override from JSON.

    "Usually property is not deserialized" is wrong, it is the default behavior.

Maybe it would be best to change the default for useInput to be false for the next major release, possibly with the exception that if the field, method or parameter is also annotated with an annotation explicitly demanding deserialization (e.g. @JsonProperty), then the default should be true.
Even though making software backward compatible is important, it might be more important to make it secure.
What do you think?
(There might also be better solutions, to be honest I am not verify familiar with Jackson.)

Merely changing the default logic to prefer injected over deserialized data might not be enough, e.g. consider a case where a security check applies strict security if the data is absent.

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

1 participant