1. snakeyaml Avatar snakeyaml
  2. snakeyaml
  3. snakeyaml
  4. Issues

[OSS-Fuzz - 47081]

Issue #531 resolved
Dae created an issue

Uncaught exception in java.base/java.util.ArrayList.hashCode

Stacktrace and crashing input attached.

‌

Comments (54)

  3. Henry Lin

    Hello Dear Snakeyaml maintainers! I am Henry from Code Intelligence. Just would like to ask that is this issue still open? Or is it fixed? We are planning to apply for CVEs for OSS-Fuzz findings in Snakeyaml, and this information will help us, thank you!

  6. Khusanjon Developer

    Hello team. Could you provide more information about this vulnerability? What is the root cause and is it possible to avoid invoking this issue? Also, we would like to know when will the fix be released. Please share if there is any mitigation available that will help till the fix is released. Thanks.

  7. Andrey Somov

    @Khusanjon Developer do you ask this team, the SnakeYAML team ? I do not see it as vulnerability at all. Please contact the reporters.
    This is not fixed because it requires to apply a lot of restrictions.

  11. Charles Nutter

    I am dubious about the DoS potential of this bug. It appears to be a normal stack overflow due to a recursive ArrayList that gets raised as a normal exception. I agree it is a bug, in that SnakeYAML should avoid creating recursive lists that are then used in a hash calculation, but a stack overflow bug does not mean this is a DoS.

  12. Andrey Somov

    @Charles Nutter please be aware that recursive ArrayList as a key must be supported by the specification. But since it may not be desired, there is a setting to fail to avoid the attack.

    I find the noise around the issues too disturbing. The low quality tooling gives a wrong impression that the library is vulnerable, when it is not.

  13. Charles Nutter

    Perhaps this is a case where the default behavior should not be strict to the specification? Or a different data structure that can handle recursive hashing could be used? I agree that this does not seem to constitute a vulnerability, but perhaps it should be mitigated by default.

  14. Andrey Somov

    Well, I think the other way around. The spec is first. Those who consume untrusted data may configure SnakeYAML. They know the context. They are prepared and they may sanitize the input before giving it to the parser.

  15. Paul King

    Can I check my understanding. For the example in the Fuzz report, there is no easy setting to guard against the SOE?

    I was trying to determine if Groovy code using SnakeYAML was affected, here are my test cases:

    import org.yaml.snakeyaml.*
import org.yaml.snakeyaml.constructor.*
import org.yaml.snakeyaml.error.YAMLException
import static groovy.test.GroovyAssert.shouldFail

// control: a list with no map element
assert new Yaml().load('&a\n- *a\n- *a\n- *a').toString() ==
    '[(this Collection), (this Collection), (this Collection)]'

// default for allowRecursiveKeys is false and this is one of the cases detected (colon on first alias)
new Yaml().with {
    assert shouldFail(YAMLException) {
        load('&a\n- *a:\n- *a\n- *a')
    }.message == 'Recursive key for mapping is detected but it is not configured to be allowed.'
}

// default for allowRecursiveKeys is false but this is one of the cases not detected (colon on 2nd alias)
new Yaml().with {
    assert shouldFail(StackOverflowError) {
        load('&a\n- *a\n- *a:\n- *a')
    }
}

// doesn't help here since it is the recursive key that was causing problems
new Yaml(new LoaderOptions(allowRecursiveKeys: true)).with {
    assert shouldFail(StackOverflowError) {
        load('&a\n- *a:\n- *a\n- *a')
    }
}

// an ugly way to stop the SOE for this example but doesn't help with loading and not applicable in general
new Yaml(new LoaderOptions(maxAliasesForCollections: 2)).with {
    assert shouldFail(YAMLException) {
        println load('&a\n- *a\n- *a:\n- *a')
    }.message == 'Number of aliases for non-scalar nodes exceeds the specified max=2'
}

