Throw JsonMappingException for deeply nested JSON (#2816, CVE-2020-36518)

[TaylorSMarks]

By throwing a checked exception instead of a runtime exception, it's less likely that Jackson's issues will bring down the caller's code in unexpected ways. I think this might be good enough to consider the CVE resolved.

I copied over the existing failing test and modified it so that it passes if either the original assertion passes or if the test throws a JsonMappingException.

[cowtowncoder]

While I appreciate all help, I don't think this is a good approach: at the time we get the StackOverflowError the damage is already done; resources consumed.
So as far as I see it this will not really address the real issue unfortunately.

I hope to start digging into better solution soon, based on earlier work for JsonNode handling.

[TaylorSMarks]

@cowtowncoder - This PR has changed from just catching a Stack Overflow Error and rethrowing a JsonMappingException to instead tracking the nesting depth and throwing if it's "Too Deep".

256 was chosen as the max depth. It's mostly arbitrary. I looked into the maximum folder depth allowed on different platforms - it's less than 130 on Windows, less than 512 on macOS. I don't hear people complaining that they can't nest their folders any deeper on Windows, so... I figured 256 was a safe maximum depth for json.

The unit tests were updated. It'll verify that 250 nested arrays or objects will deserialize properly. It verifies that 300 nested arrays or objects throws a JsonParseException.

If you're happy with this, then I look forward to seeing 2.13.3 or 2.13.2.1 or whatever you choose to name it being released soon.

[cowtowncoder]

Very cool @TaylorSMarks. I'll have to look into this more, obviously, but in general it makes sense.
Thank you very much for contributing this.

One quick note: 256 is likely too low; should be at least 1000. If you have a way to check for specific heap configuration, that could maybe suggest some other limit; otherwise let's go with 1000.

One practical thing is the CLA: unless I have one from you (apologies if I forget), it'd be this:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(or, if you prefer, CCLA that's next to it)

and the usual way is to print, fill & sign, scan/photo, email to info at fasterxml dot com.

I'll only need it to merge, I can start reviewing etc before.

[TaylorSMarks]

I pushed another commit just updating MAX_DEPTH from 256 to 1000 and updating the test accordingly. I’ve pinged a few people at the company regarding the agreement… I’m not sure whether I’m authorized to sign it or who would be the right person to do so.

…

On Mar 17, 2022, at 17:45, Tatu Saloranta ***@***.***> wrote:  Very cool @TaylorSMarks. I'll have to look into this more, obviously, but in general it makes sense. Thank you very much for contributing this. One quick note: 256 is likely too low; should be at least 1000. If you have a way to check for specific heap configuration, that could maybe suggest some other limit; otherwise let's go with 1000. One practical thing is the CLA: unless I have one from you (apologies if I forget), it'd be this: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf (or, if you prefer, CCLA that's next to it) and the usual way is to print, fill & sign, scan/photo, email to info at fasterxml dot com. I'll only need it to merge, I can start reviewing etc before. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

[TaylorSMarks]

I've pinged a dozen of so people at the company regarding the agreement. Nobody who has responded actually knows who is appropriate to sign the document. My manager told me to just wait until Monday morning - if we don't hear back from anyone by then, we'll just say that I did this as an individual and not as a company employee (this is my personal github account linked to my personal email anyways) and I can sign it without involving the company.

[cowtowncoder]

@TaylorSMarks sounds good, apologies for the hassle. Thank you very much for working to get this resolved -- otherwise things would have taken much longer.

Monday is fine from my perspective, although various users are freaked out due to the FUD nature of CVE/Vuln handling that tool vendors cause with reports like this.
(I still maintain this should not be high severity at all, given it is not a dominant use case... but good luck getting that acknowledged).

[lookitstony]

Very interested in this fix as it has our release at risk due to the CVE. If there is anything I can do to assist please let me know. Nice work on the quick turn around @TaylorSMarks!

[cowtowncoder]

@lookitstony thank you! Also: it may be worth checking whether this vulnerability is applicable for your usage.
I think that I can get this reviewed & give feedback and then once CLA is received, micro-patch is not a difficult thing to do.

I know that scanning tools are Dumb (woe is me) and have no concept of conditional/contextual applicability, but developers may be able to determine this relatively easily.
For this to apply, target type for ObjectMapper.readValue() MUST be Object.class, or there needs to be NON-polymorphic property/ies of type java.lang.Object. This is not unusual thing to do, but is not a majority/dominant use acse either.

[TaylorSMarks]

Alright - we pursued several leads but none of them lead anywhere. Nobody at the company is sure whether it's appropriate for us to mention the company in the agreement, so I went ahead and signed without filling in any company info, as I would have done in the first place had I not done it on company time. I sent it to the email address @cowtowncoder specified above with the subject "TaylorSMarks Signed Contributor Agreement for Jackson-databind".

