[3.x] Added globals disabled feature to GDScript class

[fbcosentino]

Some games and apps involve players typing their own code, which is then interpreted/executed as part of the process.

One example is games where the player is expected to write logic as part of the game mechanic (control robots, hack into fictional systems, etc). There is an entire game genre based on this (https://en.wikipedia.org/wiki/Programming_game).

Another example is games which can be customized by players (mods), including writing script code (e.g. enemy AI).

In the projects I have worked so far with this mechanic, I had to design a scripting language and write my own interpreter (I have seen others trying the same as well). Due to having to write the interpreter, the scripting was rudimentary (sequential) and my players complained they wanted usual programming features like branching, loops, functions, local variables, etc, and were frustrated by the lack of.

GDScript could be used to implement in-game programming with a very functional, complex, stable and user friendly language, and it comes with a compiler for free, zero dev effort. However, due to the access to the engine globals, this comes with severe issues. Players could:

• Break the game behaviour
• Call OS and damage something in their computer
• Call ResourceSaver and steal assets
• In multiplayer games (or with leaderboards), print to console server confidential credentials

And if players are meant to share scripts (e.g. mods), hell is unleashed with anything from "delete all hdd" pranks to trojan horse attacks.

Therefore, so far using GDScript to power in-game general purpose scripting is "a nice thing you can't have".

This PR adds a feature to the GDScript class allowing to disable globals, per script. All game scripts behave as usual by default, but an individual script can have a flag set so that specific script won't have access to any engine globals, including class names, singletons and constants, becoming detached from the game system. It still can access anything if access is explicitly given, via variables or method arguments.

The code below demonstrates how to use the new set_globals_disabled method.

Assuming a file res://user_script.gd (or a String assigned via GDScript.source_code), which is meant to be isolated:

var my_var

func my_func(arg):
    print("Hello %s!" % arg)

A node can safely run this script with globals disabled:

var new_script = load("res://user_script.gd")
new_script.set_globals_disabled(true)
if new_script.reload():
    # Script recompiled successfully
    var new_script_instance = new_script.new()
    # Any method can be called externally,
    # and variables can be externally assigned
    new_script_instance.my_var = 42
    new_script_instance.my_func("world")  # prints "Hello world!"
else:
    # Pauses with an error if running from the editor,
    # fails silently and continues running in standalone build
    print("Failed to compile script")

An example to give explicit access to something, assuming the res://test_script.gd script:

var input

func do_something():
    if input.is_action_pressed("ui_accept"):
        print("Hello world!")

The input local variable can be used to give the script access to the Input singleton:

var new_script = load("res://test_script.gd")
new_script.set_globals_disabled(true)
if new_script.reload():
    var new_script_instance = new_script.new()
    new_script_instance.set("input", Input) # Fails silently if user doesn't want to declare or use `var input`
    new_script_instance.do_something()

Function arguments also work with external objects. The following script would work even with globals disabled, if ai_search_player is called externally providing the KinematicBody arguments:

func ai_search_player(my_kinematic_body, player_kinematic_body):
    player_distance = (player_body.global_transform.origin - my_body.global_transform.origin).length()

For mod systems where a subset of the engine functionality should be exposed, access to an interface node can be given to the isolated script (like the input example), that node having a selection of methods an properties as required in the modding system. In other words, that node would work like a master singleton for the isolated script.

[dalexeev]

It seems to me that this is not enough to create a secure sandbox, and these restrictions can be easily circumvented. Do not give the false illusion of security to users.

[SekoiaTree]

What makes it insecure?

[fbcosentino]

It seems to me that this is not enough to create a secure sandbox, and these restrictions can be easily circumvented. Do not give the false illusion of security to users.

With the flag on, the following identifiers are skipped in the compiler:

• GDScriptLanguage::get_singleton()->get_global_map().has(identifier)
• ScriptServer::is_global_class(identifier)
• GDScriptLanguage::get_singleton()->get_named_globals_map().has(identifier)

what is not skipped:

• identifiers belonging to current codegen stack
• identifiers member of current class
• constants of current class
• 3 items above for ancestors classes of current class: GDScript, Script, Resource, Reference, Object

The types those ancestor classes could expose access are:

• Array
• bool
• Dictionary
• Error
• int
• PoolByteArray
• PoolStringArray
• RID
• String

Resource has a get_local_scene() method which returns Node but only if it has resource_local_to_scene and it was instantiated from a PackedScene which is not the case when creating scripts via GDScript.new() so that will return null.

From the above I don't see how a script could circumvent the limitation from inside, but if I missed something please let me know.

You do have a point on giving false sense of security. If a user gives access to a node in the scene tree (like the my_kinematic_body example I gave), that would be a hole and a user could do my_kinematic_body.get_node() to navigate or just a my_kinematic_body.get_tree() for some disaster. I wrote that example this way in here for simplicity, and a good design would involve exposing a carefully prepared script instance instead, gatekeeping the interface. I didn't spend too much time designing one, so the 5-minute example I could think of now is using an autoload and an intermediate interface script, along the lines of:

• Autoload: something like a ModSystem class having a variable player_body (containing the player's KinematicBody)

• Intermediate script interface.gd:

func player_current_position():
    return ModSystem.player_body.global_transform.origin

and an instance of interface.gd is what is given to the mod scripts, e.g. as a variable game (following the method as the input example), so user scripts could call game.player_current_position(). I'm using an autoload because if the intrmediate script had the player_body variable, users could just use game.player_body.get_tree(), and since the interface script calls a global class (because it can) to access the player, this cuts the access to the user script, which can't.

tl'dr: security flaws would be game design flaws, but adding a warning to the docs is very relevant.

[dalexeev]

From the above I don't see how a script could circumvent the limitation from inside, but if I missed something please let me know.

Sandbox Escape:

extends Node

var evil_user_script := """
func run():
    var script = get_script().duplicate()
    script.set_globals_disabled(false)
    script.source_code = "func _init(): OS.alert('Escaped!')"
    script.reload()
    var instance = script.new()
"""

func _ready() -> void:
    var script := GDScript.new()
    script.set_globals_disabled(true)
    script.source_code = evil_user_script
    var err := script.reload()
    if err:
        print('Error')
        return
    var instance = script.new()
    instance.run()

List of potentially dangerous places:

1. Object.get_script, Object.set_script
2. str2var, bytes2var, dict2inst
3. load, preload
4. instance_from_id
5. Object.set, Object.set_indexed, Object.call
6. Resource.duplicate, Resource.get_local_scene
7. GDScript.set_globals_disabled(false)

But even if we close all the holes, I'm not sure if this feature is a good idea, because if the user is not very, very, very careful, then they will not even notice how they provide a path to escape from the sandbox. Plus, this feature is not a true sandbox (but needs input from a cybersecurity expert, which I am not).

You can always create vulnerabilities. But this is TOO EASY way to create vulnerabilities. This is in contrast to bytes2var, Expression, and Object.call, which are safe by default and only cause problems if they are grossly misused.

In my opinion, we cannot provide this feature and hope that the user has the knowledge and care to use this feature correctly. Passing a Node, Resource, or other unsafe value might be too tempting for the user for convenience reasons. And at the same time, the user will have a false sense of security, which is no less dangerous.

I think it would be better to limit the possibility of insecure interfaces or add full-fledged sandboxes. But this is a much more difficult task than this PR.

By the way, creating your own scripting is not as difficult as it seems. It may not be Turing-complete or Turing-complete, but a limited language interpreted in GDScript. You can use an Expression as a base and separate the commands with line breaks. See Escoria for example.

[Zireael07]

By the way, creating your own scripting is not as difficult as it seems. It may not be Turing-complete or Turing-complete, but a limited language interpreted in GDScript. You can use an Expression as a base and separate the commands with line breaks. See Escoria for example.

Wow, thanks for that hint, it may be very helpful to other people wanting to implement limited scripting in Godot projects

[fbcosentino]

List of potentially dangerous places:

1. Object.get_script, Object.set_script
2. str2var, bytes2var, dict2inst
3. load, preload
4. instance_from_id
5. Object.set, Object.set_indexed, Object.call
6. Resource.duplicate, Resource.get_local_scene
7. GDScript.set_globals_disabled(false)

If it was a matter of closing the holes, it could be made more restricted e.g. whitelisting system functions (so forgetting about a certain method is harmless).

But if the issue is on the social/usability side, then I agree the current approach is not ready to be included.

Let's freeze this for now then - and we necromance it later when I (or someone else) come up with a solution for that (if the feature is still relevant by then)

[akien-mga]

Given the above discussion, I marked this as "draft" so that we can filter it out in the backlog of PRs which are ready to merge.