Core: Add allow_objects/full_objects parameter to text serialization

[dalexeev]

This PR adds the allow_objects/full_objects parameter to the VariantParser/VariantWriter internal API that is used for the str_to_var()/var_to_str() functions and ConfigFile methods.

I think security reason enough to break compatibility in this case. Let me know if you think compatibility is more important and we should provide users with an insecure API by default, even if it's inconsistent with binary serialization.

• Closes The fact that str_to_var and ConfigFile deserialize Objects is likely to cause security issues #80562
• See also Provide a way to load resources without running scripts godot-proposals#4925.

Production edit: closes godotengine/godot-roadmap#69

[jtnicholl]

I do think this is worth breaking compatibility for, at least in 4.2 and maybe 3.6. I don't have any statistics, but I doubt many users are deserializing objects intentionally, and even if they are it will be a simple fix to get it working again.
Edit: Actually, I'd bet a large amount of users deserializing objects intentionally are only doing so because they don't realize it's a security risk, making this more of a reason we should change this than shouldn't.

If possible, though, it might be useful to have an error message when not parsing an object, telling you it was skipped for security reasons and how to fix it if you want to. I'd add one for binary deserialization too if there's not one already.

[dalexeev]

I skipped most of the editor's ConfigFiles except ProjectManager since it uses ConfigFile to open project.godot which can contain InputEvents for example. This might cause regressions, but I don't think we should add allow_objects = true everywhere. But this PR should be carefully tested.

[dalexeev]

Can *.import and *.remap files contain objects?

[jtnicholl]

Can *.import and *.remap files contain objects?

*.import files can, I remember someone suggested it as a way to fix #68936, though I don't think it's currently used. Edit: Actually it is used, for BoneMaps.
For *.remap files, I'm pretty sure they only ever contain a single string, in which case the answer would be no, but I'm not positive.

[dalexeev]

Actually it is used, for BoneMaps.

Thanks, I found 2 matches for ImportOption(PropertyInfo(Variant::OBJECT. Plus custom plugins can use this too. So I'll update the PR to allow objects for *.import files in the editor.

godot/editor/import/resource_importer_scene.cpp

Line 1623 in 5444afa

r_options->push_back(ImportOption(PropertyInfo(Variant::OBJECT, "physics/physics_material_override", PROPERTY_HINT_RESOURCE_TYPE, "PhysicsMaterial"), Variant()));

godot/editor/import/resource_importer_scene.cpp

Line 1701 in 5444afa

r_options->push_back(ImportOption(PropertyInfo(Variant::OBJECT, "retarget/bone_map", PROPERTY_HINT_RESOURCE_TYPE, "BoneMap", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), Variant()));

[AThousandShips]

Looks correct to me, didn't verify all the possible places it's used but the logic behind it makes sense, no specific remarks on the docs (Mickeon seems to have it under control!)

[Faless]

Really great work 🚀 , the code looks good, and I've tested it with a few projects with no issues.

I was thinking that maybe API-wise having a Config.allow_object_decoding property would be simpler (like we do in SceneMultiplayer), but I'm not sure it's better than being explicit in the methods parameter (which is maybe less surprising to the users).

[reduz]

While I am not against this change, this PR should not change absolutely anything by default.
These parameters should only be available to the user when using custom code, but for Godot file formats this should remain able to load objects (and full objects) by default.

[dalexeev]

I was thinking that maybe API-wise having a Config.allow_object_decoding property would be simpler

I thought about this, but there is a question about what should happen when the property changes true -> false. Should we prohibit this and allow only false -> true, one way (like there is Array.make_read_only(), but there is no reverse method) or maybe allow it only when the config is empty.

Also, should set_value(sect, key, object) produce an error when allow_objects property is false?

The current implementation is more clear on these issues, since full_objects/allow_objects is a parameter of the read/write operation, not a "hidden" property of the ConfigFile object. This is more explicit if, for example, a function takes ConfigFile as a parameter.

[reduz]

To clarify, this is not a real security risk in any way for existing code, so nothing needs to change.

[reduz]

Let me clarify further why this is a bad idea. There are two types of security at play here:

• Real security, where you care that what you are doing can't become an attack vector for common types of exploits (such as stack or memory overwriting, predictor attacks, etc).
• Illusion of security, when it looks like you are avoiding user to do something by logic, as in, not allowing to load a script.

The problem is that, if you are doing something where security really matters (example, an online or multiplayer game) you can only trust the first, never the second. If you trust the second, then all you are doing is providing a more skilled attacker with a larger surface area to exploit the first point.

This is why, on the Godot side, the only real security measures are when handling marshalls, protcols and binary files. Everything else, the attack surface is too large to protect for real against a skilled attacker.

Installing features like this where it seems like you are having the illusion of security, when in reality you are having none is just bad from my point of view. As example, binary configuration files are far safer than text configuration files, because the surface of attack is much smaller.

You may feel this is not the case and that text configuration files are safer because you can see them, but at the same time you are afraid of scripts embedded in them that people may not see? This just makes no sense honestly, think about it for a second.

So, ultimately, I think its ok if we add only a change to var_to_str and str_to_var, but then nothing else in the engine should change, because this will simply give users a false sense of security.

Instead, I think we should discuss security guidelines in the documentation so users understand what APIs can they really trust and why in each situation.

[reduz]

Alright, after discussions in the #core channel in rocketchat, the following was decided:

• We don't want to expose unsafe code to users and give them a false sense of security.
• There is a belief that we can make the parsing code safe.
• This will require moving all the parse logic to a single file (even if it means duplicating some code such as integer/float parsing, or creating a separate string_safe.cpp file for common stuff such as unicode/integer/float/etc parsing).
• This will make it easier to audit these files and invoke the necessary technical expertise to review changes every time the file is modified (any file considered safe needs specialized technical review and frequent auditing to ensure its safety against exploits)
• Once we do this, then we can implement proper safety features in text formats.

So I guess this PR is dependant on this, and it will need to be moved to at least 4.x, as I doubt everything will happen in the current time-frame.