Repository navigation
Limit nesting depth of custom variables - #10940
julianbrost wants to merge 1 commit into
Conversation
Recently, with fb42312, we've added a limit to how deeply nested data structures can be created from API requests to limit the resulting recursion depth when processing them to prevent stack overflows. However, this resulted in a slight inconsistency (not relevant for security) where you may create custom variables with different nesting depth limits depending on the exact way to create them. This change now introduces an additional validation for custom variables consistently limiting there overall depth. This was not done yet as part of the security fixes as this also affects existing configuration in a way that might cause existing configuration now fail the validation. The limit of 16 should be large enough so that should affect none or very few users.
|
I almost went crazy while doing and writing down the tests for the PR description. I hope you don't face the same problem when looking at them. 😬 |
| if (path.size() > VarDepthLimit) { | ||
| BOOST_THROW_EXCEPTION(ValidationError(this, path, "Variables nested too deep.")); | ||
| } |
There was a problem hiding this comment.
This throws ValidationError while the preexisting checks in ConfigObject::ModifyAttribute and ConfigWriter::EmitScope throw a std::runtime_error. I'd like some consistency here, but either direction is fine with me.
It's a bit annoying because ConfigWriter::EmitScope doesn't have an object so ValidationError might need another constructor for that.
| // nested data structures that could overflow the stack later on. It is done in addition to the check in | ||
| // CustomVarObject::ValidateDepthLimit() which is indirectly called by the validation function below. |
There was a problem hiding this comment.
I don't know which "validation function below" this is in reference to. Presumably it means CustomVarObject::ValidateDepthLimit, but that isn't "below" (well it is in this GH diff, but not for anyone reading the code in their editor).
| } | ||
|
|
||
| if (auto array = dynamic_pointer_cast<Array>(obj); array) { |
There was a problem hiding this comment.
| } | |
| if (auto array = dynamic_pointer_cast<Array>(obj); array) { | |
| } else if (auto array = dynamic_pointer_cast<Array>(obj); array) { |
(either that or early return;)
| DECLARE_OBJECT(CustomVarObject); | ||
|
|
||
| void ValidateVars(const Lazy<Dictionary::Ptr>& lvalue, const ValidationUtils& utils) final; | ||
| void ValidateDepthLimit(const Value& value, std::vector<String>& path); |
There was a problem hiding this comment.
This should be const and private (or inlined into CustomVarObject::ValidateVars()), or are you anticipating that this will be used in other places in the future?
Making it static (or TU-local) would also be an option, but it uses this for the ValidationError.
| MacroProcessor::ValidateCustomVars(this, lvalue()); | ||
|
|
||
| std::vector<String> path {"vars"}; | ||
| ValidateDepthLimit(lvalue(), path); |
There was a problem hiding this comment.
Maybe could use a temporary Dictionary::Ptr so lvalue() only runs once.
Recently, with #10908, we've added a limit to how deeply nested data structures can be created from API requests to limit the resulting recursion depth when processing them to prevent stack overflows.
However, this resulted in a slight inconsistency (not relevant for security) where you may create custom variables with different nesting depth limits depending on the exact way to create them. This change now introduces an additional validation for custom variables consistently limiting there overall depth. This was not done yet as part of the security fixes as this also affects existing configuration in a way that might cause existing configuration now fail the validation. The limit of 16 should be large enough so that should affect none or very few users.
Note that the limit of 16 includes the outer
varslevel, so there is room for 15 user-defined levels.Also note that while testing, you might run into a small issue described and fixed in #10939 that happens with our handling of modified attributes if the validation fails.
Tests
Current master (8ff3d58)
Nested JSON exceeding JSON depth limit of 24 fails with an error.
Nested JSON just below depth limit creates a nested object in vars with 23 custom labels.
vars.d.o.t.t.e.d notation above the limit of 16 returns an error
vars.d.o.t.t.e.d notation just below depth limit creates a nested object in vars with 15 custom labels
Mixing vars.d.o.t.t.e.d with nested JSON allows creating up to 37 custom nested labels
This PR (5440fcb)
With nested JSON, the allowed nesting depth is reduced. 16 custom label levels are now enough for the validation to reject it before the JSON parsing would reject it.
The error message is quite lengthy and very unreadable if formatted as a JSON string. Decoded, it looks like this:
Reducing the nesting level by 1 to 15 nested custom variable labels is still accepted, so that is the new maximum here.
The limits for vars.d.o.t.t.e.d remain unchanged, 16 is rejected.
Whereas vars.d.o.t.t.e.d with 15 custom labels after vars is still accepted.
Mixing vars.d.o.t.t.e.d with nested JSON objects so that a total of 16 custom labels after vars is reached is now rejected.
Again, the somewhat more readable error message after JSON decoding:
Removing one level from vars.d.o.t.t.e.d makes that the object is accepted with 15 custom labels after vars.
Alternatively, removing one level of nesting from the JSON object also allows the object to be created with the same depth.
This limit is now also enforced in objects defined in config files, 16 levels after vars is rejected.
Reducing the nesting level in the config file by one results in a valid object again.
And finally, querying all those config objects that maxed out the limits show a consistent limit across all methods to create an object.