JSON/YAML: error if a maximum nesting is reached (prevents stack overflow) #6322
Conversation
|
Oh, I was sure we had this behavior somewhere. It turns out we have it when parsing. But it makes sense to have it also when emitting. |
|
I can totally see the security risk for parsing and we definitely should have it there, but for generating, aren't we just band-aiding not having a proper stack overflow error message here? It feels like the gain would be the same (= happy user being notified about their mistake), just everywhere instead of this specific instance. That said I'm not blocking this. |
|
I don't think we should prevent stack overflow: I think we should let it happen and fix the stack overflow error message. As for parsing, time taken to parse the JSON isn't |
|
I think, once you get stack overflow, there's not much you can do with the app. If we throw JSON::Error that's manageable, and it can be logged, the server can continue working, etc. |
|
The nesting attack on parsing isn't DoS through resource consumption but DoS through crashing the server repeatedly (and maybe indirect resource consumption of it restarting all the time). |
|
@jhass ah I see, well the fix to that is perhaps stack overflow crashing only one fiber. Of course, that has a bunch of other issues around resilience and making it easier to DoS by softlocking apps by crashing individual fibers... Okay, if we assume we can't raise a normal exception in the case of stack overflow (if we can raise normally, this is a moot point), then yes crashing is the only option. Then preventing it in the JSON parser is a good idea. |
Fixes #6321
This PR adds a
max_nestingproperty to bothJSON::BuilderandYAML::Builderwith a default value of 99. When trying to generate a JSON/YAML document with a nesting (arrays/objects inside arrays/objects) greater than the maximum, an error is raised.Other languages usually do the same, because it's super rare (I'd say unmanagable) to have a JSON/YAML document that deep. There was also a website listing how JSON is handled in each language, and checking for a maximum nesting was done and most languages did this: those that not were marked as red, because it means the language is not safe. And it's not safe because it will cause a stack overflow, which usually results in the end of a program.
The error message is copied from Ruby.
Of course there's no easy way to control this maximum nesting calling just
to_json,to_yaml. However, in the rare case you need to change this maximum nesting you can always creating a builder, configure it, and pass it toto_json/to_yaml.