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

[CVE-2022-40151] Fix StackOverflowError #316

Closed
wants to merge 1 commit into from

Conversation

basil
Copy link
Contributor

@basil basil commented Nov 4, 2022

This PR starts by copying clusterfuzz-testcase-minimized-XmlFuzzer-4827085820002304 from #314 into xstream/src/test/com/thoughtworks/acceptance/CVE-2022-40151.xml and adds a new test that fails with the stack trace described in #314. It then goes on to fix the problem by introducing the concept of a recursion depth limit (set to 500 by default), which prevents the StackOverflowError from occurring. The test case sets the recursion depth limit to 600 and asserts that this limit was reached in order to test that the limit can indeed be increased. With the changes to xstream/src/java/, the new test now passes.

Closes #314

@udhay-tw
Copy link

udhay-tw commented Nov 7, 2022

Fixing [CVE-2022-40151] will help all the libraries having transitive dependencies.
Thanks for the work around being shared in https://github.com/x-stream/xstream/security/advisories , which will help with direct dependencies
Thanks for your effort

@christophersavory
Copy link

@joehni have you seen this PR?

@joehni joehni self-assigned this Nov 14, 2022
@joehni joehni added the bug label Nov 14, 2022
@joehni joehni added this to the 1.4.x milestone Nov 14, 2022
@joehni
Copy link
Member

joehni commented Nov 15, 2022

@basil: Thanks for the pull request, but unfortunately this will not fix CVE-2022-40151, it will make it just less likely.

A StackOverflowError is like an OutOfMemoryError, that can be triggered by providing a big amount of data e.g. for a byte array. Since the stack size of a thread is depending on the environment and even configurable, you might reach the limit much faster than expected.

However, I'll add this to the next release, because it provides the ability to limit the stack size for your input if you know its depth in advance. Exceeding this limit is likely an attack then.

@basil
Copy link
Contributor Author

basil commented Nov 15, 2022

this will not fix CVE-2022-40151, it will make it just less likely.

Just like FasterXML/woodstox#160 / FasterXML/woodstox#159, which was apparently enough to "resolve" CVE-2022-40152 🤷‍♂️

@VipinMiglani
Copy link

May i get an update on this PR? When can we expect this vulnerability addressed? Thanks

@joehni
Copy link
Member

joehni commented Nov 24, 2022

May i get an update on this PR? When can we expect this vulnerability addressed? Thanks

It depends on my spare time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stackoverflow CVE-2022-40151
5 participants