Adding Safeguards against loading of potentially untrusted resources

[betalars]

Describe the project you are working on

A game that has an arrangement of nodes that need to be saved and loaded.

Describe the problem or limitation you are having in your project

I want to just save the scene of the nodes as-is, to not have to bother re-initializing the state of the whole scene. However if I understand it correctly, this would create a possible remote code execution vulnerability when users share their save files.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Add an optional trusted_stand-in variable to ResourceLoader.load(), load(), and ResoruceLoader.load_threaded().

When Resources are loaded and contain scripts (or other potentially hazardous things), they are discarded and loaded in as null. Godot then tries to replace any components that failed to load with the trusted stand-in.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

var trusted: Resource <- trusted stand-in
var untrusted: Resource <- untrusted loaded resource
var result

for property in untrusted.get_property_list()
    if property is dangerous:
        result.set(property.name, trusted.get(property.name)
    else:
        result.set(property.name, untrusted.get(property.name)

If this enhancement will not be used often, can it be worked around with a few lines of script?

I have worked around this, but it is extra effort and I frankly don't think we should assume game devs are knowledgable and caring enough to put in mitigations against this risk.

Is there a reason why this should be core and not an add-on in the asset library?

People may not know the Addon Exists and their users would be voulnerable to remote code execution when they share save games.

[betalars]

Adding a few comments for discussion: If I remember correctly, there are already some safe guards protecting online players from contamination. We could potentially reuse some of that code. I have not looked into it, but it seems plausible.

[betalars]

having a dedicated load_resource_untrusted method might be a less breaking change. However preserving a vulnerability for convenience seems bad.

And I don't think this proposal would break stuff in most instances. Loading a script via resource loader seems like a rare thing to do and we can raise a warning/error when it is replaced by null and no stand-in was provided.

[betalars]

Related: https://github.com/godotengine/godot/pull/80585

[AThousandShips]

With arbitrary code execution the threat is generally when loading at all, the act of loading causes the code to be executed, or can be made to do so, I don't see how a system where code can be both loaded and not could be made safe without just making the whole system safe and avoiding code execution at all

[betalars]

Yeah ... by now I was going under the umpression that it was a simple matter of godot scripts being attached to resources when loaded and allowing for remote code execution that way, but this is a rabbit hole of much more depth I now realize.

[AThousandShips]

As far as I know trying to fix this on the loading side in general is just a game of whack-a-mole, there's plenty of cases where you just can't differentiate safe behavior from unsafe behavior without deep behavior analysis.

So the general solution is to instead use sandboxing, to just have code you don't know that you can trust run in a safe environment where you can control what it is allowed to do or not

For example, assuming you have a node that has attached resources or nodes, how could you prevent malicious code there? What if one of the resources is just extended with a malicious class, how would you limit that to just classes you trust? And even then there's always things you can't necessarily predict

So trying to solve things by trying to plug potential holes is generally doomed to fail as you are only protected for cases you can imagine, and even then there's always the room for exploits and things.

See:

• https://github.com/godotengine/godot-proposals/issues/5010

[betalars]

Thinking of this, code signing came across my mind. This of course is really difficult due to the nature of Godot Script, but maybe we can do something along those lines could be used to prevent execution of anything that has not been known at the time of export?

[AThousandShips]

How would that work with checking files? How would it guarantee that it doesn't add code where it shouldn't? Why not just stop anything that includes code instead?

If the code is known on export you'd expect it to just use the code in the game, not code that just matches some signature, also there are various ways to beat code signing

[Maximilian-Seitz]

The way I've attempted to solve this issue in my project is to have a sort of "resource loader", which can load resources (from JSON or binary), but requires the class-name of the resource that's going to be loaded. This resource then needs to have static types for all the exported variables, which need to have types that are considered 'safe', like other fully typed Resources, Arrays with fully typed contents, integers, etc.

This means I always know what is being loaded, because the data is either plain data, or a Resource that I've explicitly exported (in my code, which I have control of), which is filled with data that I know about.

The main problems with this solution (as of right now), are that it doesn't support inheritance (like exporting a certain type of Resource, but getting an instance of a child-class thereof), and that certain types can't be fully typed (like a Dictionary and Array inside another Dictionary or Array). The first 'issue' is by design, and the other something Godot will hopefully eventually have.

A smaller issue is that the types of what's being parsed need to be passed as Strings, since native types don't really have an API, and Script instances are quite limited in their API to get the class-name thereof.

Nonetheless, a function like this built into Godot would likely solve most cases where people save/load Resources in the user-directory.

[betalars]

@AThousandShips wrote:

Why not just stop anything that includes code instead?

Okay but this was my initial proposal: not parse anything that could include code from untrusted sources and optionally replacing it with a trusted stand-in to allow lazy users to kind of save resources with code but not actually load that code from an untrusted source. I understood that you said these kinds of checks wouldn't actually help when loading data from text files in preventing remote code execution.

[AThousandShips]

An important part of any attempt at security is to analyze the potential attack vectors and risks, what situations are you expecting? What kind of potential attacks and damage? Are we talking infected save files shared online? Network attacks? Are we talking attacks that just ruin your save, or something that hacks your computer? Installs a virus? Wipes your hard-drive? Etc.

Complete security is never possible, you need to tailor your solutions to your expected problem

The one thing you do mention is users sharing their saves, but that's still very vague, and I'd be interested in some research that this actually is a real threat, I don't really see a real risk of this or what it would achieve generally. I don't expect people to be sharing their save files all that much, except for testing and debugging etc., and any game that encourages that for what ever reason should instead have a robust system for doing various kinds of data sharing instead of this very crude approach, and I'd expect most cases of sharing this kind of data to be in already trusted environments, I have a hard time imagining situations where you'd share and run save files with complete strangers, with no moderation etc. And with plain text files it's incredibly easy to just check the files for malicious stuff manually, which would be the only real reason to share text files in the first place instead of raw binary files (which have more security already, at least when using marshalling, though sharing raw resource files is inherently unsafe because they should support complex data)

I have worked around this, but it is extra effort and I frankly don't think we should assume game devs are knowledgable and caring enough to put in mitigations against this risk.

I think this is a bit of a problem, I think that people who need safety features in their development must understand basic or even higher level safety principles and procedures. You can't ever really provide a complete kit you can just use without any knowledge about broader safety principles, it requires both having the tools for it and the knowledge to use them safely

---

If we're talking about the safety of save files in particular I'd say that the better general solution is to not rely on resource files at all, they aren't IMO necessarily appropriate from a broader perspective, not just safety wise. If you want better security you should write your own serialization for your specific data, it is both safer and more robust for a lot of purposes, and more compact. This can be done by just writing data directly to a file in a structured way, this can be made more dynamic relatively easily with various kinds of headers, but you can still ensure completely that no code is run from it by just not deserializing objects at all.

[betalars]

I think it boils down to the swiss cheese approach to security.

Users do share save file, it happens. And you are correct about it happening mostly in moderated environments. This is one layer of protection. Being able to tell Godot to load a resource without accepting any executable components would add another layer of protection to mitigate risks of being compromised by a malicious safe file, because it makes creating a malicious file a lot harder. Creating a custom serializeer and deserializer is adding another layer of protection, that should arguably be done by games that encourage sharing of game data, but a lot of developers are going to be lazy and not put in that effort.

We cannot rely on files only being shared in moderation, we cannot rely on moderation always being effective, we cannot rely on the blacklisting of executable resources always working, we cannot rely on devs always doing proper serialization, we cannot rely on signing, but each layer decreases the chance of a successful attack from occurring.

[AThousandShips]

So let's break this down a bit, this proposal talks about more active and selective safety, not so much basic levels of blocking low level arbitrary code execution. Note that for binary serialization we already provide the option to not load object data or scripts etc., the same has been proposed for text based formats but it is a far more complex question there and has been held back due to concerns of completeness among other things.

So as a basic protection of ensuring you have the chance to check for safety, i.e. preventing random loading of objects from random files without any control, that's achievable, generally, though I'm sure someone can work out an attack with enough effort. That would make it possible to ensure some files just aren't supposed to contain object data but just raw data, that should be enough for a lot of cases and generally loading random object data from files would be the baseline.

But anything above that requires at least some level of involvement by the developer to achieve safety, at least basic awareness and using common mitigation patterns.

but a lot of developers are going to be lazy and not put in that effort.

I don't think we can or should try to solve the situation where the developer makes no effort at all to provide safety, or has zero degree of security awareness, even basically promising zero-effort safety is just inviting more carelessness IMO

We cannot rely on files only being shared in moderation

What I meant was that the case I proposed is one where you would download unsafe save files at your own risk, I don't run random executables on my computer even though I have antivirus software running, it's hard to provide safety in situations where the target audience has no degree of sense, my point being that I would like to know the scope of this risk, and that I question it being significant, we can't necessarily be entirely fool-proof

---

So, at the core I'd say that providing "black box" safety features is a bad idea, i.e. safety features where you just check a box and it's now "safe". It invites using it without understanding general safety, and promising things that almost always additional work to make it actually safe. Almost all general safety things require at least some effort beyond the feature itself, and marking things as "this is safe" has a risk of causing carelessness because people just see "safe" and think that they are done.

Now what I can see here as a potential idea would be to use some kind of annotation to control sandboxing, having some properties marked as "sandboxed" would make a lot of sense generally for this.

In the end I'd say that the tools are only as effective as they are used, if you don't use them or use them incorrectly they won't help, and there's a limit to how fool-proof we can make it.

If you download a strange executable from a strange website and don't run a scan with your antivirus on it before launching it there's a significant risk, and general safety for these things aren't necessary designed with utterly unsafe risk taking in mind. And promising that without expecting developers to add a level of knowledge and intent on their own is inviting carelessness, we should expect developers to do their own homework to ensure safety for their users IMO.

[dalexeev]

I've expressed my opinion on this a few times, so I'll just summarize the main points without going into too much detail.

I support the idea that save files should be safe by default, I believe that we should not allow code where it is possible and expected only data. While absolute safety is impossible and a programmer can always shoot themself in the foot by incorrectly processing data, that doesn't mean we can provide an API unsafe by default, especially when the risks aren't even documented.

However, I don't think Godot resources (.tres/.res/.tscn/.scn formats) are designed or intended to be loaded from outside res:// (which should be considered a trusted source). Also, Godot resources are bad for versioning save files and tightly couple data and implementation.

My efforts were focused on text serialization (var_to_str(), ConfigFile), not resources. I made PR godotengine/godot#80585, but it turns out that this approach still has security risks in that critical code is spread across multiple places. reduz said that if we want to implement this, the code should be organized the same way as binary serialization.

[betalars]

@dalexeev issue being config files still allow remote code execution as far as i have been told.

[CycloneRing]

Sandbox it! https://github.com/libriscv/godot-sandbox