-
Notifications
You must be signed in to change notification settings - Fork 293
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
[GHSA-92jh-gwch-jq38] PocketMine-MP server crash with certain invalid JSON payloads in LoginPacket
due to dependency vulnerability (again)
#4371
Conversation
Hi there @dktapps! A community member has suggested an improvement to your security advisory. If approved, this change will affect the global advisory listed at github.com/advisories. It will not affect the version listed in your project repository. This change will be reviewed by our Security Curation Team. If you have thoughts or feedback, please share them in a comment here! If this PR has already been closed, you can start a new community contribution for this advisory |
List of invalid security issues: |
Not sure I agree with this change. The fork was created to mitigate the aforementioned security issues in PocketMine-MP by closing loopholes in JsonMapper's validation. I acknowledge that these automated reports tarring JsonMapper with "severe" security issues on account of PM is unjustified considering the scope of the library, but I do think the report is still correct. Perhaps the report could be rephrased to say something like "behaviour unexpected by PocketMine-MP" or something to that effect. Not sure if that would make these security report scrapers stop complaining though. |
Just picking some statement from the first issue: I won't directly reject the notion that by using JsonMapper, a denial of service attack could be possible. However stating that JsonMapper has a problem that allows denial of service IN EVERY CASE is wrong. The security report is poorly written, and was picked up by consumers that published it in an automated fashion, now creating doubts if a software is in danger of denial of service attacks (which seems to only happen to your specific software due to very specific circumstances). Unfortunately no report I have seen properly states what has to happen to trigger the issue. It only is yelling of "DENIAL OF SERVICE", high severity numbers, thus validating that subscribing to any of these services is doing something useful. Would you work with me trying to identify what seems to be the issue here? I have created this cweiske/jsonmapper#233 to discuss what should be the proper handling of NULL values in arrays. However I am not confident your issue matches my observation. To be specific, I would very much like to see the "malformed JSON" mentioned in the quote above, and the target mapped objects this is suppose to be transformed into. |
Just as a reminder, I have received your cweiske/jsonmapper#226 issue statement, but got no response after initial discussion where you'd defend your point. I don't state your claim is invalid. I would consider it to be somewhat lacking the interesting details, for example: What would you expect in the mentioned situation? And in addition, your issue does not at all refer to any JWT or |
Yeah, that's correct.
The issue provides the minimal reproducing code for the issue in JsonMapper. The PocketMine security report goes into surrounding detail to explain how the problem occurs in PM's case, but that's not relevant to JsonMapper. The only important stuff for JsonMapper is the JSON validation.
The associated issues for this security report would be these: cweiske/jsonmapper#211, cweiske/jsonmapper#233 There are currently 3 outstanding issues that I encountered:
I've responded to some of your other comments directly on the relevant issues. |
I've updated the advisories in question to make it clearer; let me know if you think it still needs improvement Not sure if the changes will get automatically reflected on this repo or not |
Alas they will not 😞 Sorry for getting to this PR so late, but is there anything more than reflecting those GHSAs that I should put through for you or does that suffice? 😄 |
From my side, as long as the issue with "server crash" isn't attributed to the netresearch/jsonmapper package anymore, I am fine. We are working on fixing the issues, but they are not crashing any server by themself, they need "support" from the outside code using the mapper. |
I see. So for the purposes of this thread would you say that the behavior that @dktapps ran into is expected (or allowed) behavior in |
The described behavior in this vulnerability report is: As illustrated in cweiske/jsonmapper#233 (comment), there is an explicit test validating that a JSON array containing a Personally, I wouldn't recommend using unvalidated JSON input with the mapper if the proper structure of JSON is important. JSON schema validation is quite a complex task in itself, it does not fit into this tiny mapper library. And when it comes to JWT, the validation process is again much more complex and warrants using any of the existing dedicated JWT libraries that do the cryptographic validation. |
Gotcha. Would some language like @dktapps would you also be ok with that sort of language? |
Yeah, that's pretty much how I rephrased the original advisory after seeing this PR |
Yep. As I said previously, PM is only using JsonMapper to process claims in some JWTs (which have relatively simple structure and can be directly described using classes). The JWT detail mentioned in the security report is just to highlight where in PocketMine this problem occurred and is orthogonal to JsonMapper itself. |
@SvenRtbg your thoughts? Is my proposed language change acceptable? |
The statement is correct from my POV. |
74080ec
into
SvenRtbg/advisory-improvement-4371
Hi @SvenRtbg! Thank you so much for contributing to the GitHub Advisory Database. This database is free, open, and accessible to all, and it's people like you who make it great. Thanks for choosing to help others. We hope you send in more contributions in the future! |
Cool. I've gone ahead and altered the language to be a little less accusatory. Either of you please feel free to be accusatory toward me and yell about how much worse I made everything (/s) and we can reboot the conversation :) |
Updates
Comments
This report only is relevant to pocketmine/pocketmine-mp because that repo contains the class
LoginPacket
that does not properly validate the input values. The relevant package dealing with mapping JSON to PHP classes is a fork in https://github.com/pmmp/netresearch-jsonmapperThis report does not affect the original JsonMapper library, as having NULL entries in arrays is a defined and tested use case (admittedly an unexpected one). The original project has received several invalid vulnerability reports because of this incorrect attribution, as multiple "security report collector" companies spread this incorrect information through channels beyond the maintainers control.