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

[cowtowncoder]

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.

[vin01]

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/

[Marcono1234]

As response to https://github.com/FasterXML/jackson-databind/issues/3972#issuecomment-1637273209:

Ok. My only concern is with extra work for filing CVEs, but then again number is fairly low (after polymorphic deser flood), and at least we'd have more say in actually blocking invalid reports (and not just get optional FYI from submitter).

You can request CVE IDs through GitHub advisories (see documentation). Though I don't know if it would be possible (or allowed) that you are your own CNA, but let GitHub request the CVEs.

But since every maintainer of a GitHub project is now able to request CVE IDs on their own, maybe Mitre should just reject taking requests for GitHub projects, telling the reporter to contact the maintainer instead (and refer to the GitHub documentation, in case the maintainer is not aware that they can request CVE IDs easily)? And Mitre should only act then for arbitration requests, or when maintainers are unresponsive?

[cowtowncoder]

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.

[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[^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.

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

[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:

https://github.com/FasterXML/jackson-core/pull/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.