[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.

[jtnicholl]

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.

No, it is not a large compat break. Fixing ConfigFile is as simple as adding one line, and fixing str_to_var is a simple find and replace. Not to mention that very few people will be affected by this in the first place.

A big compat break would be something like the reverse Z buffer change that happened in 4.3, that required a large number of shaders to be rewritten and affected most non-trivial 3D projects.

That's irrelevant, if someone can't just throw their code into Redot from Godot then it would defeat the point.

How so? What even is the point? Because I didn't think compatibility had anything to do with it.

Wanting to maintain compatibility is good, but it can't be absolute. It's not reasonable to expect 100% compatibility with Godot as Redot diverges, Godot doesn't even keep 100% compatibility with itself. There are compat breaks in every release, often from places you wouldn't even expect:

• Add a new class to the engine? GDScript has no namespaces, so that's a compat break. When Godot 3 added occlusion culling, I had to update a bunch of scripts in one of my projects because I already had my own class called Room, which I was forced to rename.
• Fix a bug? Someone may have written logic to work around that bug, knowingly or unknowingly. Fixing it will break their project.

These examples are so minor no one even considers them, but the point is that there is a spectrum. The cost of a compat break vs the benefit has to be weighed.

The number of people who would use this project setting is minuscule, and the number who would actually need it is zero. Options have a maintenance cost and adding them for every little obscure, niche use case has the potential to get out of hand.

Just to be clear I don't hate the idea, I don't think it's a huge deal either way, but your comment included "Thoughts?" so I gave them.

[Spartan322]

No, it is not a large compat break.

I'm not arguing this, it is, it breaks compatibility with Godot and makes it impossible to port over to Redot without changing the source code unless accounted for, that is a break, it violates SemVer, that's by definition a major problem, we are not negotiating compatibility with Godot out of Redot here period, END OF DISCUSSION.

[jtnicholl]

No, it is not a large compat break.

I'm not arguing this, it is, it breaks compatibility with Godot and makes it impossible to port over to Redot without changing the source code unless accounted for, that is a break, it violates SemVer, that's by definition a major problem, we are not negotiating compatibility with Godot out of Redot here period, END OF DISCUSSION.

You should consider actually reading posts before replying to them.

[Spartan322]

Add a new class to the engine? GDScript has no namespaces, so that's a compat break.

That's not a compat break, not even remotely.

Fix a bug? Someone may have written logic to work around that bug, knowingly or unknowingly. Fixing it will break their project.

That's almost never a compat break.

The reason I don't care to bother thinking beyond this is because didn't actually demonstrate you even understand what a compat break is, a compat break doesn't just mean a change to the API or behavior, it specifically means a change to the API that is not backwards compatible. If the API did not remove something that's not a compat break, if behavior changes in such a way that previously valid behavior is prohibited that's a compat break. We will always intend to support conversion from Godot without question for as long as possible and I will bend as much as I can to ensure we minimize conversions, that's simply non-negotiable and will be for a while into the future.

[jtnicholl]

If it's not negotiable, then how about next time, you don't ask for opinions on the idea, and then get mad and start shouting at people in all caps when they give you what you asked for?

I am a former Godot contributor, I'm not some troll. Even if a random person with a new account were to come in and say something objectively and very obviously wrong, that is not an excuse to immediately assume malice and jump at their throats the way you did.

And now I have to repeat my earlier question: What is the point of this fork? Because the original mission statement was to allow people to contribute and to participate in a community when they had been excluded by Godot. In other words, to be more inclusive of differing ideas. Your response to a civil attempt at discussion was the exact opposite of that.

And if that isn't the point, there is no point. Godot is MIT licensed. If a piece of software is FLOSS, you don't have to care one bit about who made it. Its developers could start murdering babies and there still wouldn't be any immediate reason for anyone to stop using it. Forking it just to offer the same thing with a different name is pointless.

[Spartan322]

If it's not negotiable, then how about next time, you don't ask for opinions on the idea, and then get mad and start shouting at people in all caps when they give you what you asked for?

Your ideas were considered and the crucial part is being implemented, but not everything you said was either correct or even a valid argument, you don't know what "maintaining compatibility" is and you completely disregard basic software requirements and you refuse to listen insisting that you're the only one that can be correct. We are not building an engine to be completely butchered into an unrecognizable state, we don't get to butcher the engine, we need to consider every user, not just your niche. I already gave you security by default and you still insist that's wrong, that means there is no compromising with you because you refuse to consider anyone who disagrees. That's not my fault, that's why I'm being deliberately harsh to you, it doesn't matter if you think it won't effect many if any users, the fact that they can exist is all that matters, that's how you maintain legacy rightly in a professional space, if you still can't understand that then there is literally no talking to you reasonably.

And now I have to repeat my earlier question: What is the point of this fork? Because the original mission statement was to allow people to contribute and to participate in a community when they had been excluded by Godot. In other words, to be more inclusive of differing ideas. Your response to a civil attempt at discussion was the exact opposite of that.

The mission of Redot is about improving on Godot (for now) and being a community that seeks to be apolitical and not cult-like while contributing to the engine, whether that counts as inclusive or not is up to each individual to decide, it means things like not banning people from the community for disagreements, but that doesn't mean every single thing someone suggests is gonna be called a good idea, there is such things as incorrect and poor ideas, or good ideas that include poor ones, and for those ones it needs to be pointed out there are incorrect ideas. It does not mean the idea is disregarded, its about arguing whether an idea is good or not, but when you insist on an idea like you have that literally cannot work and you refuse to listen to disagreements about it, there is nothing else that can be done but putting a foot down.

And if that isn't the point, there is no point. Godot is MIT licensed. If a piece of software is FLOSS, you don't have to care one bit about who made it. Its developers could start murdering babies and there still wouldn't be any immediate reason for anyone to stop using it. Forking it just to offer the same thing with a different name is pointless.

You missed the point of Redot already, we don't want to completely abandon the Godot lineage because its a very useful thing to keep, (else a fork is worthless, especially when you have a small team like us) we just don't like how it was run, how the community "functioned", how slow it moved, how it disregarded things people needed, that does not mean that there aren't (technical) red lines we won't cross or disagreements we won't have. That's a mandated requirement for any software project, community, or FOSS in the first place.

I am a former Godot contributor, I'm not some troll. Even if a random person with a new account were to come in and say something objectively and very obviously wrong, that is not an excuse to immediately assume malice and jump at their throats the way you did.

Also that's not remotely related to what I said, I never presumed malice and whether you were a Godot contributor or not is irrelevant to me, I really could not care about credentials either way, if someone can prove themselves with good ideas and good arguments then I can work with them, or if they could be convinced that something they said might wrong or doesn't make sense when one makes arguments for then I can also work with them, but if someone insists on telling me core fundamental needs of software maintenance are functionally not a concern at all, that's just simply false, I really don't care who you were, you could be John Carmack, Linus Torvalds, Peter Molyneux, my best friend, or my worst enemy, a CS PhD or a high school dropout, an experienced professional or a complete amateur who doesn't know anything, I care for the ability to make an argument and handle discussion reasonably.

[SkogiB]

@jtnicholl Compat breaks create massive logistical problems for our small team. Yes, someday we will diverge so much that its a non issue, but at this time that isn't on our radar. We've discussed that would be aimed at a 5.0 release someday, to ensure some kind of stability. On top of that is just the workload we can't maintain. Godot's upstream work keeps the 4.4dev moving forward without us having to do it all too, so we can focus on putting in work as extras on top.

[jtnicholl]

Your ideas were considered and the crucial part is being implemented, but not everything you said was either correct or even a valid argument, you don't know what "maintaining compatibility" is and you completely disregard basic software requirements and you refuse to listen insisting that you're the only one that can be correct. We are not building an engine to be completely butchered into an unrecognizable state, we don't get to butcher the engine, we need to consider every user, not just your niche. I already gave you security by default and you still insist that's wrong, that means there is no compromising with you because you refuse to consider anyone who disagrees. That's not my fault, that's why I'm being deliberately harsh to you, it doesn't matter if you think it won't effect many if any users, the fact that they can exist is all that matters, that's how you maintain legacy rightly in a professional space, if you still can't understand that then there is literally no talking to you reasonably.

I have not insisted anything, or refused to consider disagreement. I wrote at the end of my second comment, the one you didn't bother to read:

Just to be clear I don't hate the idea, I don't think it's a huge deal either way, but your comment included "Thoughts?" so I gave them.

And after that I didn't bring up the compat break topic again. You're the one who keeps making everything about that, completely missing the point of what I've said since.

Again, I was asked for an opinion, and so I gave it. If this is how you're going to respond to people who are making an effort to contribute, just for saying something you don't think is accurate, don't expect to continue seeing contributors.

@jtnicholl Compat breaks create massive logistical problems for our small team. Yes, someday we will diverge so much that its a non issue, but at this time that isn't on our radar. We've discussed that would be aimed at a 5.0 release someday, to ensure some kind of stability. On top of that is just the workload we can't maintain. Godot's upstream work keeps the 4.4dev moving forward without us having to do it all too, so we can focus on putting in work as extras on top.

That is completely reasonable and all that you needed to say.

[SkogiB]

@jtnicholl Compat breaks create massive logistical problems for our small team. Yes, someday we will diverge so much that its a non issue, but at this time that isn't on our radar. We've discussed that would be aimed at a 5.0 release someday, to ensure some kind of stability. On top of that is just the workload we can't maintain. Godot's upstream work keeps the 4.4dev moving forward without us having to do it all too, so we can focus on putting in work as extras on top.

That is completely reasonable and all that you needed to say.

We do appreciate the work people put in, just have to manage everything properly. Software guys are all hard headed, it comes with the territory, but I generally don't permit squabbling if I can put a stop to it. No fingers pointed, just the way it is. I think everyone has both been on the giving and receiving end of this sort of dispute in software. Positions can be stated succinctly and a decision made from there, that's all that matters