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 primitive value deserializers to avoid deep wrapper array nesting wrt UNWRAP_SINGLE_VALUE_ARRAYS [CVE-2022-42003] #3590

Closed
cowtowncoder opened this issue Sep 6, 2022 · 44 comments
Labels
CVE Issues related to public CVEs (security vuln reports)
Milestone

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 6, 2022

TL;DNR:

Fix included in:

  • 2.14.0 once released (until then, 2.14.0-rc1 and rc2)
  • 2.13.4.2 micro-patch (jackson-bom 2.13.4.20221013). (NOTE: 2.13.4.1/2.13.4.20221012 have an issue that affects Gradle users)
  • 2.12.7.1 micro-patch (jackson-bom 2.12.7.20221012)

(note: similar to #3582 )
(note: originally found via oss-fuzz https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51020)

Implementation of methods like _parseBooleanPrimitive (in StdDeserializer) uses idiom:

            if (ctxt.isEnabled(DeserializationFeature.UNWRAP_SINGLE_VALUE_ARRAYS)) {
                p.nextToken();
                final boolean parsed = _parseBooleanPrimitive(p, ctxt);
                _verifyEndArrayForSingle(p, ctxt);
                return parsed;
            }

to handle unwrapping. While simple this exposes possibility of "too deep" nesting and possible problem with resource exhaustion in some cases. We should change this similar to how #3582 was handled.

@cowtowncoder cowtowncoder added the to-evaluate Issue that has been received but not yet evaluated label Sep 6, 2022
@cowtowncoder
Copy link
Member Author

cowtowncoder commented Sep 6, 2022

Methods to update (and regression test):

  • _parseBooleanPrimitive()
  • _parseBytePrimitive()
  • _parseShortPrimitive()
  • _parseIntPrimitive()
  • _parseLongPrimitive()
  • _parseFloatPrimitive()
  • _parseDoublePrimitive()
  • _parseDateFromArray() (maybe?)

Also note that method _deserializeWrappedValue() implements checks although cannot quite be called as-is.
Similarly, BeanDeserializer._deserializeFromArray() at around line 632 has similar checks.

@cowtowncoder cowtowncoder added 2.14 and removed to-evaluate Issue that has been received but not yet evaluated labels Sep 6, 2022
@cowtowncoder cowtowncoder changed the title Add check in primitive value deserializers to avoid wrapper array nesting wrt UNWRAP_SINGLE_VALUE_ARRAYS Add check in primitive value deserializers to avoid deep wrapper array nesting wrt UNWRAP_SINGLE_VALUE_ARRAYS Sep 7, 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=51020

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 Same as #3582 (see my comments there).

@henryrneh
Copy link

@cowtowncoder thank you 👍

@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

henryrneh commented Sep 12, 2022

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?

@pjfanning
Copy link
Member

@henryrneh the disclosure in https://nvd.nist.gov/vuln/detail/CVE-2022-42003 should really have been delayed until jackson-databind v2.14.0 was released. The RC should have been given a good chance to be tested and an orderly release of 2.14.0 done when it is ready. Now we risk having people clamouring for an early release of 2.14.0.

@DavidKorczynski
Copy link

@pjfanning -- @AdamKorcz and I applied for this CVE and was done in coordination with the current audit of jackson. We coordinated this with Tatu in terms of agreeing we should apply for CVEs for these issues.

@jgoeres
Copy link

jgoeres commented Oct 5, 2022

Our OWASP scan just ran into this finding for our use of jackson-databind 2.13.3. We are very hesitant to go to an RC of 2.14 (and of course fully understand that this issue cannot change the release plans for 2.14, so we will not join the crowd that is pushing for an "early release of 2.14" @pjfanning :-)).
But since 2.13 is an actively maintained branch according to https://github.com/FasterXML/jackson#active-developed-jackson-2x-branches, I assume a fix will be made available there as well? Any activities or maybe even an ETA for that?

