jackson-databind: Stack overflow error caused by serialization of `Map` with cyclic dependency -- NOT CVE

Stack overflow error caused by jackson serialization Map

Description

jackson before v2.15.2 was discovered to contain a stack overflow via the map parameter.

Error Log

Exception in thread "main" java.lang.StackOverflowError
	at java.base/java.lang.String.startsWith(String.java:1470)
	at com.fasterxml.jackson.databind.util.ClassUtil.isJDKClass(ClassUtil.java:1163)
	at com.fasterxml.jackson.databind.introspect.BasicClassIntrospector._findStdTypeDesc(BasicClassIntrospector.java:258)
	at com.fasterxml.jackson.databind.introspect.BasicClassIntrospector.forSerialization(BasicClassIntrospector.java:80)
	at com.fasterxml.jackson.databind.introspect.BasicClassIntrospector.forSerialization(BasicClassIntrospector.java:11)
	at com.fasterxml.jackson.databind.SerializationConfig.introspect(SerializationConfig.java:906)
	at com.fasterxml.jackson.databind.ser.BasicSerializerFactory.createKeySerializer(BasicSerializerFactory.java:210)
	at com.fasterxml.jackson.databind.SerializerProvider.findKeySerializer(SerializerProvider.java:915)
	at com.fasterxml.jackson.databind.SerializerProvider.findKeySerializer(SerializerProvider.java:926)
	at com.fasterxml.jackson.databind.ser.impl.PropertySerializerMap.findAndAddKeySerializer(PropertySerializerMap.java:144)
	at com.fasterxml.jackson.databind.ser.std.StdKeySerializers$Dynamic._findAndAddDynamic(StdKeySerializers.java:284)
	at com.fasterxml.jackson.databind.ser.std.StdKeySerializers$Dynamic.serialize(StdKeySerializers.java:262)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:797)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)



PoC

        <dependency>
            <groupId>com.fasterxml.jackson.core</groupId>
            <artifactId>jackson-databind</artifactId>
            <version>2.15.2</version>
        </dependency>
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;

import java.util.HashMap;

public class PoC2 {

    public static void main(String[] args) {
        HashMap<String,Object> map=new HashMap<>();
        map.put("t",map);
        ObjectMapper objectMapper = new ObjectMapper();
        try {
            String jsonString = objectMapper.writeValueAsString(map);
            System.out.println(jsonString);
        } catch (JsonProcessingException e) {
            e.printStackTrace();
        }
    }
}

Rectification Solution

  1. Refer to the solution of jackson-databind: Add the depth variable to record the current parsing depth. If the parsing depth exceeds a certain threshold, an exception is thrown. (https://github.com/FasterXML/jackson-databind/commit/fcfc4998ec23f0b1f7f8a9521c2b317b6c25892b)

  2. Refer to the GSON solution: Change the recursive processing on deeply nested arrays or JSON objects to stack+iteration processing.((https://github.com/google/gson/commit/2d01d6a20f39881c692977564c1ea591d9f39027))

References

  1. https://github.com/jettison-json/jettison/issues/52
  2. https://github.com/jettison-json/jettison/pull/53/files

About this issue

  • Original URL
  • State: closed
  • Created a year ago
  • Reactions: 36
  • Comments: 76 (43 by maintainers)

Commits related to this issue

Most upvoted comments

Whoever filed CVE https://www.mend.io/vulnerability-database/CVE-2023-35116 should go fuck themselves.

@PoppingSnack I assume you did not file this – given that as a project we do not agree in assessment. But there are so many bottom-feeding security “researchers” that it is more likely someone simply saw this issue report (which is not an invalid thing to report at all!) and decided to score points for filing an CVE. Which is why this whole security issue theater is so very annoying; lots of incompetent and/or malicious actors not following the process and trying to short-circuit modest checks there are.

This happens with any recursive data structure. Jackson will not detect loops in data structures. For example:

    public static void main(String[] args) throws JsonProcessingException {
        A a = new A();
        A b = new A();
        a.next = b;
        b.next = a;
        new ObjectMapper().writeValueAsString(a); // will fail
    }

    static class A {
        A next;

        public A getNext() {
            return next;
        }
    }

imo this is not really an issue. For example, calling hashCode on the same map would also cause a stack overflow, but this is not considered a security issue. You can’t construct such a data structure using jackson deserialization. Others may disagree on this though.

BeanSerializer has some error handling to make the error a bit nicer, but it’s still a stack overflow with all the issues associated with that.

This vulnerability allows attackers to cause a Denial of Service (DoS) via a crafted string.

Your test case does not demonstrate this, can you please show how this can be exploited via a crafted string?

This is laughable. Who’s gonna file a CVE against JDK Map.hashCode which has the same “issue”?

Our dependency check jobs started failing again.

https://nvd.nist.gov/vuln/detail/CVE-2023-35116#VulnChangeHistorySection

Initial Analysis by NIST 6/26/2023 12:52:40 PM

Action Type Old Value New Value
Added CPE Configuration OR *cpe:2.3:a:fasterxml:jackson-databind:*:*:*:*:*:*:*:* versions up to (including) 2.15.2
Added CVSS V3.1 NIST AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
Added CWE NIST CWE-502
Changed Reference Type https://github.com/FasterXML/jackson-databind/issues/3972 No Types Assigned https://github.com/FasterXML/jackson-databind/issues/3972 Exploit, Issue Tracking

Congratulations to NIST looks like they are unable to read this issue properly and are also a week late. Maybe they should consider partnering with Internet Explorer. Now they also classified it as affecting all Jackson Databind versions… and according to them this issue contains an exploit… Yeah sure I also always copy java code into the system I want to exploit.

Also the CVSS score makes zero sense: https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?name=CVE-2023-35116&vector=AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H&version=3.1&source=NIST

  • Attack vector = Network ?
  • Attack complexity = Low ?

If someone has the motivation they may contact NIST and try to get it revoked. I’m not motivated anymore and going to file an issue at my company that we disable CVE scanning and just keep our dependencies up-to-date because this is a clown show.

For those coming from https://github.com/jeremylong/DependencyCheck:

So I just came in this morning and was a bit surprised to see that all our projects with the dependency - even with up to date versions - are marked as vulnerable.

The CVE is still tracked inside Sonatype’s OSS INDEX (which is used by DependencyCheck) - I wrote them a mail to get this fixed.

See also: https://github.com/jeremylong/DependencyCheck/issues/5779

The person who filed this CVE should really think long and hard about whether their personal pride was worth all the needless work people around the world now have to spend on suppressing this CVE in countless projects. Well done.

Ok I gave too much benefit of doubt I guess.

But fundamentally beyond blatant disregard to project accepting a report as vulnerability, reproduction here is not anything an attacker could do as shown. It is a standalone Foot Gun example in which developer of a service can DoS their own service. Ridiculous. And description still makes baseless claim of “crafted String” – while there is nothing of the sort in alleged reproduction.

So this is not in any way shape or from legit basis for a CVE: there is no attack vector for malicious clients.

@PoppingSnack I assume you did not file this

I wouldn’t be so sure…

20230618_10h49m26s_grim

@pjfanning Thanks! Is there anything we can do to help get CVE-2023-35116 revoked?

I sent a request to Mitre using https://cveform.mitre.org/

I don’t know if it is worth having multiple people chime in. I’ll update this issue thread with any feedback from Mitre. There is no guarantee that they’ll listen to me. A very high fraction of the CVEs are very low quality in terms of what they are reporting and often when they document real issues, the CVE description itself is still vague or inaccurate.

I do believe this needs to be fixed. Any REST application receives json data from potentially malicious users. There is no mechanism to test the validity of that json data outside the context of the json parser. It is the responsibility of the parser to detect malicious json data. The example provided above by @yawkat with a.next and b.next pointing to each other is not the same thing. That would be the developer’s fault, not the fault of a malicious actor. If it is possible for a malicious actor to craft a data structure that when supplied to a RESTful service, crashes that service, then there is a security vulnerability. If the json parsing framework is not capable of detecting this without stack overflow and crashing, then the json parsing framework cannot be trusted. This is my two cents and not meant to be rude or offensive to anyone. Please correct me where I’m wrong here.

Some minor positive update: NIST has upgraded description at https://nvd.nist.gov/vuln/detail/CVE-2023-35116 to include:

NOTE: the vendor’s perspective is that this is not a valid vulnerability report, because the steps of constructing a cyclic data structure and trying to serialize it cannot be achieved by an external attacker

which is accurate. We do not consider this a valid vulnerability.

Having said that, SHOULD there be a way to add cyclic dependency, serialization would be blocked by:

https://github.com/FasterXML/jackson-core/pull/1055

to be included in Jackson 2.16.

I sent the NVD a message through their contact form, 🤞

Joining here from https://github.com/aws/aws-sdk-java-v2 and https://github.com/aws/aws-sdk-java. I understand this is not exploitable, but our customers demand that we upgrade to a “fixed” version. We’re “stuck” on 2.12. @cowtowncoder would you accept a pull request from us backporting fcfc499, and also do us the favor of releasing a 2.12.7.2 with the “fix”?

Sorry, would not accept a PR. Like @pjfanning said above, change that will catch too deep nesting will go in 2.16 and cannot be backported due to requiring API additions. Not even to 2.15, much less earlier.

And 2.16 itself is months out. The only short term way to resolve this problem is to get rid of CVE.

@cowtowncoder I tried to change from the ‘inside’ when I was on the CVE Editorial Board for 10 years. They are well aware of many, many problems in the program. One problem is the board is full of a few that were actively trying to change things for the better, a lot of vendors that only cared about the program as how it affected them, and a ton of lurkers. Since then, the program has gotten dramatically worse in many ways.

Alas, project owners are at mercy of CVE maintainers, who can update state and descriptions. It puzzles me, for example, how “project has the view that the library is not to be used for untrusted content” came about – this is not the gist of what is being said here at all.

Rather, the project team considers this an invalid report: there is no valid reproduction of a security vulnerability.

@cowtowncoder @ajakk it most likely is that @PoppingSnack user - see https://github.com/janino-compiler/janino/issues/201 - another CVE raised despite the Janino team explaining that that tool is not designed for untrusted input - there has to be a point at which lib maintainers can say that users have to understand that they have to police against malicious input themselves

I’m a bit late to the party. So, I recently faced a security vulnerability for this issue in one of my Docker images. When can we expect for CVE-2023-35116 to get revoked?

We have no control over the CVEs. Talk to whoever provides your security analysis tool.

@PoppingSnack here is a cubic foot of stainless steel. Why don’t you check if anything is vulnerable to dropping it from one yard of height in standard atmospheric conditions. Report back when ready with data to support your findings.

@tonyschwartz That statement made by submitted of CVE is indeed factually incorrect; reproduction included fails to show alleged vulnerability. And beyond that, the burden of proof really has to be with submitter: there has been radio silence on anything related to actual solid reproduction of vulnerability. This feels more Drive-By-Bug-Reporting with no follow up. And it is not an isolated instance either (there was a companion report for this one (#3973) similarly invalid).

I have tried to make Mitre change the description as have others – to no avail. In fact, the description became even worse (but I digress). Mitre folks do not really seem to pay much attention (due to lack of resources etc) to correctness of reports; I assume they think that reporters and maintainers should somehow sort it all out without their involvement. The whole system is rotten to its core.

I suggest you read, say https://daniel.haxx.se/blog/2023/09/05/bogus-cve-follow-ups/ (if you haven’t) – it sounds like you may overestimating integrity (and perhaps competence) of people submitting CVEs. I do not want to rehash everything said there but agree 100%.

Literally anyone, anywhere, can file bogus CVEs and have a reasonable change of that being published and causing lots of FUD without being based on anything solid.

@tonyschwartz

If it is possible for a malicious actor to craft a data structure that when supplied to a RESTful service, crashes that service, then there is a security vulnerability

It is not possible to construct such a data structure from json.

Yes, I have filed CVEs myself: that is not problematic. It is also not the case that anyone SHOULD be able to file an issue without co-operating with maintainers – it is the problem that while they SHOULD NOT, some still do. What happens, basically is that either:

  1. Someone reports an alleged vulnerability and immediately files CVE (“we did notify maintainers!”) but without waiting for any response
  2. Someone just, well, files CVE without anything – often based on a Github issue they think implicates a vulnerability.

So basically CVE filing is an honor system as things are (AFAIK). Becoming a CNA would, I think, not necessarily even prevent filing but give ability to modify CVEs to indicate they are valid. And possibly in some cases make Mitre even prevent the original filing.

@OrangeDog I suspect that orgs maintaining reports are bit like patent office; overworked and with too little resources to investigate in detail, mostly relying on reporters’ competency and good will. And where last 2 are lacking results are like here. 😦

An inaccurate CVE in the database is OK, at least people can investigate and ignore it. But now it’s lying about what “the vendor” said, which is much worse. How did that even happen? Just massive incompetence?

Our dependency check jobs started failing again. Is there any reasonable action other than to exclude this in the dep check configuration?

the CVE has been updated to say ‘DISPUTED’ but they made the text worse by adding ‘NOTE: the vendor’s perspective is that the product is not intended for use with untrusted input.’

This is absolutely not the case, this issue is about output not input. We have enhancements to police for malicious input.

https://github.com/CVEProject/cvelist/commit/e7481e1d5efe3ddea0cdb01706056b94f4bfb11e#diff-1d23b5f10a1be30eac5d043258c91f067a6f7e0cc86e7c6fc3e5c7462d6323f9

@cowtowncoder I guess the pressure to make a change is going to come on - even if we do successfully engage with whoever assigned CVE-2023-35116 to get it revoked.

There are lots of ways to create data structures where you can get infinite recursion.

  • Collections where one of the values is the top level Collection itself
  • Classes with a parent type where the parent has already been seen. The simplest is to have an instance whose parent is itself. Can also be done with child types or sibling types.

The serialize APIs tend to take thse params - (T value, JsonGenerator gen, SerializerProvider provider).

So as not to have to change the APIs to sneak in another value in the param list, could we consider tracking this in the JsonGenerator? JsonWriteContext looks like a good place to track the depth. protected int _nestingDepth is already inherited from JsonStreamContext.

Specifically, we need to track the depth - how many levels down have we gone since the we started serializing the original instance?

I have https://github.com/FasterXML/jackson-core/pull/1055 open for discussion - a partial implementation.

This could also be tracked via the SerializerProvider. Again, we could track the depth. Alternatively, we could keep track of the values, so that if we are asked to serialize a child value, we can check if we have already serialized that value. I think depth is a safer approach - value equality is something that is hard to rely - some classes will have equals() implementations that will not suit our needs.

I see. So, I’m a bit confused on the CVE then. I will study.

What I meant was, maybe we should try to convince Mitre (and any other CNAs such as Snyk) that they stop filing CVEs for GitHub repositories by default[^1][^2], and that issuing a CVE for a GitHub repo becomes a rare exceptional thing they do. So they only do it in cases of arbitration or for inactive maintainers; e.g. if a public GitHub issue mentioning a vulnerability without going into details and asking for contact details remains unanswered for a long time.

And that if someone approaches Mitre then and says “I would like a CVE for this vulnerability in GitHub repository XYZ”, that they respond with something like this:

Please contact the repository maintainer directly; check their “Security” page or contribution guide for contact details, or open an issue asking for contact details. The maintainers of XYZ can then request the CVE themselves. If they don’t know how they can do that, refer them to the GitHub documentation.

Also as side note, recently for curl a bogus CVE was issued as well; it appears they consider becoming their own CNA now as well. But it is really not an ideal situation that maintainers have to become their own CNAs; the proper solution might be really if Mitre changed their behavior.

[^1]: Possibly except for cases where a GitHub repository is clearly only a mirror of a Git repository hosted somewhere else. [^2]: Or of course unless those CNAs work together with the GitHub repo maintainers, for example when the maintainers are using HackerOne, which is also a CNA.

This was definitely filed by @PoppingSnack as shown on Snyk Credit for https://security.snyk.io/vuln/SNYK-JAVA-DEGROBMEIERJSON-5710359

Most likely it was reported via Snyk as well as they are also a CNA: https://snyk.io/blog/vulnerability-disclosure-program/

Is it false positive?

Yes, please refer to discussions above 👍🏼

@yawkat I haven’t heard back from Mitre

@millems CVE-2023-35116 is not an issue. We reject that it is a security issue. There is nothing committed to jackson yet but we do intend to put some hardening in place in Jackson 2.16.0 - a release that is many months away. This fix will not be backported to any previous Jackson versions. https://github.com/FasterXML/jackson-core/pull/1055 is the proposed PR. To reiterate, this includes new APIs and will not be backported.

@pjfanning Thank you for tackling that jackson-core issue that I filed earlier: I think that is the best way to go, based on similar thing on reader-side included in 2.15. There may still be reasons to tackle actual cyclic part (there have been PRs in the past proposing ways to keep track of parent object stack), but while related I think this one would tackle SO aspect reliably and efficiently.

(cycle tracking could actually possible be done at streaming level since there is hierarchic tracking of “current object” already… but that is still more complicated and adds significant overhead for deeper object hierarchies (so probably opt-in feature); whereas simple counter approach can be enabled for all users by default)

yes, we’ve discussed extensively that the CVE is wrong, but apparently we can’t do anything about it.

I suggested in https://github.com/github/advisory-database/issues/2718 to have GitHub extend its CNA scope, possibly similar to what GitLab is doing for their platform. Maybe that could improve the situation as well, but let’s see what the GitHub employees think about this.

I have not heard anything further for my submission via https://cveform.mitre.org/ . I assume anyone is free to file other requests, although we do not want to spam them and slow down processing.

We have also noted a reason for the dispute

Did you not point out that they reason they have noted is a lie?

I sent the NVD a message through their contact form, crossed_fingers

MITRE maintains the CVE data, not NVD. NVD is really just a frontend for that data. You need to talk to MITRE to get a CVE revoked or changed or anything:

https://cveform.mitre.org

Ugh. That is quite unfortunate. I would not agree that Jackson should not be used for untrusted input as a general rule: if you (f.ex) use mapper.readTree() or even readValue() without default typing, there are no current exploits to be concerned about.

Going forward I guess we should refrain from mentioning “do not use for untrusted input” in general: it is good advice for specific cases but I guess can be misread as general statement, instead of specific case suggestion.

@millems fyi the supported versions are listed here: https://github.com/FasterXML/jackson/wiki/Jackson-Releases – sounds like you should indeed upgrade.

(it incorrectly says 2.12 was closed in April 2022 when it was closed in April 2023, @pjfanning can you fix this? I don’t have access.)

@pjfanning Thanks! Is there anything we can do to help get CVE-2023-35116 revoked?

Also quick note on Rectification Solutions: Jackson 2.15 does both – default maximum nesting depth on reader side is set at 1000 levels, and List / JsonNode deserializers both have non-JDK-stack based handling.