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

Stack overflow (50083) found by OSS-Fuzz #387

Closed
henryrneh opened this issue Feb 21, 2023 · 10 comments
Closed

Stack overflow (50083) found by OSS-Fuzz #387

henryrneh opened this issue Feb 21, 2023 · 10 comments
Labels
TOML Issue related to TOML format backend
Milestone

Comments

@henryrneh
Copy link

Dear jackson-dataformats-text developers,

Fuzzing has found a stack overflow in OSS-Fuzz with JVM Fuzzer Jazzer in jackson-dataformats-text. We have reviewed the finding and consider it security-related due to the potential of a denial of service.

Part of the crash stack trace:
== Java Exception: com.code_intelligence.jazzer.api.FuzzerSecurityIssueLow: Stack overflow (use '-Xss921k' to reproduce) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseKeyVal(Parser.java:463) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseInlineTable(Parser.java:416) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseValue(Parser.java:229)
Caused by: java.lang.StackOverflowError 
at com.fasterxml.jackson.dataformat.toml.Lexer.yylex(Lexer.java:755) 
at com.fasterxml.jackson.dataformat.toml.Parser.poll(Parser.java:101) 
at com.fasterxml.jackson.dataformat.toml.Parser.pollExpected(Parser.java:106) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseAndEnterKey(Parser.java:173) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseKeyVal(Parser.java:461) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseInlineTable(Parser.java:416) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseValue(Parser.java:229) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseKeyVal(Parser.java:463) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseInlineTable(Parser.java:416) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseValue(Parser.java:229) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseKeyVal(Parser.java:463) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseInlineTable(Parser.java:416) 
at com.fasterxml.jackson.dataformat.toml.Parser.parseValue(Parser.java:229)
...

We have included a Reproducer zip which contains a README file that describes how to reproduce the issue.
Reproducer zip: 50083-jackson-dataformats-text-TOMLFuzzer.zip
We would appreciate if you could take a look into the findings!

OSS-Fuzz Issue: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50083
Hint: The provided OSS-Fuzz Issue links are only accessible if the issue gets fixed or if you are the maintainer of the OSS-Fuzz project.

Fuzz target: https://github.com/google/oss-fuzz/blob/master/projects/jackson-dataformats-text/TOMLFuzzer.java

@yawkat
Copy link
Member

yawkat commented Feb 21, 2023

so basically FasterXML/jackson-databind#3397 but for toml? fun...

@cowtowncoder cowtowncoder added the TOML Issue related to TOML format backend label Feb 21, 2023
@cowtowncoder
Copy link
Member

Right. I have been thinking of adding low-level maximum nesting limits via JsonStreamContext in jackson-core but even when added (it's tricky as there's both critical path performance & API compatibility to figure out) I don't think it'd prevent problems here.

Short-term thing here would be just to keep nesting counter and impose some upper limit (possibly configurable via TOMLFactory.Feature -- or whatever per-format reader features are, or can be added) and fail on "too deep" nesting.

@cowtowncoder
Copy link
Member

@yawkat Yes and no: that one is for databind, this is in format parser. Most incremental streaming parsers do not use recursion so JSON etc do not have a problem at this level but deserializers and serializers do use recursion (except where optimized away as with JsonNode and java.lang.Object deserializers).

But as per my above comment it may be relatively straight-forward to catch in this case (although you know the code better than I do so I could be wrong).

yawkat added a commit that referenced this issue Mar 6, 2023
This replaces the StackOverflowError with a StreamReadException with a proper message.

Fixes #387
@pjfanning
Copy link
Member

pjfanning commented Mar 7, 2023

Also can be fixed by #398

@yawkat
Copy link
Member

yawkat commented Mar 8, 2023

fixed by #398

@yawkat yawkat closed this as completed Mar 8, 2023
cowtowncoder added a commit that referenced this issue Mar 9, 2023
@henryrneh
Copy link
Author

Dear jackson-dataformats-text developers,

There was StackoverflowError[1] fixed in jackson-dataformats-text with [2]. A release was already published containing the fix. Do you consider to publish a security advisory? If you need assistance, we can request a CVE and keep you updated.

[1] https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50083
[2] #398

@cowtowncoder
Copy link
Member

@henryrneh Yes, I think you can go ahead file for a CVE given the situation like you explained.

@yawkat
Copy link
Member

yawkat commented Jul 29, 2023

please note this only affects the toml module

@henryrneh
Copy link
Author

henryrneh commented Jul 31, 2023

Thanks @cowtowncoder @yawkat!

@cowtowncoder
Copy link
Member

@yawkat Yes, very important point.

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

Successfully merging a pull request may close this issue.

4 participants