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

Add check in BeanDeserializer._deserializeFromArray() to prevent use of deeply nested arrays [CVE-2022-42004] #3582

Closed
cowtowncoder opened this issue Aug 24, 2022 · 28 comments
Labels
CVE Issues related to public CVEs (security vuln reports)
Milestone

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 24, 2022

Fix included in

  • 2.13.4
  • 2.12.7.1 micro-patch (jackson-bom 2.12.7.20221012)

(note: found by oss-fuzz, see: https://bugs.chromium.org/p/oss-fuzz/issues)

Currently feature DeserializationFeature.UNWRAP_SINGLE_VALUE_ARRAYS is supported by most types, and deserializers tend to implement support using recursion, effectively allowing multiple nested layers of JSON Arrays to be unwrapped.
This is not a feature to support but just an implementation detail; ideally we should only allow a single JSON Array to wrap a value.

I think I have removed ability for deeper nesting from some other types so there may be some prior art.

@cowtowncoder cowtowncoder added to-evaluate Issue that has been received but not yet evaluated 2.14 and removed to-evaluate Issue that has been received but not yet evaluated labels Aug 24, 2022
cowtowncoder added a commit that referenced this issue Aug 24, 2022
@cowtowncoder cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label Aug 24, 2022
@cowtowncoder cowtowncoder changed the title Add check in BeanDeserializer._deserializeFromArray() to try to prevent use of deeply nested arrays Add check in BeanDeserializer._deserializeFromArray() to prevent use of deeply nested arrays Aug 24, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Sep 4, 2022
@henryrneh
Copy link

Hello dear @cowtowncoder,

I am Henry from Code Intelligence. First of all thank you for your quick fixes of this issue!

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50490

Is this issue regarded as a security issue? If yes, we are thinking about applying CVE for it, so the community knows about it and will update to the latest version of jackson-databind.

Thank you for your fixes and support for OSS-Fuzz!

Best regards,
Henry

@cowtowncoder
Copy link
Member Author

@henryrneh I think it is reasonable to file a CVE for this, although one caveat is that it is only applicable if users enable specific DeserializationFeature and not with vanilla (default) setting of ObjectMapper. So that should probably at least be reflect in applicability -- I do not have any statistics of how common enabling this feature is but it probably is minority of usage.

@henryrneh
Copy link

@cowtowncoder thanks for your reply and info! We will communicate the information and handle the application to Google. Many thanks!

@DavidKorczynski
Copy link

This issue was found by a fuzzer written by the Ada Logics team and is part of an ongoing security assessment. @henryrneh can you please ensure the issues you report are found by the fuzzers written by your team (https://github.com/google/oss-fuzz/blob/master/projects/jackson-core/JsonFuzzer.java and https://github.com/google/oss-fuzz/blob/master/projects/jackson-databind/ObjectReaderFuzzer.java) then we'll take care of those from our fuzzers.

@henryrneh
Copy link

I already canceled the application. We will do our best to try not to apply CVEs for fuzz targets written by AdaLogics, however we will need some assitance or notification by you to know who wrote which fuzz target, because OSS-Fuzz is not designed to support this, maybe you can use some special prefix in the fuzz target name so it's more obvious for us so we can filter it out?

@DavidKorczynski
Copy link

We will do our best to try not to apply CVEs for fuzz targets written by AdaLogics

Great, thanks!

we will need some assitance or notification by you to know who wrote which fuzz target

Do the links I provided above suffice?

@henryrneh
Copy link

henryrneh commented Sep 14, 2022

Thank you that works! In the future when AdaLogics add a new fuzz target please let us know or add some prefix to the name, so this will not happen again

@DavidKorczynski
Copy link

Thank you that works! In the future when AdaLogics add a new fuzz target please let us know or add some prefix to the name, so this will not happen again

sounds good -- I'll also send over an email after the assessment so you can see details about the findings we got using Jazzer

@henryrneh
Copy link

Great, thank you!

eperret added a commit to eperret/omakase that referenced this issue Oct 4, 2022
Updated to a new version of the com.fasterxml.jackson libraries to address CVE-2022-42004.  FasterXML/jackson-databind#3582

Cleaned up the code where the jackson is used
eperret added a commit to salesforce/omakase that referenced this issue Oct 5, 2022
Updated to a new version of the com.fasterxml.jackson libraries to address CVE-2022-42004.  FasterXML/jackson-databind#3582

Cleaned up the code where the jackson is used.
eperret added a commit to eperret/omakase that referenced this issue Oct 5, 2022
Updated to a new version of the com.fasterxml.jackson libraries to address CVE-2022-42004.  FasterXML/jackson-databind#3582

Cleaned up the code where the jackson is used
eperret added a commit to eperret/omakase that referenced this issue Oct 5, 2022
Updated to a new version of the com.fasterxml.jackson libraries to address CVE-2022-42004.  FasterXML/jackson-databind#3582

Cleaned up the code where the jackson is used
@cowtowncoder cowtowncoder changed the title Add check in BeanDeserializer._deserializeFromArray() to prevent use of deeply nested arrays Add check in BeanDeserializer._deserializeFromArray() to prevent use of deeply nested arrays [CVE-2022-42004] Oct 5, 2022
@cowtowncoder cowtowncoder added CVE Issues related to public CVEs (security vuln reports) and removed has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue labels Oct 5, 2022
@cowtowncoder cowtowncoder removed this from the 2.14.0 milestone Oct 5, 2022
@AppyS23
Copy link

AppyS23 commented Aug 9, 2023

Hello @cowtowncoder,
I have a question regarding the backporting of this fix to jackson-1 library.

Let me give some context. I am working to fix a vulnerability in our atlassian fork https://github.com/atlassian/jackson-1. While this is not actively developed, we still fix some vulns from time to time based on our synk scan. I have recently encountered two vulns related to this fix namely #3582 and #3590. While analysing the code, I have noticed that there are significant changes in _deserializeFromArray() function in BeanDeserializer. I am unsure if these changes can be backported at all or if this vulnerability can have a major impact on the older version? I would appreciate your expertise and suggestion on how should this be tackled.

Looking forward to your reply :)

@pjfanning
Copy link
Member

@AppyS23 as you well know, Jackson 1 is no longer supported. It has lots of bugs that have been fixed in Jackson 2. It is highly recommended that all users abandon Jackson 1 and using it is at your own risk. Your are on your own on this one. Changing from Jackson 1 to Jackson 2 is quite easy.

@AppyS23
Copy link

AppyS23 commented Aug 10, 2023

@pjfanning I understand and the more I analyse , it does seem like an awful lot of changes to fix this vulnerability. I will heed your advice and probably propose an upgrade. Thanks again :)

@yawkat
Copy link
Member

yawkat commented Aug 10, 2023

@AppyS23 jackson 1 is likely just as vulnerable to these attacks. whether these vulnerabilities can be considered "major impact" at all is another question, but i do not believe they got any worse in jackson 2.

@AppyS23
Copy link

AppyS23 commented Aug 10, 2023

@yawkat thank you for your opinion. In order to make changes to our atlassian fork of jackson-1 , I wanted to ensure this would be the right strategy. minor bugfixes are still done occasionally but this change looks major and it involves significant changes and additions. It would be rather beneficial to upgrade to jackson-2 and save ourselves the duplication efforts. The upgrade is planned but I am unsure of the timeline on when this would be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CVE Issues related to public CVEs (security vuln reports)
Projects
None yet
Development

No branches or pull requests

10 participants