@pjfanning
Copy link
Member

pjfanning commented Oct 5, 2022

@jgoeres @alzimmermsft are you even using the UNWRAP_SINGLE_VALUE_ARRAYS option?

@alzimmermsft
Copy link

alzimmermsft commented Oct 5, 2022

I'll second what @jgoeres, that we're getting security alerts around usage of Jackson 2.13.4 and won't be able to upgrade to the 2.14 RC any time soon and downstream consumers of our SDKs will also get alerts as well. We aren't using UNWRAP_SINGLE_VALUE_ARRAYS but a lot of noise will still be caused.

Edit:

I know that the fix for this issue now results in an exception being thrown, which is a runtime breaking change within a patch release, but there has been prior art on this being accepted with the fix for #2816. There was also API changes as well, though looking at the fix for this issue the shared method could be made non-protected if it's backported and shouldn't be exposed.

@pjfanning
Copy link
Member

@pjfanning -- @AdamKorcz and I applied for this CVE and was done in coordination with the current audit of jackson. We coordinated this with Tatu in terms of agreeing we should apply for CVEs for these issues.

Just watch the comments flow in looking for special attention now this change is made public. I've already had swagger-akka-http/swagger-akka-http#342

@jgoeres
Copy link

jgoeres commented Oct 5, 2022

@pjfanning
We have roughly dozen microservices spread across as many teams, we haven't checked for all of them. But even if none of them used that setting, scans done in various places (both within our organization, but also by customers that run this on-prem) will find the lib and this will lead to irritation and demands to resolve it by updating even if we claim that it is not exploitable.

@cowtowncoder cowtowncoder changed the title Add check in primitive value deserializers to avoid deep wrapper array nesting wrt UNWRAP_SINGLE_VALUE_ARRAYS Add check in primitive value deserializers to avoid deep wrapper array nesting wrt UNWRAP_SINGLE_VALUE_ARRAYS [CVE-2022-42003] Oct 5, 2022
@cowtowncoder cowtowncoder added the CVE Issues related to public CVEs (security vuln reports) label Oct 5, 2022
@cowtowncoder
Copy link
Member Author

@DavidKorczynski I think we need coordinate better in future: my understanding was that a CVE would be filed, I'd be notified with id allocated (so I can update this issue, release notes), but CVE only made public once official release was out.
But perhaps I am assuming there is a mechanism that does not exist between allocating an id and making it available for the public.

What is needed at minimum, at any rate, is the linkage between CVE and issues. The problem right now is that users email me or file issues asking "what's up with CVE-2022-xxxx" and then I have to go digging. It's not a huge effort of course but is an interruption, and something we can avoid by collaboration.

We may also need to discuss some of finer details: in this case I hope I mentioned (but perhaps I didn't?) that this setting is disabled by default -- that should have an effect in ranking.

I realize I did not quite realize everything that needs to be discussed before I say it's fine to proceed with CVE filing.
Live and learn. Thank you everyone.

@cowtowncoder
Copy link
Member Author

Note: also added the "sibling" CVE marker (2022-42004) for #3582.

odl-github pushed a commit to opendaylight/odlparent that referenced this issue Nov 25, 2022
FasterXML/jackson-databind#3590
FasterXML/jackson-databind#3627

Change-Id: If1bf01ca0a05772e140427fe2164dde05be264d7
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
odl-github pushed a commit to opendaylight/odlparent that referenced this issue Nov 25, 2022
FasterXML/jackson-databind#3590
FasterXML/jackson-databind#3627

Change-Id: If1bf01ca0a05772e140427fe2164dde05be264d7
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 36012dd)
odl-github pushed a commit to opendaylight/odlparent that referenced this issue Nov 25, 2022
FasterXML/jackson-databind#3590
FasterXML/jackson-databind#3627

Change-Id: If1bf01ca0a05772e140427fe2164dde05be264d7
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 36012dd)
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