From 8c99f2232bdb52459ccf2a5b751cbe3f7797abc3 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 17 Dec 2021 18:31:29 +0100 Subject: Don't let HTTP API pass through untrusted function This has been a problem since the first day, oops. --- src/script/lua_api/l_http.cpp | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) (limited to 'src/script/lua_api/l_http.cpp') diff --git a/src/script/lua_api/l_http.cpp b/src/script/lua_api/l_http.cpp index 751ec9837..b385b698c 100644 --- a/src/script/lua_api/l_http.cpp +++ b/src/script/lua_api/l_http.cpp @@ -163,6 +163,20 @@ int ModApiHttp::l_http_fetch_async_get(lua_State *L) return 1; } +int ModApiHttp::l_set_http_api_lua(lua_State *L) +{ + NO_MAP_LOCK_REQUIRED; + + // This is called by builtin to give us a function that will later + // populate the http_api table with additional method(s). + // We need this because access to the HTTP api is security-relevant and + // any mod could just mess with a global variable. + luaL_checktype(L, 1, LUA_TFUNCTION); + lua_rawseti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_HTTP_API_LUA); + + return 0; +} + int ModApiHttp::l_request_http_api(lua_State *L) { NO_MAP_LOCK_REQUIRED; @@ -205,16 +219,16 @@ int ModApiHttp::l_request_http_api(lua_State *L) return 1; } - lua_getglobal(L, "core"); - lua_getfield(L, -1, "http_add_fetch"); + lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_HTTP_API_LUA); + assert(lua_isfunction(L, -1)); lua_newtable(L); HTTP_API(fetch_async); HTTP_API(fetch_async_get); // Stack now looks like this: - // - // Now call core.http_add_fetch to append .fetch(request, callback) to table + //
+ // Now call it to append .fetch(request, callback) to table lua_call(L, 1, 1); return 1; @@ -247,6 +261,7 @@ void ModApiHttp::Initialize(lua_State *L, int top) API_FCT(get_http_api); } else { API_FCT(request_http_api); + API_FCT(set_http_api_lua); } #endif -- 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/lua_api/l_http.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 afb061c374ed6797f47b0806aba26845713d15ac Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 4 Feb 2022 20:29:28 +0100 Subject: Fix broken server startup if curl is disabled (#12046) --- src/script/lua_api/l_http.cpp | 35 +++++++++++++++++++++-------------- src/script/lua_api/l_http.h | 7 ++++--- 2 files changed, 25 insertions(+), 17 deletions(-) (limited to 'src/script/lua_api/l_http.cpp') diff --git a/src/script/lua_api/l_http.cpp b/src/script/lua_api/l_http.cpp index bd359b3cc..5566a8523 100644 --- a/src/script/lua_api/l_http.cpp +++ b/src/script/lua_api/l_http.cpp @@ -162,20 +162,6 @@ int ModApiHttp::l_http_fetch_async_get(lua_State *L) return 1; } -int ModApiHttp::l_set_http_api_lua(lua_State *L) -{ - NO_MAP_LOCK_REQUIRED; - - // This is called by builtin to give us a function that will later - // populate the http_api table with additional method(s). - // We need this because access to the HTTP api is security-relevant and - // any mod could just mess with a global variable. - luaL_checktype(L, 1, LUA_TFUNCTION); - lua_rawseti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_HTTP_API_LUA); - - return 0; -} - int ModApiHttp::l_request_http_api(lua_State *L) { NO_MAP_LOCK_REQUIRED; @@ -215,6 +201,22 @@ int ModApiHttp::l_get_http_api(lua_State *L) #endif +int ModApiHttp::l_set_http_api_lua(lua_State *L) +{ + NO_MAP_LOCK_REQUIRED; + +#if USE_CURL + // This is called by builtin to give us a function that will later + // populate the http_api table with additional method(s). + // We need this because access to the HTTP api is security-relevant and + // any mod could just mess with a global variable. + luaL_checktype(L, 1, LUA_TFUNCTION); + lua_rawseti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_HTTP_API_LUA); +#endif + + return 0; +} + void ModApiHttp::Initialize(lua_State *L, int top) { #if USE_CURL @@ -231,6 +233,11 @@ void ModApiHttp::Initialize(lua_State *L, int top) API_FCT(set_http_api_lua); } +#else + + // Define this function anyway so builtin can call it without checking + API_FCT(set_http_api_lua); + #endif } diff --git a/src/script/lua_api/l_http.h b/src/script/lua_api/l_http.h index 17fa283ba..8d084ecd9 100644 --- a/src/script/lua_api/l_http.h +++ b/src/script/lua_api/l_http.h @@ -41,9 +41,6 @@ private: // http_fetch_async_get(handle) static int l_http_fetch_async_get(lua_State *L); - // set_http_api_lua() [internal] - static int l_set_http_api_lua(lua_State *L); - // request_http_api() static int l_request_http_api(lua_State *L); @@ -51,6 +48,10 @@ private: static int l_get_http_api(lua_State *L); #endif + // set_http_api_lua() [internal] + static int l_set_http_api_lua(lua_State *L); + + public: static void Initialize(lua_State *L, int top); static void InitializeAsync(lua_State *L, int top); -- cgit v1.2.3