Stack overflow error caused by serialization of Map with cyclic dependency -- NOT CVE

[PoppingSnack]

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. (fcfc499)

2. Refer to the GSON solution: Change the recursive processing on deeply nested arrays or JSON objects to stack+iteration processing.(（google/gson@2d01d6a20f39881c692977564c1ea591d9f39027）)

References

1. If the value in map is the map's self, the new new JSONObject(map) cause StackOverflowError which may lead to dos jettison-json/jettison#52
2. https://github.com/jettison-json/jettison/pull/53/files

[yawkat]

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?

[cowtowncoder]

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.

[pjfanning]

@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 FasterXML/jackson-core#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.

[cowtowncoder]

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.

[ajakk]

@PoppingSnack I assume you did not file this

I wouldn't be so sure...

[pjfanning]

@cowtowncoder @ajakk it most likely is that @PoppingSnack user - see janino-compiler/janino#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

[yawkat]

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

[cowtowncoder]

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.

[cowtowncoder]

@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)

[AB-xdev]

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: jeremylong/DependencyCheck#5779

[Marcono1234]

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¹², 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.

Footnotes

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. ↩

[cowtowncoder]

While I am all for changing behavior for better, I am somewhat cynical about likelihood of changing behavior of others.

Thank you for sharing your thoughts @Marcono1234 -- interesting thing about Curl, in particular.

[attritionorg]

Actually, becoming a CNA would prevent filing essentially. Then CNA rules tell every other CNA, including MITRE, to defer to you first for the assignment.

Please don't hold your breath for MITRE to change. There is no real incentive for them to do so.

[cowtowncoder]

@attritionorg Yes, I think this is the intent wrt becoming CNA. And I 100% agree in thinking it is rather unlikely anyone can easily change Mitre's SOP from outside -- my understanding is that there is widespread discontent wrt lax granting of CVEs, overblown Scary-Scoring and various other suboptimal aspects (f.ex Curl author made good points that I fully agree with).
But despite this being the case, I don't see any changes forthcoming -- and I also do not think they were unaware of this friction (i.e. politely telling them to stop blindly accepting garbage submission probably will do nothing).
Still, I am not going to stop anyone from trying to change things in positive way. Just suggest to tone down expectations.

[attritionorg]

@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.

[cowtowncoder]

@attritionorg Alas, this is very close to what I was guessing might be the case. :-(
I wish it wasn't.

Thank you for sharing -- I do not have internal view, just external observations.

And there is the definite business motive for tooling vendors to "solve" problems by blindly forcing upgrades due to low-quality reports. Not saying vendors had no concerns, just that their business model benefits from higher volume of warnings, alerts, so despite wanting to "do the right thing" there is that fundamental conflict of interests.

[yawkat]

Maybe it would be worth it to join forces with the curl project (and/or other large OSS projects) to become a CNA.

[attritionorg]

@yawkat Unless there is some 'official' organization that encompasses many projects, that likely isn't possible and doesn't fall under the scope of an 'organization' or business that qualifies to become a CNA. Especially with GitHub being a CNA and every project rolling up to them for assignments if any given repo/project opts to. It's a pain in the ass I bet, but any high-profile project or library should look to become one in my opinion, ONLY if they are seeing a lot of bad assignments. Then it allows you take the power away from MITRE or other CNAs on assignment. At worst, a researcher can protest to MITRE that their vuln should get one, but then MITRE would defer to you... and more importantly, a conversation would happen between you and MITRE. If it didn't, then it means MITRE is further breaking down their own polices and procedures, which have warped considerably the last 5 - 10 years.

[Marcono1234]

I suggested in github/advisory-database#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.

[cowtowncoder]

@Marcono1234 Ah. Did not know Gitlab is doing this (I used them at my previous company, they seemed to have a few things that were ahead of Github, but are much less well known). Thank you for filing that issue (regardless of whether anything happens -- at least it can be pointed to as one possible way forward, plus exposes the issues).

[tonyschwartz]

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.

[yawkat]

@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.

[cowtowncoder]

@tonyschwartz Please re-read the discussion again with thought. There is no vulnerability shown here.
As @yawkat suggests JSON does not allow expressing of cyclic data -- nor does Jackson object mapper by default.

So given PoC for assumed vulnerability is specifically constructed to allow cycles and then specifically crafted JSON that PoC is sending produces SOE. That is not vulnerability: attacker has way to force use of specific data model.

[tonyschwartz]

I believe it is possible though. The stack is only so big. If I send you a 20MB json with many child objects, I can crash your app, no?
{"a":{"a":{...

[cowtowncoder]

@tonyschwartz That is different issue, solved via #943 (for Jackson 2.15.0).

[yawkat]

@tonyschwartz we have protection against deserialization of deeply nested structures

[tonyschwartz]

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

[tonyschwartz]

I hear you and agree completely. But, the authors of the CVE say, "allows attackers to cause a denial of service or other unspecified impact via a crafted object". It implies the attacker has control, not the developer. I trust you have ruled this out and my comment was not intended to refute your claim. But, the CVE, as written, does imply there is more to the story. I would like to talk to the person/team who authored the CVE. Thanks for the reply!

[yawkat]

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

[cowtowncoder]

@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.

[cowtowncoder]

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:

FasterXML/jackson-core#1055

to be included in Jackson 2.16.

[cowtowncoder]

To prevent further spamming by folks asking for already answered question -- CVE-2023-35116 (https://nvd.nist.gov/vuln/detail/CVE-2023-35116) is INVALID BASELESS BOGUS NO-GOOD FALSE ALARM -- will try closing comments to this issue.