FasterXML / FasterXML/jackson-annotations

Consider making `@JacksonInject` `useInput=false` the default

Open
#186 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
1.1k
Forks
342
Avg merge
8h 53m
Merged PRs (30d)
2

Description

For the recently found vulnerability [CVE-2021-25646](https://nvd.nist.gov/vuln/detail/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.](https://www.youtube.com/watch?v=wPqK-Ealz-0)".

Part of the problem is also that the current Jackson documentation is incorrect, or incomplete:
- The [wiki](https://github.com/FasterXML/jackson-annotations/wiki/Jackson-Annotations#deserialization-details) 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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.