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

Optimize UntypedObjectDeserializer wrt recursion [CVE-2020-36518] #2816

Closed
cowtowncoder opened this issue Aug 13, 2020 · 77 comments
Closed

Optimize UntypedObjectDeserializer wrt recursion [CVE-2020-36518] #2816

cowtowncoder opened this issue Aug 13, 2020 · 77 comments
Labels
CVE Issues related to public CVEs (security vuln reports) most-wanted Tag to indicate that there is heavy user +1'ing action performance Issue related to performance problems or enhancements
Milestone

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 13, 2020

EDIT: related to to CVE-2020-36518 (see https://nvd.nist.gov/vuln/detail/CVE-2020-36518)

EDIT: Fix included in

EDIT: included as one of Snyk's "top-10 vulns of 2022" CVEs -- see https://go.snyk.io/snyk-top-10-open-source-vulnerabilities-dwn-typ.html


Current implementation UntypedObjectDeserializer is relatively expensive for deeply nested Object and Array values as it uses recursion even for "vanilla" case (one where there are no custom List/array or Map deserializers).
In practical terms it is possible to exhaust typical modest JVM memory with documents having about ten thousand levels of nestings, due to size of call stack from recursive calls.

NOTE: specifically this ONLY APPLIES if the target type is "untyped" or generic Collection<Object> / Map<String, Object> -- it DOES NOT APPLY to cases where target is POJO (except if POJO itself has "untyped" property or properties).

Similar issue was already solved wrt JsonNode (see #3397), included in 2.13.0; this might show a way to approach this problem: by replacing simple recursion with iteration, either completely or at some inner levels.

Also note that it may ultimately be necessary to have lower-level constraints for streaming parser too, see: FasterXML/jackson-core#637

Ideally it should be:

  1. Possible to handle at least tens of thousands of levels of nesting (100k should be processable with 256M heap, say)
  2. Have streaming level limits that -- by default -- block documents with more than limit we deem safe (less than 100k -- perhaps 10k or something, to be determined).

This issue is specifically about (1) as (2) is about jackson-core.

cowtowncoder added a commit that referenced this issue Aug 13, 2020
@cowtowncoder
Copy link
Member Author

Looks like nesting of about 2000 works, 3000 fails, with default settings for both Arrays and Objects.

cowtowncoder added a commit that referenced this issue Aug 13, 2020
@cowtowncoder cowtowncoder added the performance Issue related to performance problems or enhancements label Aug 23, 2020
@cowtowncoder
Copy link
Member Author

Tagging as "performance" (since resource consumption of call stack causes the exception). Also added similar test for JsonNode. Hoping to eventually tackle for these types but realistically support from streaming parser (jackson-core) is probably needed (will file separate ticket) for more general complexity limits for input documents.

@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Oct 27, 2020
@cowtowncoder cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label Feb 20, 2021
@cowtowncoder cowtowncoder added 2.14 most-wanted Tag to indicate that there is heavy user +1'ing action and removed 2.13 labels Jul 15, 2021
@cowtowncoder
Copy link
Member Author

cowtowncoder commented Feb 10, 2022

FWTW, similar issues does NOT affect JsonNode, as of Jackson 2.13.0 and later on.
Should have created separate issue to point to, but iterate version of JsonNodeDeserializer was merged in at around March 2021.
Would be great to achieve the same here for 2.14.

Could start with UntypedObjectDeserializer.Vanilla first, see if generalizable to full case (should be).

@meier-th
Copy link

@cowtowncoder , could you please clarify if the same fix is planned to be backported to the versions 2.12.x and 2.11.x of Jackson?

@cowtowncoder
Copy link
Member Author

cowtowncoder commented Mar 13, 2022

EDIT: fix actually backported, see initial Description (now Edited as well) for details.


@meier-th Very unlikely that I would try to backport this to anything prior to 2.14.0. The new implementation probably has non-trivial chance of regression and this functionality is widely used. Note that no implementation yet exists, but I fully intend to work on this to try to get it done for 2.14.0 (or someone else could beat me to it).

@AlvinYueChao
Copy link

@meier-th I'm sure that the same issue existing in 2.12.x and 2.11.x. May I know the fixing plan ? if the issue can only be fixed by upgrading to 2.14.x, when do you plan to release this version ?

@cowtowncoder
Copy link
Member Author

cowtowncoder commented Mar 14, 2022

Oh fucking great. Someone decided to file an CVE for this one.

Surely teaches me to file issues on things I want to work on -- and then some Very Nice Person going to file an CVE to freak out everybody.

@wsdng
Copy link

wsdng commented Mar 16, 2022

I recommend that you simply throw some error if the nesting is to deep to cope with. You could improve the nesting depth later on, but you (and we users) will get rid of this annoying cve.

dbyron-sf added a commit to dbyron-sf/kork that referenced this issue Jun 13, 2022
mergify bot pushed a commit to spinnaker/kork that referenced this issue Jun 14, 2022
akatona84 pushed a commit to akatona84/jackson-databind that referenced this issue Jul 26, 2022
@cowtowncoder cowtowncoder modified the milestones: 2.13.2.1, 2.13.3 Nov 15, 2022
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) most-wanted Tag to indicate that there is heavy user +1'ing action performance Issue related to performance problems or enhancements
Projects
None yet
Development

No branches or pull requests