// If I really need to cater for this example, I can load the example with a custom list
class MyList extends ArrayList { int hashCode() { -1 } }
def myList = new Yaml(new Constructor(MyList, new LoaderOptions(allowRecursiveKeys: true))).load('&a\n- *a\n- *a:\n- *a')
assert myList.size() == 3
assert myList*.getClass() == [MyList, LinkedHashMap, MyList]
assert myList[0] === myList && myList[2] === myList
myList[1].entrySet()[0].with { assert getKey() instanceof List && getValue() == null }

    I am not suggesting you would ever use the MyList class as is and certainly not as a key to a map, but if you want to load recursive data, you should use data types which can handle that recursion and ArrayList isn't suitable for this example. Perhaps that could be added to the "CVE & NIST" wiki page.

    My conclusions:

    • There does appear scope for detecting more cases where the recursive keys might appear. It isn’t clear to me at this point whether the case which causes ArrayList to barf is possible/easy to detect (nice if it could) but it isn’t likely that a general solution for all cases could be implemented.
    • Perhaps the doco could be improved a little. Reducing the stack size (if you don’t make extensive use of recursion) might limit the scope of a DoS attack in situations where potentially tainted data can’t be further validated. And something about recursive data structures as per above might be useful.
    • If you want to stick with ArrayList, SOE is the expected behavior here.

    Does that sound about right?

    ‌

  16. Alexey Belostotsky

    To be honest, it feels like this CVE rating is overly pessimistic.

    First, is assumes that the attack vector is network with user input possibility. But in many cases this library is used primarily for configuration (eg spring boot), without any potential of untrusted user input over the network

    Secondly, SOE in Java is not fatal exception, so the DoS part is also questionable

    Maybe Henry Lin can shed some light on this

  19. Paul King

    @Andrey Somov My comments weren’t about the spec I don’t think, but rather the SnakeYAML implementation. '&a\n- *a:\n- *a\n- *a' fails loading with YAMLException whereas '&a\n- *a\n- *a:\n- *a' fails with StackOverflowError. Does the spec cover that scenario? Also, I showed how you could use a custom list to make both pass even though the default implementation using ArrayList fails. I wasn’t particularly trying to defend or dismiss the CVE, just increase my understanding to potentially update Groovy documentation around the same subject.

    I suspect most Groovy users using YAML are processing their own config but some might be using YAML as the payload for a REST service, who knows. If anyone falls into that latter category, I’d want to provide them with as many config options that make sense so they can dial down spec compliance and increase protection against various scenarios (if they wanted). And over and above that, I’d want the documentation to mention the high risk cases and ways to mitigate.

  20. Andrey Somov

    @Paul King I am afraid I do not understand you.

    Can you please provide a PR to make the things clear ?

  21. Paul King

    @Andrey Somov The complete code is in the above comment. It’s not a PR but a Groovy script. Is there any part you need me to explain?

  26. Andrey Somov

    @Pawel Veselov why not to ask the source ?

    I am afraid, you are here not because of CVE, you are here because of a tool which report a problem in SnakeYAML. What is it ?

  27. Pawel Veselov

    @Andrey Somov I’m here because of a tool that checks for vulnerable depencies (e.g., https://jeremylong.github.io/DependencyCheck/dependency-check-maven/index.html, but there are many that validate projects against depencies registered in NIST as vulnerable). We don’t use SnakeYAML directly, but one of our dependencies (Okta SDK) does.

    The dependency checker is flagging any version of org.yaml:snakeyaml as vulnerable for CVE-2022-38752 because the associated CPE has no version part.

  29. Pawel Veselov

    I’ve read the CVE&NIST document, and though I agree with the analogies on how this approach is extremely crude, I don’t agree to the philosophy. In terms of this document, the shop (terminal software) most likely has no bandwidth and capacity to defend against all possible problems people walking through its doors can bring in, i.e., to investigate whether any version of software flagged as vulnerable will have an impact or not. You are saying that Okta SDK doesn’t read YAML from untrusted sources, but I can’t take it on its face value without my own audit, which I don’t have time or resources to perform. I have 300 dependencies just for one project, and that’s considered “small”.

    No automated check will be able to determine whether a potentially vulnerable software can be exploited or not in its current configuration/application, or declare that it’s OK to use a vulnerable library through a one dependency, and not another. Even though I don’t agree that this particular problem (stack overflow) warrants this particular fuss, any exceptions need to be defended to the investors, customers and insurance companies. And OWASP analyzers are quite dumb in this case, but are straight forward. Yes, this is very inefficient and bureaucratic, but the alternatives are ridiculously expensive.

    And, software has bugs, and OWASP analyzers have bugs that flag false positives, that’s just how things are. In this case, however, technically it’s not a false positive, because the matching software for CVE CVE-2022-38752 is - all versions of org.yaml:snakeyaml, period. IDK who’s responsible for updating the CVE database, or who bungled this up - usually the version shows up in the CVE entry before the fix, which means this is either NIST’s or Google’s oversight. I figure either them or the maintainer are the only people that can do something about it.

    This can be compared to CVE-2022-25857, where the CPE is - all versions of org.yaml:snakeyaml earlier than 1.31.

  30. Andrey Somov

    @Pawel Veselov it is sad, that you missed the point. The analyzers present false as true. They confuse. The NIST database does not confuse, it provides the information for those who understand the difference (analyzers do not)
    What is the philosophy to treat a man without a gun in the same way as a man with a gun ?

    ‌

  31. Pawel Veselov

    @Andrey Somov I believe I have clearly explained what exactly is wrong with the data in the NIST database in this particular case. Any human or machine that will read the description of CVE-2022-38752 will have to conclude that all versions of SnakeYAML have the problems described in there, unless they investigate, because there is no mention of which versions(s) are vulnerable, neither in the description meant for humans, nor in the one for the machines. While humans can investigate, machines can not.

    As to the threat assessment, we already treat everybody as a potential threat in societies as well, look at the airport security, or cages in the jewelry shops. We are treating people as if everybody had a gun, exactly because we know some people do.

    But I don’t feel that the shop analogy is all that fair because in case of software, the fix is trivial - upgrade or ignore. Yes, in some cases there are true false positive, I’ve seen a handful, the analyzers give you a way to suppress those. And I had to suppress SnakeYAML being flagged - but not because of it being a false positive, but because the source data is incorrect.

    From that perspective, threat risks are same as safety risks. If a software in a plane has a potential hypothetical problem of stalling that is only theoretical, you’d still rather eliminate it rather than discovering you overlooked a condition that may lead to its trigger.

    Again, the particular problem in this ticket is not that critical, but nobody has time to deal with those on an individual level, NIST database grows by hundreds of entries daily.

  32. Anton Mostovoy

    Hi, Andrey. First of all, thank you for working on this project. I think OSS maintainers don’t get enough credit.
    I am also curious why it is not possible to petition to update CVE-2022-38752 cap it to 1.31, since the issue has been fixed in 1.32
    Is it because you don’t have time spend on this or some other reason?

    Thank you!

    ‌

  34. Andrey Somov

    @Anton Mostovoy I wonder you ask this project and not those who create and maintain the issues there ?
    Sure, I do not want to waste any time for this. It already consumed far too much

  35. Chad Wilson

    To my knowledge the email-based process to get NIST NVD updated is the only way currently, and there are a bunch of humans at NIST NVD evaluating the submissions which flow from CVE Numbering Authorities (Google in this case) into MITRE and then NVD. NIST NVD assess CVSS scores and make amendments, link additional evidence etc. It’s also possible to dispute CVEs via MITRE and/or request re-evaluations of severity, although the process seems a bit painful and I’ve not been successful getting spurious data removed (easy to create noise on little evidence, very difficult to remove).

    I am happy to write to the NIST NVD to ask them to review and update the fix version based on the publicly available information, and started trying to do so.

    However in trying to provide them links to evidence of the fix, I can’t clearly find anything other than the assertion above “The fix will be delivered in version 1.32”. I can find a proof that this is a false positive when properly configured (and the defaults for many versions seemed to be to disable recursiveKeys) in https://bitbucket.org/snakeyaml/snakeyaml/commits/481078991274c1c8a0a550634164a230b4c23334, but that doesn’t seem the same as a fix. Can someone help me see what am I missing from the commits? The commits don’t seem linked to this issue, so if anyone can point me to the commit(s) where the relevant fixes are I will try and get NIST NVD corrected/updated.

  36. Anton Mostovoy

    @Andrey Somov Thank you for your response. I was asking to determine how I could help with getting the CVE updated. Looks likes Chad beat me to it.

  37. Karsten Spang Account Deactivated

    @Chad Wilson I don’t think it is possible to “fix” this in that sense. YAML has recursive keys, so it needs to be supported by the software. And where you have recursion, you have the risk of infinite recursion. You probably can’t get any further than disabling the feature by default and having a warning saying “Don’t enable it unless you really need it, and you have full control over your input!”.

  38. Andrey Somov

    @{61a91ab2c510bc006b0b9c1a} it is the other way around - you do not disable it till you have control.
    In many places (inside and outside of SnakeYAML) I scream to provide us with a use case when somebody is blindly reading the data from the untrusted source. No single one was provided.

    ‌

  39. Andreas Igel

    @{61a91ab2c510bc006b0b9c1a} Is really an ArrayList is used in this framework for holding the nodes? Becuase this issue here has been, that hashCode is generated invinite. Would it be a solution to use a replacement of ArrayList, maybe some kind of LinkedList?

    As I understood, this is not a Vulnerability-topic anymore here. According to the creators of that vulnerability, it has been fixed: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47081. Just NVD is not updated yet: https://nvd.nist.gov/vuln/detail/CVE-2022-38752. But that should be only a matter of time!

  41. Andrey Somov

    I do not like this issue because so many people are confused and we waste so mush time for nothing.

    @Andreas Igel when LinkedList has the last node point to the first - it still makes infinite list.
    For your information - those who create such a List, they do it on purpose and do not call hashCode() on it. There is nothing to fix.

  42. Andreas Igel

    @{61a91ab2c510bc006b0b9c1a} That is a good question, we will see. In maven-central it is already shown to have no vulnerability anymore (starting from 1.32).

    @Andrey Somov You are fully right, I have thought that the JDK already solved that in implementation or by a special kind of collection. It is a bit of surprising because hashCode is really a basic functionality, used in many default Java-functions (in background) and lists are such a basic class too. So if hashCode could not be used on an object, you can fast get in massive trouble. But you are right, it creating a much of noise and taking a much of time here! And at the end it is just the answer: do not use self linked nodes there! But unfortunately just saying “use better tools” is not an answer too. All of us have to do much more in dependency-vulnerability tracking because the world has changed. And often you have the case, that a dependency needs another dependency, needs another dependency and so on. By that route you loose the information how each of them is using the dependency before and often me is getting just the information, that we are using a transitive dependency, which has a function recognized to be vulnerable in some circumstances. Then it is not easy to rate that! By forbidding it with configuration (disable recursiveKeys), you did a great job so that there is a way to just disable the potential risk (if someone is really using data, inserted by an enduser)!

  43. Chad Wilson

    @{61a91ab2c510bc006b0b9c1a} Yes, that was my understanding too (in terms of the wider issue not being completely “fixable”), but my understanding was previous versions disabled recursive keys by default as well. Nevertheless, I realised that something must have changed in the default behaviour if https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47081 is now reported as being fixed.

    So after git bisecting my way to figure out which change removed the stack overflow here by default config I found it was 5056a448f09c46250346c338e821386caa751182 Issue 531: Improve mainly due to the changes in SafeConstructor to check keyNode.isTwoStepsConstruction() at https://bitbucket.org/snakeyaml/snakeyaml/commits/5056a448f09c46250346c338e821386caa751182#chg-src/main/java/org/yaml/snakeyaml/constructor/SafeConstructor.java which I presume allows it to detect this particular case anyway, so it further extends the “safe by default” behaviour around recursive keys.

    I missed this earlier as it was further back in the commit history than expected given the conversation above.

    Anyway, since this does confirm some change was made to “fix” something (rather than, perhaps, “not an issue”), I will try and contact NVD to remove some of this further noise :-)

  44. Chad Wilson

    Good afternoon,

    Thank you for bringing this to our attention. We appreciate community input in order to provide the most accurate and up-to-date information as possible. After reviewing publicly available information, we have made the appropriate modifications to reflect that the affected versions are up to (excluding) 1.32. Please allow up to 24 hours for the changes to be reflected on the website and in the data feeds.

    V/r,

    National Vulnerability Database Team

    nvd@nist.gov

    ‌

  47. Pawel Veselov

    Nothing was fixed in 1.32. For this to go away, there needs to be a code change that prevents the infinite recursion, and throws a different exception.

  48. Chad Wilson

    There was a code change that prevented these particular OSSFuzz cases in the default configuration where allow recursive keys is disabled and causes a specific exception to be thrown as with other recursive keys cases that were being caught earlier. Did you look at/run the tests? That's presumably why OSSFuzz considers the issues addressed. Already posted that analysis above. Do you have a specific critique?

    Honestly im pretty confused with your messaging as much earlier in this thread you appeared to be complaining that NVD wasn't capping the affecting versions of the earlier CVE to 1.31. I then try to help the developers and community out by getting it reviewed and updated to that on my own accord, now you seem to be claiming it shouldn't be updated at all? What are you trying to achieve here?

  49. Pawel Veselov

    What I ultimately want is for my builds to not trip on the CVEs filed against this library (the use I have very little control over). My rants about the previous CVE was that the owners' claim that the CVE was addressed in 1.32 was not propagated to the NVD database, which kept NVD checkers flag it as problematic.

    OSSFuzz will not configure the library in a certain way before trying it against their sample data. The fix done for 1.32 is only applicable to custom configuration. So I’m not at all surprised this new CVE popped up. However, I also got the distinct feeling that the owners believe that the entire process of automated checking for vulnerabilities is rather pointless, and that there is nothing that needs to be fixed here, and we should trust >3,500 other libraries that use SnakeYaml to configure it correctly.

    However, if one does want these CVEs to stop, the code should just not throw stack overflow exceptions when presented with bad data, no matter how the library is configured. I.e., have the hashCode() implementation (along with any other methods that recurse blidnly) stop recursing if it runs into an object that was already seen.

  51. Karsten Spang Account Deactivated

    @Pawel Veselov The problem with a library like this is that sometimes you actually want recursion, and don’t want to limit it. There should be an option to turn it on. Off course, you should not do so if you don’t have control over the input. As you write

    OSSFuzz will not configure the library in a certain way before trying it against their sample data.

    So if the default configuration is to turn recursion off, we should be good.
    If one of the >3500 other libraries actively set the configuration to allow recursion, then the problem is in that other library.

  52. Pawel Veselov

    @{61a91ab2c510bc006b0b9c1a} I don’t want nor am advocating for the recursion support to be turned off, even in default configuration. I also don’t agree that CWE-121 is a danger to Java programs all together, because it’s not (in Java) caused by an unbound write (CWE-787) (But I may not understand all the nuances there). It’s dumb that the language is not a consideration for CWEs (one of the points of Java is that at least some of unchecked behavior is guarded by the VM), but it’s a reality (with a consolation that at least ArrayIndexOutOfBounds exceptions are not a CWE-122, why not) that has to be explained to bureaucrats who can’t tell the difference.

    @Andrey Somov I will get that PR ASAP.

  53. Pawel Veselov

    @Andrey Somov Here is a WiP: https://bitbucket.org/veselov_xl4/snakeyaml/branch/issue-531

    A few things I realized:

    • SO are bad, they tank a lot of CPU (still not an unbound write)
    • The default is NOT to allow recursion (that was my misunderstanding from the beginning)
    • An example of when unsanitized input can contain recursion is a web-app backend for a YAML visualizer (that wants to show valid recursion)
    • There seem to be code that tried to prevent recursion by throwing other exception, but that code caught Exceptions, which StackOverflowError is not
    • I’m obviously navigating the code base as well as a blind kitten would.
    • I think there should be an example of recursing YAML that only involves objects, but I can’t come up with one. Not sure why this isn’t infinitely recursing.
    • I’ve first tried to cap off recursion after values are built (just like some of the existing code does), but this ended up being futile, especially because of postponed recursive fill (BaseConstructor.fillRecursive would throw an SO if things got that far). AFAICS, if the DOM has infinite recursion, it’s enough to blow the parsing

    If you have any feedback, I’d appreciate it. Otherwise, I’ll try to close it up based on my best understanding of the cases and internal API rules

  54. Pawel Veselov

    OK, whatever I said in my previous comment was mostly an utter load of gibberish.

    The SOs can be all together prevented. The constructor already handles recursive references very well, it’s the default native JDK objects that can’t handle it.

    https://bitbucket.org/snakeyaml/snakeyaml/pull-requests/37 - the main idea is to use default native objects that won’t throw SOs on hashCode() (which seems to be the only recursion-prone method that the constructor indirectly invokes).

  55. Log in to comment
Assignee
Type
bug
Priority
major
Status
resolved
Votes
0
Watchers
1
Jira Software: the preferred issue tracker for Bitbucket. Join the team!