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:
@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.
For the recently found vulnerability CVE-2021-25646 in Apache Druid a flaw with
@JacksonInjectwas 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 class javadoc currently says:
"Usually property is not deserialized" is wrong, it is the default behavior.
Maybe it would be best to change the default for
useInputto befalsefor 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 betrue. 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.