From b9051386ae296a6112383725bc8bfcd96dc9a226 Mon Sep 17 00:00:00 2001 From: Lejo Date: Fri, 26 Nov 2021 19:31:05 +0100 Subject: Add Lua bitop library (#9847) --- src/script/cpp_api/s_security.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/script/cpp_api/s_security.cpp') diff --git a/src/script/cpp_api/s_security.cpp b/src/script/cpp_api/s_security.cpp index 580042ec2..5faf8cc80 100644 --- a/src/script/cpp_api/s_security.cpp +++ b/src/script/cpp_api/s_security.cpp @@ -106,6 +106,7 @@ void ScriptApiSecurity::initializeSecurity() "string", "table", "math", + "bit" }; static const char *io_whitelist[] = { "close", @@ -298,6 +299,7 @@ void ScriptApiSecurity::initializeSecurityClient() "string", "table", "math", + "bit", }; static const char *os_whitelist[] = { "clock", @@ -834,4 +836,3 @@ int ScriptApiSecurity::sl_os_remove(lua_State *L) lua_call(L, 1, 2); return 2; } - -- cgit v1.2.3 From f4054595482bf4573075f45d3ca56076a0d6113e Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 17 Dec 2021 18:35:30 +0100 Subject: Remove setlocal and setupvalue from `debug` table whitelist It's likely that these could be used trick mods into revealing the insecure environment even if they do everything right (which is already hard enough). --- src/script/cpp_api/s_security.cpp | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/script/cpp_api/s_security.cpp') diff --git a/src/script/cpp_api/s_security.cpp b/src/script/cpp_api/s_security.cpp index 5faf8cc80..11c277839 100644 --- a/src/script/cpp_api/s_security.cpp +++ b/src/script/cpp_api/s_security.cpp @@ -129,12 +129,10 @@ void ScriptApiSecurity::initializeSecurity() "traceback", "getinfo", "getmetatable", - "setupvalue", "setmetatable", "upvalueid", "sethook", "debug", - "setlocal", }; static const char *package_whitelist[] = { "config", -- cgit v1.2.3 From b2409b14d0682655363c1b3b3b6bafbaa7e7c1bf Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 17 Dec 2021 19:04:46 +0100 Subject: Refactor trusted mod checking code --- src/script/cpp_api/s_security.cpp | 33 +++++++++++++++++++++++++++++++++ src/script/cpp_api/s_security.h | 14 +++++++++----- src/script/lua_api/l_http.cpp | 39 +++------------------------------------ src/script/lua_api/l_util.cpp | 32 +------------------------------- 4 files changed, 46 insertions(+), 72 deletions(-) (limited to 'src/script/cpp_api/s_security.cpp') diff --git a/src/script/cpp_api/s_security.cpp b/src/script/cpp_api/s_security.cpp index 11c277839..ccd1214e3 100644 --- a/src/script/cpp_api/s_security.cpp +++ b/src/script/cpp_api/s_security.cpp @@ -27,6 +27,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include +#include #include @@ -604,6 +605,38 @@ bool ScriptApiSecurity::checkPath(lua_State *L, const char *path, return false; } +bool ScriptApiSecurity::checkWhitelisted(lua_State *L, const std::string &setting) +{ + assert(str_starts_with(setting, "secure.")); + + // We have to make sure that this function is being called directly by + // a mod, otherwise a malicious mod could override this function and + // steal its return value. + lua_Debug info; + + // Make sure there's only one item below this function on the stack... + if (lua_getstack(L, 2, &info)) + return false; + FATAL_ERROR_IF(!lua_getstack(L, 1, &info), "lua_getstack() failed"); + FATAL_ERROR_IF(!lua_getinfo(L, "S", &info), "lua_getinfo() failed"); + + // ...and that that item is the main file scope. + if (strcmp(info.what, "main") != 0) + return false; + + // Mod must be listed in secure.http_mods or secure.trusted_mods + lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_CURRENT_MOD_NAME); + if (!lua_isstring(L, -1)) + return false; + std::string mod_name = readParam(L, -1); + + std::string value = g_settings->get(setting); + value.erase(std::remove(value.begin(), value.end(), ' '), value.end()); + auto mod_list = str_split(value, ','); + + return CONTAINS(mod_list, mod_name); +} + int ScriptApiSecurity::sl_g_dofile(lua_State *L) { diff --git a/src/script/cpp_api/s_security.h b/src/script/cpp_api/s_security.h index 73e763548..619bf824f 100644 --- a/src/script/cpp_api/s_security.h +++ b/src/script/cpp_api/s_security.h @@ -40,11 +40,6 @@ with this program; if not, write to the Free Software Foundation, Inc., class ScriptApiSecurity : virtual public ScriptApiBase { public: - int getThread(lua_State *L); - // creates an empty Lua environment - void createEmptyEnv(lua_State *L); - // sets the enviroment to the table thats on top of the stack - void setLuaEnv(lua_State *L, int thread); // Sets up security on the ScriptApi's Lua state void initializeSecurity(); void initializeSecurityClient(); @@ -57,8 +52,17 @@ public: // Checks if mods are allowed to read (and optionally write) to the path static bool checkPath(lua_State *L, const char *path, bool write_required, bool *write_allowed=NULL); + // Check if mod is whitelisted in the given setting + // This additionally checks that the mod's main file scope is executing. + static bool checkWhitelisted(lua_State *L, const std::string &setting); private: + int getThread(lua_State *L); + // sets the enviroment to the table thats on top of the stack + void setLuaEnv(lua_State *L, int thread); + // creates an empty Lua environment + void createEmptyEnv(lua_State *L); + // Syntax: "sl_" '_' // (sl stands for Secure Lua) diff --git a/src/script/lua_api/l_http.cpp b/src/script/lua_api/l_http.cpp index b385b698c..bd359b3cc 100644 --- a/src/script/lua_api/l_http.cpp +++ b/src/script/lua_api/l_http.cpp @@ -21,14 +21,13 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "common/c_converter.h" #include "common/c_content.h" #include "lua_api/l_http.h" +#include "cpp_api/s_security.h" #include "httpfetch.h" #include "settings.h" #include "debug.h" #include "log.h" -#include #include -#include #define HTTP_API(name) \ lua_pushstring(L, #name); \ @@ -181,40 +180,8 @@ int ModApiHttp::l_request_http_api(lua_State *L) { NO_MAP_LOCK_REQUIRED; - // We have to make sure that this function is being called directly by - // a mod, otherwise a malicious mod could override this function and - // steal its return value. - lua_Debug info; - - // Make sure there's only one item below this function on the stack... - if (lua_getstack(L, 2, &info)) { - return 0; - } - FATAL_ERROR_IF(!lua_getstack(L, 1, &info), "lua_getstack() failed"); - FATAL_ERROR_IF(!lua_getinfo(L, "S", &info), "lua_getinfo() failed"); - - // ...and that that item is the main file scope. - if (strcmp(info.what, "main") != 0) { - return 0; - } - - // Mod must be listed in secure.http_mods or secure.trusted_mods - lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_CURRENT_MOD_NAME); - if (!lua_isstring(L, -1)) { - return 0; - } - - std::string mod_name = readParam(L, -1); - std::string http_mods = g_settings->get("secure.http_mods"); - http_mods.erase(std::remove(http_mods.begin(), http_mods.end(), ' '), http_mods.end()); - std::vector mod_list_http = str_split(http_mods, ','); - - std::string trusted_mods = g_settings->get("secure.trusted_mods"); - trusted_mods.erase(std::remove(trusted_mods.begin(), trusted_mods.end(), ' '), trusted_mods.end()); - std::vector mod_list_trusted = str_split(trusted_mods, ','); - - mod_list_http.insert(mod_list_http.end(), mod_list_trusted.begin(), mod_list_trusted.end()); - if (std::find(mod_list_http.begin(), mod_list_http.end(), mod_name) == mod_list_http.end()) { + if (!ScriptApiSecurity::checkWhitelisted(L, "secure.http_mods") && + !ScriptApiSecurity::checkWhitelisted(L, "secure.trusted_mods")) { lua_pushnil(L); return 1; } diff --git a/src/script/lua_api/l_util.cpp b/src/script/lua_api/l_util.cpp index 528d9c6dd..b04f26fda 100644 --- a/src/script/lua_api/l_util.cpp +++ b/src/script/lua_api/l_util.cpp @@ -41,7 +41,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "util/hex.h" #include "util/sha1.h" #include "util/png.h" -#include #include // log([level,] text) @@ -444,36 +443,7 @@ int ModApiUtil::l_request_insecure_environment(lua_State *L) return 1; } - // We have to make sure that this function is being called directly by - // a mod, otherwise a malicious mod could override this function and - // steal its return value. - lua_Debug info; - // Make sure there's only one item below this function on the stack... - if (lua_getstack(L, 2, &info)) { - return 0; - } - FATAL_ERROR_IF(!lua_getstack(L, 1, &info), "lua_getstack() failed"); - FATAL_ERROR_IF(!lua_getinfo(L, "S", &info), "lua_getinfo() failed"); - // ...and that that item is the main file scope. - if (strcmp(info.what, "main") != 0) { - return 0; - } - - // Get mod name - lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_CURRENT_MOD_NAME); - if (!lua_isstring(L, -1)) { - return 0; - } - - // Check secure.trusted_mods - std::string mod_name = readParam(L, -1); - std::string trusted_mods = g_settings->get("secure.trusted_mods"); - trusted_mods.erase(std::remove_if(trusted_mods.begin(), - trusted_mods.end(), static_cast(&std::isspace)), - trusted_mods.end()); - std::vector mod_list = str_split(trusted_mods, ','); - if (std::find(mod_list.begin(), mod_list.end(), mod_name) == - mod_list.end()) { + if (!ScriptApiSecurity::checkWhitelisted(L, "secure.trusted_mods")) { return 0; } -- cgit v1.2.3 From 9a12e4499ecf5c1a3467af9c831d0d350a21923d Mon Sep 17 00:00:00 2001 From: sfan5 Date: Thu, 13 Jan 2022 22:12:44 +0100 Subject: Minor improvements to Lua sandbox --- src/script/cpp_api/s_security.cpp | 30 ++++++++++++++++++++++++++---- src/script/cpp_api/s_security.h | 1 + 2 files changed, 27 insertions(+), 4 deletions(-) (limited to 'src/script/cpp_api/s_security.cpp') diff --git a/src/script/cpp_api/s_security.cpp b/src/script/cpp_api/s_security.cpp index ccd1214e3..a6c5114b2 100644 --- a/src/script/cpp_api/s_security.cpp +++ b/src/script/cpp_api/s_security.cpp @@ -121,9 +121,7 @@ void ScriptApiSecurity::initializeSecurity() "date", "difftime", "getenv", - "setlocale", "time", - "tmpname", }; static const char *debug_whitelist[] = { "gethook", @@ -219,6 +217,7 @@ void ScriptApiSecurity::initializeSecurity() // And replace unsafe ones SECURE_API(os, remove); SECURE_API(os, rename); + SECURE_API(os, setlocale); lua_setglobal(L, "os"); lua_pop(L, 1); // Pop old OS @@ -250,6 +249,11 @@ void ScriptApiSecurity::initializeSecurity() lua_pop(L, 1); // Pop old jit #endif + // Get rid of 'core' in the old globals, we don't want anyone thinking it's + // safe or even usable. + lua_pushnil(L); + lua_setfield(L, old_globals, "core"); + lua_pop(L, 1); // Pop globals_backup @@ -285,7 +289,7 @@ void ScriptApiSecurity::initializeSecurityClient() "rawset", "select", "setfenv", - // getmetatable can be used to escape the sandbox + // getmetatable can be used to escape the sandbox <- ??? "setmetatable", "tonumber", "tostring", @@ -307,7 +311,7 @@ void ScriptApiSecurity::initializeSecurityClient() "time" }; static const char *debug_whitelist[] = { - "getinfo", + "getinfo", // used by builtin and unset before mods load "traceback" }; @@ -867,3 +871,21 @@ int ScriptApiSecurity::sl_os_remove(lua_State *L) lua_call(L, 1, 2); return 2; } + + +int ScriptApiSecurity::sl_os_setlocale(lua_State *L) +{ + const bool cat = lua_gettop(L) > 1; + // Don't allow changes + if (!lua_isnoneornil(L, 1)) { + lua_pushnil(L); + return 1; + } + + push_original(L, "os", "setlocale"); + lua_pushnil(L); + if (cat) + lua_pushvalue(L, 2); + lua_call(L, cat ? 2 : 1, 1); + return 1; +} diff --git a/src/script/cpp_api/s_security.h b/src/script/cpp_api/s_security.h index 619bf824f..880ce1638 100644 --- a/src/script/cpp_api/s_security.h +++ b/src/script/cpp_api/s_security.h @@ -79,4 +79,5 @@ private: static int sl_os_rename(lua_State *L); static int sl_os_remove(lua_State *L); + static int sl_os_setlocale(lua_State *L); }; -- cgit v1.2.3 From 06d197cdd042392e1551e5e7244c61300a6bb4e3 Mon Sep 17 00:00:00 2001 From: Jude Melton-Houghton Date: Tue, 29 Mar 2022 12:07:00 -0400 Subject: Store vector metatable in registry --- builtin/common/tests/misc_helpers_spec.lua | 1 + builtin/common/tests/serialize_spec.lua | 1 + builtin/common/tests/vector_spec.lua | 2 +- builtin/common/vector.lua | 6 ++---- builtin/mainmenu/tests/serverlistmgr_spec.lua | 1 + games/devtest/mods/unittests/misc.lua | 12 ++++++++++++ src/script/common/c_converter.cpp | 19 +++---------------- src/script/common/c_internal.h | 1 + src/script/cpp_api/s_base.cpp | 8 ++++++++ src/script/cpp_api/s_security.cpp | 6 ++++++ 10 files changed, 36 insertions(+), 21 deletions(-) (limited to 'src/script/cpp_api/s_security.cpp') diff --git a/builtin/common/tests/misc_helpers_spec.lua b/builtin/common/tests/misc_helpers_spec.lua index b16987f0b..b11236860 100644 --- a/builtin/common/tests/misc_helpers_spec.lua +++ b/builtin/common/tests/misc_helpers_spec.lua @@ -1,4 +1,5 @@ _G.core = {} +_G.vector = {metatable = {}} dofile("builtin/common/vector.lua") dofile("builtin/common/misc_helpers.lua") diff --git a/builtin/common/tests/serialize_spec.lua b/builtin/common/tests/serialize_spec.lua index e46b7dcc5..69b2b567c 100644 --- a/builtin/common/tests/serialize_spec.lua +++ b/builtin/common/tests/serialize_spec.lua @@ -1,4 +1,5 @@ _G.core = {} +_G.vector = {metatable = {}} _G.setfenv = require 'busted.compatibility'.setfenv diff --git a/builtin/common/tests/vector_spec.lua b/builtin/common/tests/vector_spec.lua index 2f72f3383..25880236b 100644 --- a/builtin/common/tests/vector_spec.lua +++ b/builtin/common/tests/vector_spec.lua @@ -1,4 +1,4 @@ -_G.vector = {} +_G.vector = {metatable = {}} dofile("builtin/common/vector.lua") describe("vector", function() diff --git a/builtin/common/vector.lua b/builtin/common/vector.lua index 581d014e0..90010f6de 100644 --- a/builtin/common/vector.lua +++ b/builtin/common/vector.lua @@ -6,10 +6,8 @@ Note: The vector.*-functions must be able to accept old vectors that had no meta -- localize functions local setmetatable = setmetatable -vector = {} - -local metatable = {} -vector.metatable = metatable +-- vector.metatable is set by C++. +local metatable = vector.metatable local xyz = {"x", "y", "z"} diff --git a/builtin/mainmenu/tests/serverlistmgr_spec.lua b/builtin/mainmenu/tests/serverlistmgr_spec.lua index a091959fb..ab7a6c60c 100644 --- a/builtin/mainmenu/tests/serverlistmgr_spec.lua +++ b/builtin/mainmenu/tests/serverlistmgr_spec.lua @@ -1,4 +1,5 @@ _G.core = {} +_G.vector = {metatable = {}} _G.unpack = table.unpack _G.serverlistmgr = {} diff --git a/games/devtest/mods/unittests/misc.lua b/games/devtest/mods/unittests/misc.lua index cf4f92cfa..ba980866a 100644 --- a/games/devtest/mods/unittests/misc.lua +++ b/games/devtest/mods/unittests/misc.lua @@ -36,3 +36,15 @@ local function test_dynamic_media(cb, player) -- if the callback isn't called this test will just hang :shrug: end unittests.register("test_dynamic_media", test_dynamic_media, {async=true, player=true}) + +local function test_v3f_metatable(player) + assert(vector.check(player:get_pos())) +end +unittests.register("test_v3f_metatable", test_v3f_metatable, {player=true}) + +local function test_v3s16_metatable(player, pos) + local node = minetest.get_node(pos) + local found_pos = minetest.find_node_near(pos, 0, node.name, true) + assert(vector.check(found_pos)) +end +unittests.register("test_v3s16_metatable", test_v3s16_metatable, {map=true}) diff --git a/src/script/common/c_converter.cpp b/src/script/common/c_converter.cpp index 08fb9ad30..b5ff52f73 100644 --- a/src/script/common/c_converter.cpp +++ b/src/script/common/c_converter.cpp @@ -52,25 +52,12 @@ if (value < F1000_MIN || value > F1000_MAX) { \ /** - * A helper which sets (if available) the vector metatable from builtin as metatable - * for the table on top of the stack + * A helper which sets the vector metatable for the table on top of the stack */ static void set_vector_metatable(lua_State *L) { - // get vector.metatable - lua_getglobal(L, "vector"); - if (!lua_istable(L, -1)) { - // there is no global vector table - lua_pop(L, 1); - errorstream << "set_vector_metatable in c_converter.cpp: " << - "missing global vector table" << std::endl; - return; - } - lua_getfield(L, -1, "metatable"); - // set the metatable - lua_setmetatable(L, -3); - // pop vector global - lua_pop(L, 1); + lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_VECTOR_METATABLE); + lua_setmetatable(L, -2); } void push_v3f(lua_State *L, v3f p) diff --git a/src/script/common/c_internal.h b/src/script/common/c_internal.h index 94cfd61fb..c43db34aa 100644 --- a/src/script/common/c_internal.h +++ b/src/script/common/c_internal.h @@ -55,6 +55,7 @@ extern "C" { #define CUSTOM_RIDX_CURRENT_MOD_NAME (CUSTOM_RIDX_BASE + 2) #define CUSTOM_RIDX_BACKTRACE (CUSTOM_RIDX_BASE + 3) #define CUSTOM_RIDX_HTTP_API_LUA (CUSTOM_RIDX_BASE + 4) +#define CUSTOM_RIDX_VECTOR_METATABLE (CUSTOM_RIDX_BASE + 5) // Determine if CUSTOM_RIDX_SCRIPTAPI will hold a light or full userdata diff --git a/src/script/cpp_api/s_base.cpp b/src/script/cpp_api/s_base.cpp index f7b8a5102..595c9e540 100644 --- a/src/script/cpp_api/s_base.cpp +++ b/src/script/cpp_api/s_base.cpp @@ -121,6 +121,14 @@ ScriptApiBase::ScriptApiBase(ScriptingType type): lua_newtable(m_luastack); lua_setglobal(m_luastack, "core"); + // vector.metatable is stored in the registry for quick access from C++. + lua_newtable(m_luastack); + lua_rawseti(m_luastack, LUA_REGISTRYINDEX, CUSTOM_RIDX_VECTOR_METATABLE); + lua_newtable(m_luastack); + lua_rawgeti(m_luastack, LUA_REGISTRYINDEX, CUSTOM_RIDX_VECTOR_METATABLE); + lua_setfield(m_luastack, -2, "metatable"); + lua_setglobal(m_luastack, "vector"); + if (m_type == ScriptingType::Client) lua_pushstring(m_luastack, "/"); else diff --git a/src/script/cpp_api/s_security.cpp b/src/script/cpp_api/s_security.cpp index a6c5114b2..f68cd1777 100644 --- a/src/script/cpp_api/s_security.cpp +++ b/src/script/cpp_api/s_security.cpp @@ -98,6 +98,7 @@ void ScriptApiSecurity::initializeSecurity() "type", "unpack", "_VERSION", + "vector", "xpcall", }; static const char *whitelist_tables[] = { @@ -254,6 +255,10 @@ void ScriptApiSecurity::initializeSecurity() lua_pushnil(L); lua_setfield(L, old_globals, "core"); + // 'vector' as well. + lua_pushnil(L); + lua_setfield(L, old_globals, "vector"); + lua_pop(L, 1); // Pop globals_backup @@ -296,6 +301,7 @@ void ScriptApiSecurity::initializeSecurityClient() "type", "unpack", "_VERSION", + "vector", "xpcall", // Completely safe libraries "coroutine", -- cgit v1.2.3