-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Godot] str_to_var
and ConfigFile
allow for arbitrary code execution
#760
Comments
str_to_var
and ConfigFile
allow for arbitrary code executionstr_to_var
and ConfigFile
allow for arbitrary code execution
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 |
Turn out |
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.
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. |
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.
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.
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. |
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.
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:
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. |
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. |
That's not a compat break, not even remotely.
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. |
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. |
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.
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.
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.
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. |
@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. |
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:
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.
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 |
Tested versions
All verisons
System information
Ubuntu 24.04
Issue description
The global method
str_to_var
and ConfigFile'sload
/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.
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.
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.
The text was updated successfully, but these errors were encountered: