search

netplex / json-smart-v2

Apache License 2.0
151 stars 68 forks source link

net.minidev.json.parser.ParseException: Malicious payload, having non natural depths. #154

Closed brad302 closed 6 months ago

brad302 commented 1 year ago

As I'm sure you're aware, this package is used by Talend by two key components, tExtractJSONFields and tFileInputJSON.

We're currently upgrading to Talend V8 which brings with it v2.4.9 of this package after having been on v2.4.7 for quite some time.

Can I ask why that exception was introduced because at present, it's halting our upgrade. We have a seemingly innocuous sized JSON structure that is now failing because of the package upgrade.

My question is, how can we possibly overcome it? It's quite the issue.

Thanks

Brad

shoothzj commented 1 year ago

@brad302 I think it has been discussed this in https://github.com/netplex/json-smart-v2/issues/131#issuecomment-1474092205 Could you please provide us the case? We can add an argument to customize this limit if need. cc @UrielCh

shoothzj commented 1 year ago 
seemingly innocuous sized JSON structure

I think this error only occurs when json size very large. Could you please provide a minimal reproduced case?

brad302 commented 1 year ago 
seemingly innocuous sized JSON structure

I think this error only occurs when json size very large. Could you please provide a minimal reproduced case?

Yep, have attached one.

When I say "innocuous" I mean, I would expect it to be able to be processed. I consider the payload to be large enough but I have seen much larger.

Let me know how you go. Thanks payload.zip

shoothzj commented 1 year ago

@brad302 @UrielCh I hava created a draft pr #155 for this issue. PTAL

brad302 commented 1 year ago

What are the chances of having the default value substantially increased? My issue is, when this new version is released, there's nothing to say that Talend will follow suit and provide the ability to enter a value and override the default.

It may be a big ask but something like 2000 (rather than 400) would help a lot. I'd say that would still alleviate any issues relating to a DDOS attack but not sure if it would then re-expose the CVE.

Happy to get your thoughts.

UrielCh commented 1 year ago

If a real non-malicious application needs more, adding a flag to drop this security may be a better choice, and increasing the current 400 to more like 1000, is fine, 400 never blew up any stack, and 1000 is still kind of okay.

UrielCh commented 1 year ago

discussions are in progress in the draft #155

shoothzj commented 6 months ago

I think we can close this issue now, see #156 to drop the limit.