The fact that str_to_var and ConfigFile deserialize Objects is likely to cause security issues

[jtnicholl]

Godot version

v4.1.1.stable.official [bd6af8e]

System information

Ubuntu 23.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 it's not uncommon for players to share save files online (especially for games where a lot of content requires unlocking).

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 (hence why I'm opening this publicly), the docs don't mention it.

IMO this is too dangerous to have as the default behavior, even if it were documented. There's already separate bytes_to_var and bytes_to_var_with_objects methods so it's not like this wasn't though of, I don't know why text data wasn't treated the same.

Steps to reproduce

This is a minimal example that will print "Arbitrary code" as soon as it is parsed.

"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

https://github.com/godotengine/godot-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 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"
	}
}

[HolonProduction]

This definitely needs clarification in the docs. Reading the following This helper class can be used to store Variant values on the filesystem I think it is reasonable to assume that some people (like me) think that ConfigFiles does not support objects. That was why I switched from a custom json solution to ConfigFile in the first place.

[daveTheOldCoder]

I just did a test using Godot4.2-dev6. I was able to duplicate the exploit described above for ConfigFile.

But the vulnerability can be reduced by using the option to encrypt the file.

When I change the saved config file to encrypted, and I modify the saved file, the loading of the file fails with the error ERR_FILE_UNRECOGNIZED or ERR_FILE_CORRUPT, depending on where I insert the text, and the malicious code does not run.

This solution assumes that the encryption password is not known. If you're able to discover the password, which is not impossible, then you could decrypt the file, add the malicious code, re-encrypt the modified file and run the app.