I did include a coworker on the agreement since he worked with me on the changes. Just so you're not surprised/confused by his name showing up on the pdf.

[yawkat]

@cowtowncoder are you still looking for an iterative version of the UntypedObjectDeserializer? I can work on that. (I have a CLA already :D)

[cowtowncoder]

@yawkat Yes, I am looking for an iterative version for 2.14. I like the idea of simple(r) counter-based approach for 2.13, and then iteration for 2.14 where we get bit more testing (including performance testing/verification).

PR would of course be awesome! And JsonNodeDeserializer has prior art if that helps; maybe not.

[lookitstony]

@lookitstony thank you! Also: it may be worth checking whether this vulnerability is applicable for your usage. I think that I can get this reviewed & give feedback and then once CLA is received, micro-patch is not a difficult thing to do.

I know that scanning tools are Dumb (woe is me) and have no concept of conditional/contextual applicability, but developers may be able to determine this relatively easily. For this to apply, target type for ObjectMapper.readValue() MUST be Object.class, or there needs to be NON-polymorphic property/ies of type java.lang.Object. This is not unusual thing to do, but is not a majority/dominant use acse either.

Thanks for the great response. This is helpful.

[weburnit]

@cowtowncoder can you release this version?
synk doesn't allow us to release!
thanks.

[cowtowncoder]

@weburnit As soon as I have to time to make a release I will do this. Trying to ping me will only use more time if I have to response. Patience my friend.

[pjfanning]

@cowtowncoder @TaylorSMarks would it make sense to add a public static method to UntypedObjectDeserializer.java that allows users to set a different limit (but still with 1000 default)?

[TaylorSMarks]

Given this is a temporary hotfix that’ll go away in 2.14, I think it’s good enough without that value being configurable. If there were an API to adjust the value, that just makes the later implementation in 2.14 more difficult (or backwards incompatible.) Taylor

…

On Mar 23, 2022, at 21:49, PJ Fanning ***@***.***> wrote:  @pjfanning commented on this pull request. In src/main/java/com/fasterxml/jackson/databind/deser/std/UntypedObjectDeserializer.java: > @@ -661,6 +661,10 @@ protected Object mapObject(JsonParser p, DeserializationContext ctxt, { private static final long serialVersionUID = 1L; + // Arbitrarily chosen. + // Introduced to resolve CVE-2020-36518 and as a temporary hotfix for #2816 + private static final int MAX_DEPTH = 1000; @cowtowncoder @TaylorSMarks would it make sense to add a public static method to UntypedObjectDeserializer.java that allows users to set a different limit (but still with 1000 default)? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[cowtowncoder]

Version 2.13.2.1 of jackson-databind now released: usable using version 2.13.2.20220324 of jackson-bom.

[akatona84]

Hi @cowtowncoder, do you guys planning to make a patch release with this CVE fix for 2.10.5 (which would be 2.10.5.2)?
I tried the backporting and it was almost a clean cherrypick (see that on my fork).

[pjfanning]

@akatona84 this fix is available in v2.12.6.1 and v2.13.2.1. Have you tried upgrading? If you upgraded and hit trouble, can you describe the trouble? Backporting to arbitrary old versions of jackson is time consuming.

[cowtowncoder]

@akatona84 No, I have no plans for further 2.10.x releases and this is quite sizable change -- no reports of breakages, but I am quite hesitant wrt backporting fixes like this. Same goes for 2.11, so 2.12.7 or 2.13.3 are your best bets.

[akatona84]

@pjfanning.
I tried to upgrade jackson in our softwares (including kafka 2.5, cruise control and one with dropwizard in it) long time ago (Jan of 2021). When I changed 2.10.5 to 2.12.x for kafka and cruise control, it was pulling in newer scala in cruise control and broke the build. I stopped at this point saying let's upgrade the whole kafka first to 2.7+ (where jackson was already 2.12) and then cruise control too (dunno the version, whatever was working with 2.7+ kafka).

(I was also upgrading jetty, jersey and dropwizard versions with jackson and I can't exactly recall how these were entangled, just remembering that I couldn't just upgrade jackson itself.)

Thanks @cowtowncoder,
We're doing a maintenance kindof release (bugfixes and CVE fixes) and we can't upgrade kafka in it, that's why I asked if we'll ever have a 2.10.x patch. I had to be sure that this CVE won't be fixed in older versions.

Thanks for the info and the quick response from both of you! 👍

[cowtowncoder]

@akatona84 thanks, yes, that makes sense. I can relate to challenges in upgrading components, with all the transitive dependencies, versions, even minor differences in one place can have big cascading effects and challenges.