Provide a way to load resources without running scripts

[dploeger]

Describe the project you are working on

I'm the core developer of EgoVenture, a first person adventure game framework and a contributor to Escoria, a third person adventure game framework. Both projects are using Resources to provide a savegame functionality.

Describe the problem or limitation you are having in your project

As recommended in the official Godot docs, Resources should be used to store and retrieved structured data.

This can be used e.g. to persist the current game's state (aka savegame).

As many godot developers (GDquest, among others) has pointed out: Using resources to store data can be dangerous as resources can also hold scripts which are executed when loading the resource later.

That way, malicious people can send seemingly valid savegames and embed malicious code.

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

As it can be required to load scripts from Resources, Godot should feature a SecureResourceLoader, which specifically skips loading/executing scripts from resources so developers can still rely on resources as the most optimised method for storing structured data.

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

Very simply put, SecureResourceLoader should extend ResourceLoader, override the load/load_interactive methods and not load and execute any scripts in the given Resource.

(this might sound way easier than it is to actually embed it, I still have to dig through the code)

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

No, because loading resources is a core functionality.

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

Loading resources is a core functionality and security should be a core responsibility as well.

[dploeger]

Hm, so I guess the pragmatic way to implement it would be by adding a load_secure method to ResourceFormatLoader and e.g. clean the scripts from resource definitions when loading them e.g. in ResourceFormatText. The SecureResourceLoader would call load_secure instead of load.

Alternatively, we could implement this into the load function of ResourceLoader by adding a boolean secure parameter which defaults to false (to be backwards compatible)...

The not so pragmatic way would be to implement secure casting to resource for every object that could be loaded. But as this is only about .tres resources (if I got everything right), we maybe better implement it on a ResourceFormatLoader level.

[YuriSizov]

We've been discussing it a bit, and it should load scripts, because otherwise you lose the benefits of scripted resources. It should ignore bundled scripts and external scripts loading from the outside of res://.

Another option would be to validate the loading resource against some sort of schema. I can imagine you can refer to your internal type as a schema and the engine would strictly validate serialized members against it. This way you can avoid extra fields being added and extra types being used, preventing the possibility of a scriptless attack.

[dploeger]

The schema would not work that great in my context because it's variable for each game designed with the framework so adding a schema would put that burden on every developer using the framework.

Maybe instead something like an allow_scripts field in the Resource class which could be set to false or something?

[YuriSizov]

The schema would not work that great in my context because it's variable for each game designed with the framework so adding a schema would put that burden on every developer using the framework.

A schema would be resource based. So ideally it would require zero work from the developer beyond designing their resources.

Maybe instead something like an allow_scripts field in the Resource class which could be set to false or something?

Scripts are not the only attack vector here. And it shouldn't be a resource decision, it should be a loader decision. Because otherwise the bad actor can still replace the script in your resource that allows it with their script. Which is why it should filter out scripts based on their quality while loading: if they are bundled or if they are external and not from res://.

[dploeger]

@YuriSizov good call

[NathanLovato]

I'm all for it, and actually, if this proposal gets accepted and an implementation idea vetted, I'd like to offer to sponsor someone to implement this new feature.

[TheDuriel]

Just to bring this over from twitter:

It is my opinion that the correct way to solve this, would be to completely disallow embedding script within any kind of godot resource format. Be that resource, scene, or others. Yes, this would include removing the entire embedded script functionality. (Which already lost most of its usefulness due to lack of feature support for things like class names.)

(I been theorizing a way to inject code through the icon.png and default audio bus files present in all projects. Completely outside of the control of the developer.)

[mieldepoche]

this would include removing the entire embedded script functionality

I personnally use them sometimes, at least when prototyping.

I been theorizing a way to inject code through the icon.png and default audio bus files present in all projects. Completely outside of the control of the developer.

Do you mean, corrupting a pck file? You can litterally inject anything you want inside it really easily.

[TheDuriel]

No, this wouldn't touch the PCK at all. But it's irrelevant in any case.

[NathanLovato]

I'd also lean towards removing embedded scripts, which could be part of solving this, for all their limitations. The way they come to bite you when your project grows and you get errors in embedded scripts, for instance, you quickly lose the little time they saved you initially.

But that likely depends on finding some consensus, I remember quite a few people liked them.

[Sslaxx]

Not sure how relevant this might be, but https://twitter.com/reduzio/status/1549655657183416320 - @reduz here talks about a "JSON-like" format?

[Calinou]

I'd also lean towards removing embedded scripts, which could be part of solving this, for all their limitations. The way they come to bite you when your project grows and you get errors in embedded scripts, for instance, you quickly lose the little time they saved you initially.

This was proposed in the past and rejected: godotengine/godot#54553

Since then, built-in scripts have been improved significantly, so I wouldn't remove them (even though I don't use them myself).

