Skip to content

Lua sandboxing #5389

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.

Sign up for GitHub

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

Merged
Loobinex merged 1 commit into from
Oct 6, 2026
Merged

Lua sandboxing #5389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,8 +1254,8 @@ long get_rid(const struct NamedCommand *desc, const char *itmname)
}
if (strcasecmp("RANDOM", itmname) == 0)
{
i = (rand() % i);
return desc[i].num;
int32_t idx = (int32_t)LbRandomSeries((uint32_t)i, &game.action_random_seed, __func__, __LINE__);
return desc[idx].num;
}
return -1;
}
Expand Down
68 changes: 67 additions & 1 deletion src/lua_base.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,51 @@ static int lua_disabled_os_function(lua_State *L)
}


static void lua_blacklist(lua_State *L, const char *libname, const char *name)
{
lua_getglobal(L, libname);
if (!lua_istable(L, -1)) {
lua_pop(L, 1);
return;
}
lua_pushnil(L);
lua_setfield(L, -2, name);
lua_pop(L, 1);
}

static void lua_whitelist(lua_State *L, const char *libname, const char *keep)
{
lua_getglobal(L, libname);
if (!lua_istable(L, -1)) {
lua_pop(L, 1);
return;
}
int t = lua_gettop(L);
lua_pushnil(L);
while (lua_next(L, t) != 0) {
if (lua_type(L, -2) == LUA_TSTRING) {
const char *key = lua_tostring(L, -2);
if (strcmp(key, keep) != 0) {
lua_pushnil(L);
lua_setfield(L, t, key);
}
}
lua_pop(L, 1);
}
lua_pop(L, 1);
}

// Remove Lua functions which can break the sandbox/determinism
static void disable_lua_functions(lua_State *L)
{
// TODO - remove/replace these
lua_whitelist(L, "io", "open");

@cerwym cerwym Oct 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please go the full distance on this, I don't want it possible for any lua script to operate with os, io or debug at all

lua_getglobal(L, "package");
lua_getfield(L, -1, "loaders");
if (lua_istable(L, -1)) {
	lua_pushnil(L);
	lua_rawseti(L, -2, 3);
	lua_pushnil(L);
	lua_rawseti(L, -2, 4);
}
lua_pop(L, 2);

Would disable the package loaders too that would allow bypass of require() which now you've highlighted this, im terrified of

@nstbayless nstbayless Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. The problem is that existing Lua maps use these for saving and loading. So we need a new API to replace that. What would you suggest?

@cerwym cerwym Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, and I think what we need to do, in the vein of security is give a specialized API function that can only write to a sane location in our control;

see PlatformWindows::GetUserPrefDir() for an example of PlatformIO.

What I envision is an IFileSystem interface that also has C bindings for API methods in Lua to use.

Basically I don't want any path traversal or any ability to break out of the sandbox. This would be net-new and not use any of the Lb* functions.

I saw this PR and got to to thinking, in 5 minutes I was able to put up a POC and invite Loob into a game where I performed an RCE on his machine.

It's safe to run maps downloaded from strangers. We could even sync maps over netplay.

In order to fulfill that dream, we'd have to really think about this deeply, you're on the right track with this

@PieterVdc PieterVdc Oct 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug specifically I do rely on to check if a function is properly serializable, don't see why that one would be an issue?

@nstbayless nstbayless Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we have to think deeply, but I'm also not intending that this PR will solve 100% of vulnerabilities. My goal with this PR is to remove all vulnerabilities which are not presently relied on for any current lua maps.

@nstbayless nstbayless Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IFileSystem interface

I can see this being useful for loading datafiles perhaps. But why give write access at all? If we want to let the script store persistent data, IMO we should have a special section in the save files for it or something like that.

Buuut I don't want to touch save file data right now ;)

@cerwym cerwym Oct 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why give write access at all?

So you can load sounds and things, lua scripts can legitimately play audio files

@nstbayless nstbayless Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's read access, no?

@cerwym cerwym Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still inherently bad. Realistically, our allowance of IO should be mapped to a VFS, as it stands, a lua script could read any file on the system.

The TLDR solution is to wrap any lua IO code with something akin to this;

static int safe_io_open(lua_State *L) {
    const char *path = luaL_checkstring(L, 1);
    const char *mode = luaL_optstring(L, 2, "r");

    // Prevent path traversal attempts
    if (strstr(path, "..") != NULL || path[0] == '/') {
        return luaL_error(L, "Access denied: Invalid file path.");
    }

    // Force all file paths into a restricted directory
    char safe_path[512];
    snprintf(safe_path, sizeof(safe_path), "sandbox_dir/%s", path);

    // Call underlying fopen
    FILE *f = fopen(safe_path, mode);
    if (!f) return luaL_fileresult(L, 0, safe_path);

    // Push standard Lua file handle object
    luaL_Stream *p = (luaL_Stream *)lua_newuserdata(L, sizeof(luaL_Stream));
    p->f = f;
    p->closef = &io_fclose; // standard lua close function
    luaL_setmetatable(L, LUA_FILEHANDLE);
    return 1;
}

If io.open was to be allowed you'd rely on OS process isolation, but for KeeperFX that solution would be unbelievably silly, overkill and hurt performance


// TODO - remove/replace these
lua_whitelist(L, "os", "remove");

lua_blacklist(L, "package", "loadlib");

if (game.game_kind != GKind_MultiGame) {
return;
}
Expand Down Expand Up @@ -293,11 +336,34 @@ static void open_lua_script_for_mod_all(lua_State* L, LevelNumber lvnum)
/* @comment
* The loading items of open_lua_script and open_lua_script_for_mod need to be consistent.
*/
static void open_lua_standard_libraries(lua_State *L)
{
static const luaL_Reg stdlib[] = {
{"", luaopen_base},
{LUA_TABLIBNAME, luaopen_table},
{LUA_IOLIBNAME, luaopen_io},
{LUA_OSLIBNAME, luaopen_os},
{LUA_STRLIBNAME, luaopen_string},
{LUA_MATHLIBNAME, luaopen_math},
{LUA_DBLIBNAME, luaopen_debug},
{LUA_LOADLIBNAME, luaopen_package},
{LUA_BITLIBNAME, luaopen_bit},
// {LUA_JITLIBNAME, luaopen_jit},
// {LUA_FFILIBNAME, luaopen_ffi},
{NULL, NULL}
};
for (const luaL_Reg *lib = stdlib; lib->func != NULL; lib++) {
lua_pushcfunction(L, lib->func);
lua_pushstring(L, lib->name);
lua_call(L, 1, 0);
}
}

TbBool open_lua_script(LevelNumber lvnum)
{
Lvl_script = luaL_newstate();

luaL_openlibs(Lvl_script);
open_lua_standard_libraries(Lvl_script);
disable_lua_functions(Lvl_script);

lua_set_random_seed(game.action_random_seed);
Expand Down
Loading