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

Disallow nested objects and arrays as keys in objects #772

Merged
merged 4 commits into from
Oct 1, 2023

Conversation

eamonnmcmanus
Copy link
Contributor

Fixes #771.

For object keys, we can just skip the part of `nextValue()` that parses values
that are objects or arrays. Then the existing logic for unquoted values will
already stop at `{` or `[`, and that will produce a `Missing value` exception.
@stleary
Copy link
Owner

stleary commented Sep 28, 2023

@eamonnmcmanus I think this is a good fix and probably overdue. However, since JSONTokenizer is central to the lib, more testing would be helpful. For example, the non-quoted value might be an object key, object value, or an array element. The value itself might be a simple string or might contain one or more JSON control chars: backslash, curly brace, bracket, comma, colon, or double quote. It might be number-like (contains one or more periods, hyphens, plus signs, or 'e' chars). It would be good to know if the tokenizer processes these examples in a reasonable way. There might also be other potential tests I haven't thought of. This could get tedious to code and might be more than you signed up for. Let me know if you want some help with the unit tests. Either way, thanks for getting this started.

@eamonnmcmanus
Copy link
Contributor Author

I've added some extra tests to cover unquoted text. They are not as extensive as listed in @stleary's comment. Further test coverage for unquoted text would surely be good, but I think is a bit out of scope for this PR. The goal here is to fix a DoS problem, while not breaking any existing reasonable uses. I think the tests cover that quite well.

@stleary stleary changed the title Generalize the logic to disallow nested objects and arrays as keys in objects. Disallow nested objects and arrays as keys in objects. Sep 29, 2023
@stleary
Copy link
Owner

stleary commented Sep 29, 2023

What problem does this code solve?
Unquoted object and array values should be restricted to simple types

Risks
Moderate. This is a change to existing behavior and may impact users.

Changes to the API?
No

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No. New unit tests were added.

Was any code refactored in this commit?
A test method name typo was fixed.

Review status
APPROVED

Starting 3-day comment window.

@stleary stleary merged commit beb2fb5 into stleary:master Oct 1, 2023
5 checks passed
@stleary stleary changed the title Disallow nested objects and arrays as keys in objects. Disallow nested objects and arrays as keys in objects Oct 13, 2023
johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this pull request Oct 17, 2023
Disallow nested objects and arrays as keys in objects.
claireagordon added a commit to yext/JSON-java that referenced this pull request Mar 26, 2024
Port of stleary/JSON-java#772
to partially remediate
https://www.cve.org/CVERecord?id=CVE-2023-5072 , where
nested keys can allow relatively small inputs to
cause OOM errors through recursion.

Test by:
- package & import into alpha locally
- confirm a suite of unit tests depending on JSONObjects
passes
- verify that the following CVE Proof-of-concept fails
with an 'unexpected character' exception:
https://security.snyk.io/vuln/SNYK-JAVA-ORGJSON-5962464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logic to exclude object keys that are themselves objects is imperfect
2 participants