Not sure how relevant this might be, but twitter.com/reduzio/status/1549655657183416320 - @reduz here talks about a "JSON-like" format?

I think reduz is talking about ConfigFile here (whose syntax is closer to TOML than JSON).

[Sslaxx]

Is this also potentially an "abuse" of Resource and its ilk anyway, i.e. it was never intended to be used in this fashion and so any such use will be unsupported? If so, might be worth rewording the documentation to discourage this practice and/or preventing them from being saved to/loaded from outside of res://.

The use of "events": [ Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":0,"alt":false,"shift":false,"control":false,"meta":false,"command":false,"pressed":false,"scancode":16777232,"physical_scancode":0,"unicode":0,"echo":false,"script":null) and similar in files should also be forbidden outside of res://.

[NathanLovato]

I think it doesn't matter that it wasn't initially meant to be used that way if it's possible to support the use case. Resources are a great tool for modding, bringing in user-created data into Godot in general, they have the potential to save you a lot of code and coupling (because of how there's this built-in load() system that'll load resources only once), they can work as text in debug builds for quick bug fixing, and binary in release builds for smaller file sizes and faster loading...

They have pros, they're just lacking several small improvements, in this case for user safety.

[NathanLovato]

This was proposed in the past and rejected: godotengine/godot#54553

Since then, built-in scripts have been improved significantly, so I wouldn't remove them (even though I don't use them myself).

Makes total sense, yeah, if the community uses them then built-in scripts are here to stay.

[SilencedPerson]

I'd also lean towards removing embedded scripts, which could be part of solving this, for all their limitations. The way they come to bite you when your project grows and you get errors in embedded scripts, for instance, you quickly lose the little time they saved you initially.

But that likely depends on finding some consensus, I remember quite a few people liked them.

Maybe it could be useful to not allow embedded scripts on load?

[NathanLovato]

Yes that's already an idea in this conversation, see these two comments by @YuriSizov:

• Provide a way to load resources without running scripts #4925 (comment)
• Provide a way to load resources without running scripts #4925 (comment)

[dalexeev]

Personally, I don't see a problem with the .tres, .res, etc. formats supports script embedding. The problem only arises when this format is used for purposes for which it was not intended.

In my opinion, .tres is not intended for save files and similar external files, the purpose of which is to store data, and not extend the functionality of the game. And it's not just security, but other aspects that I can talk about, but they are beyond the scope of this discussion.

And as mentioned above, you can safely use Resource for external files, but you need to use ResourceFormatLoader for that, not .tres/.res.

At the same time, I am completely in favor of having an analog of the allow_objects parameter in str2var, ConfigFile, etc.

[Sslaxx]

Custom resources (in 3.x) require C++ modules, which may be beyond the scope of some people's intended usage, or skills.

[YuriSizov]

In my opinion, .tres is not intended for save files and similar external files, the purpose of which is to store data, and not extend the functionality of the game.

That's pretty arbitrary. Resources are just scriptable objects with built-in serialization capacity. In C# land you can serialize any class with XML in the same way, and it's used in games all the time. I think, that's the go-to way in Unity, at least from my experience with digging into game saves of games made with it.

So from the ease of use perspective and the level of integration into the engine, custom resources are perfect for storing user data.

[dploeger]

That's also exactly what the docs say. 😉

[dploeger]

Custom resources (in 3.x) require C++ modules, which may be beyond the scope of some people's intended usage, or skills.

No, you can just extend the Resource class and you get a custom resource. I heavily make use of that in EgoVenture.

[Sslaxx]

This may be somewhat addressed by #5010

[Proggle]

One of Godot's big strengths is streamlining common gamedev tasks, so you don't have to spend time reinventing the wheel. Saving and loading the game is one of the most common functions that devs need to implement (up there with having collision detection), but currently it isn't as straightforward as it could be.

And of course, from the game developer perspective, as far as the player's experience goes, the only real question that needs to be answered is "What data goes into the savefile?". Whatever method the developer uses to cram the data into a save file, and then pull it back out unchanged, will function equally well.

So, since this is a common feature where the exact algorithm doesn't matter to the end product, the developer should be able to just trust a built-in Godot feature to do it properly, and not worry about what's going on under the hood.

As is, resources are almost the perfect way for Godot to handle saving and restoring states. It uses Godot's unique strengths to handle savegames easily, and doesn't require futzing around with special cases to handle unsupported datatypes (like Vector2s in json).

It's unfortunate that this method is currently unsafe, but if inputs can be sanitized, it's a perfect way to never need to think about the underlying savegame/loadgame algorithms ever again.

[TheDuriel]

Or just, save the resource using File, not the ResourceSaver, and encrypt it.

Or, since your savegame boils down to just being a dictionary anyways. (And lets be honest, custom Resources are just fancy typed dictionaries.) Just use File.store_var()

Resources as savegames only blipped up recently as a consideration, since I think people have finally caught on that json text files are a poor idea. But the next step is raw binary data dumps. Saving and loading a resource is actually more (or at least identical) work than making a dictionary and saving/loading that.

But that isn't even the topic of this issue. The topic is the glaring security problem caused by Godot automatically loading resources that could contain malicious code. Which is only a problem in two cases: A developer using resources where normal files would be sufficient. Or a malicious agent placing resources where it will override the pck contents to inject code. (Which requires the developer to have omitted the res.// in the file path.)

[dalexeev]

I agree that File.store_var/get_var in combination with a dictionary is already sufficient for a convenient implementation of save files. Although the analog of allow_objects should be added to the VariantParser, so that text serialization (for example, ConfigFile) also becomes safe.

tscn/tres are not suitable for save files, not only because of security, but also because of compatibility. This format is simply not intended for such purposes, as it contains references to external resources and contains nested resources, that is, it makes the save file dependent on their implementation. Attempting to load a save file based on resources from an old incompatible version of the game will result in an error loading the resource, while json, ConfigFile or a serialized dictionary can be loaded without error and then converted to the new version.

[alekh-ai]

Is there any way to use unique tokens for custom resources. That way it will be safer to use.

[Calinou]

Is there any way to use unique tokens for custom resources. That way it will be safer to use.

Are you referring to encryption or signing of custom resources? That's a topic for a different proposal. Godot already offers way to save/load encrypted files, but I doubt there's a built-in signing mechanism you could make use of.

[lyuma]

Hi, sorry I never replied with the implementation we use in V-Sekai since last year. @fire pinged me to reply to this issue. If you're interested you can find the solution we use here:
https://github.com/V-Sekai/godot/tree/resource_loader_whitelist

This feature was rejected from core due to security considerations, but I think another discussion is needed. I'll explain.

I suspect this patch is sufficient for the types of resources that a game, for example, might use to store a save game or a level, usually a custom extends Resource class being written to disk.

While we (the V-Sekai team) suspect the above patch solves the pertinent problem to resources in particular, it does not make all types of resources entirely secure. It is critical that scenes in particular be further validated in terms of node paths or animations, and possibly other cases.

Also, once the resource is loaded, most of the other security risks can be handled in script through a combination of recursion and property validation. [Here is an example of one such possible implementation.])https://github.com/V-Sekai/v-sekai-game/blob/main/addons/vsk_importer_exporter/vsk_map_validator.gd) Note that this type of validation logic is not secure in general, and must be adapted to your game.

That said, because of its applicability to extends Resource type resources commonly used for saved games and other user-sharable files, I do think the mitigation in our implementation would go a long way and I'd love to see it in core, even if imperfect. (As a user, I will lament the loss of widespread ability to mod Godot games through save game editing, but it's for the best.)

[reptofrog]

It would've been great to have an ability to granularly restrict what loaded scripts can and what they can't do, something that was proposed in here: #4642 in conjunction with #5010

This would enable the creation of games that can be modded safely without resorting to any extensions or custom solutions.

[derkork]

I have created a small library that should solve this problem for saved games. It basically works similar to the whitelist @lyuma posted, but it will flat-out refuse to load any resource file that contains resources of type GDScript. This will probably not be good enough for user generated content where you may want to allow some sandboxed scripting, but it should work decently for saved games which usually will not contain any inline scripts, so it's a nice 80/20 solution I think.

[TheDuriel]

A resource is entirely capable of pointing towards a script definition outside of itself, or even outside of the project.

[derkork]

Yes it is. But for this to work, the player would have needed to download and place this script in addition to the save game file on their computer, so I wouldn't think this is a practical attack. That being said, if you have comments regarding the library it may be better to open up an issue there instead of using this discussion thread.

[lyuma]

I'm sorry I forgot to link this here.
Last month I wrote a GDScript resource parser which can be used for validation before loading the resource, without any need for engine support:
https://gist.github.com/lyuma/8de4620a402d565b86e1287150c8fb31

It's a bit scary in that it's security critical code, but on the plus side, in this form it would be fairly easy to audit and ensure it functions identically to the resource loader.

The risk of this approach is if Godot's resource parser ever diverges from this code (one if statement is different, for example), then that difference in code path could be used to create an exploit. My expectation is Godot would increase FORMAT_VERSION if it changes the format, but if its parsing code is not careful and they add the new features to the old format, it would create an exploit.

I think this approach would be safe if it explicitly checks for a specific Godot major and minor version (e.g. 4.2) and human review is done on each version to ensure it continues to match parsing logic. Personally, I'm willing to take this approach because I think it is less error prone than the failure case of accidentally using an unpatched engine, or having validation built into Godot and having security critical code decay by mistake during an unrelated change.

Let me know what you think about this design.

[theraot]

Since nobody seems to have mentioned it, I'll point out that if loading resources can skip scripts it would also address godotengine/godot#65393