-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
XML.toJSONObject may lead to StackOverflowError which may lead to dos when parses untrusted xml: CVE-2022-45688 #708
Comments
Are we expecting a fix for this soonish? |
One way to address this could be RFC 8259 9. Parsers: |
Hey, @stleary . I plan to send a PR, but I'm not sure if it's better for this project to just establish a default arbitrary nesting depth limit or, in addition, add more public methods that allow users to set the nesting depth limit themselves. As this project is in maintenance mode, I'm not sure if you wanted to expand the public API. |
Maybe set a default and just look for a system property override? That way not api changes necessary but it gives people a hook to change if necessary? |
@cleydyr An arbitrary nesting limit is fine to start with. Recommend setting the limit only for XML documents at this time. It's OK to hardcode a limit, but consider allowing more control via the XMLParserConfiguration object. For example, you can add a boolean property to enable/disable the limit, and/or a numeric property to set the limit. My preference would be to disable it by default. Please don't add or modify the XMLParserConfiguration constructor signatures; all but the default constructor is deprecated. Instead, use the with*() methods. See for example the method withcDataTagName(). That could change the code fragment above to something like this:
Keep in mind that users can already have some control by setting the -Xss JVM option. |
Can we update the ticket to include the CVE number in the title so others can find this issue easily. I know I did not think it was related. |
Does anyone have concerns about the implementation in #720 for this issue? |
This issue is being fixed in #720 |
I have concern. Please ignore these code changes. Stack overflow error is natural. No changes needed. |
I don't think this can just be ignored. It had reached all the security reporting services as potential DOS attack. |
I suggest it was in java 8. What about java 17 and 19? |
I guess I will leave it up to the experts but I would think this is vulnerable no matter the Java version. |
@javadev When it was just a complaint about stack overflow, I was not concerned. But the CVE escalated the issue. I think it's important for the open-source community to address published security issues. |
I agree, to fix CVE this check may be added. |
Now that I think about it should we ensure the value is less than the stack size? So if for some reason someone has their stack size set to 256 wouldn't this cause an overflow if we default to 512? So maybe it should be the lower of 512 or stack size-1?? |
Yes it could cause an overflow in such cases - however, I think it is their responsibility, given that now it is possible to change the recursion depth limit. There is a default that is suitable for most use cases; in specific cases, affected developers have the option to override the limit as they see fit. |
❗ So, this is a bit funny. If you are a slack user, who has the github app installed on your slack, do not add a link to this issue in your slack. It looks like the Github app's auto url-unfurler attempts to unroll this XML, causing the slack desktop and mobile client to crash. |
Poc code as follow:
and the. result:
The text was updated successfully, but these errors were encountered: