Denial Of Service (DOS) Vulnerability found in library

[paulrwest]

Denial Of Service (DOS) Vulnerability found in library

[SvenRtbg]

I found this: https://stackoverflow.com/questions/69936667/how-do-i-get-details-of-a-veracode-vulnerability-report

I believe it isn't worthwhile to bother contacting Veracode for details, they are entirely publishing bogus security reports according to public opinion.

[cweiske]

This is not a Denial Of Service (DOS) attack vector. It's just a normal exception that can be caught by the application.
DOS would mean it would take the whole server down, which it does not.

[SvenRtbg]

Ah, the trustworthyness of a website that hides the relevant info behind a paywall...

[SvenRtbg]

Another counter argument: The check introduced in the commit mentioned would only prevent the DoS attack if the flag would be set to true, which defaults to false. So either a proper fix would have to set the flag to true in addition to this check, or remove making this optional entirely. Simply adding a check that is disabled by default does not count as a proper fix from my perspective

I'm happy to discuss details once there is even a theoretical code presented that leads to misbehaving in the library.

[paulrwest]

Thanks for your responses. I totally understand the lack of actual information on the report is frustrating. Like the other reporter, I'm just reporting what we've found. And I also agree the fix appears to be a setting that may not be set anyway but the security scans are not the brightest. :/

You do have a PR out that looks like it resolves the bStrictObjectTypeChecking setting. if that was merged and a new patch released it may solve the SCA report?

#225

[SvenRtbg]

Unfortunately I believe that PR is not doing the right thing. And the claim in these reports about DoS potential is totally out of proportion in relation to this libraries abilities. It's true that you can pass NULL values into an array that claims to only allow certain objects, and there even is a test case for this, validating this happens. So one can hardly claim this is a bug - it is a feature, although an unexpected one.

I have yet to find out what the intention should be, and it will most likely be fixed in a major version release.

[dktapps]

@paulrwest

The problems in PocketMine had were because of unexpected behaviour which arose due to JsonMapper attempting to hydrate objects using flat types. Since JsonMapper performs no checks on the __construct or its inputs during hydration, it can silently lead to unexpected behaviour, or other types of exceptions (aside from JsonMapper_Exception) being thrown. This is not documented anywhere and I made bad assumptions when using JsonMapper around this.

Maintainers previously said they were ok with this behaviour, as JsonMapper isn't intended for data validation; IMHO this defeats half the point of using the library, but as long as you're aware of this, you won't have crash issues like PocketMine did. I made the assumption that JsonMapper would fully validate the data, but it does not.

The referenced commit disables object hydration for arrays. Since PM always uses bStrictObjectTypeChecking in security-sensitive pathways, this was the easiest duct tape I could come up with that didn't break BC.

In addition, PM isn't a web application, everything is handled within a single process, so issues that would just crash one web request will take down a whole PM server due to the architecture, hence the higher vulnerability rating in PocketMine-MP. A tool can be used several ways :)