From ea4ae55e24d2c27524490cb8e0d3c34aa46e3da3 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Sun, 3 Nov 2024 16:53:01 +0100 Subject: [PATCH] Implement script sandboxing for main menu --- builtin/mainmenu/async_event.lua | 7 +- builtin/mainmenu/content/contentdb.lua | 36 +++---- builtin/mainmenu/init.lua | 1 + src/script/cpp_api/s_async.cpp | 25 ++++- src/script/cpp_api/s_async.h | 5 +- src/script/cpp_api/s_security.cpp | 1 + src/script/lua_api/l_mainmenu.cpp | 129 +++++++++---------------- src/script/lua_api/l_mainmenu.h | 8 -- src/script/scripting_mainmenu.cpp | 43 ++++++++- src/script/scripting_mainmenu.h | 17 +++- 10 files changed, 146 insertions(+), 126 deletions(-) diff --git a/builtin/mainmenu/async_event.lua b/builtin/mainmenu/async_event.lua index 04bfb78d6..2358ee70e 100644 --- a/builtin/mainmenu/async_event.lua +++ b/builtin/mainmenu/async_event.lua @@ -11,11 +11,6 @@ end core.async_event_handler = handle_job function core.handle_async(func, parameter, callback) - -- Serialize function - local serialized_func = string.dump(func) - - assert(serialized_func ~= nil) - -- Serialize parameters local serialized_param = core.serialize(parameter) @@ -23,7 +18,7 @@ function core.handle_async(func, parameter, callback) return false end - local jobid = core.do_async_callback(serialized_func, serialized_param) + local jobid = core.do_async_callback(func, serialized_param) core.async_jobs[jobid] = callback diff --git a/builtin/mainmenu/content/contentdb.lua b/builtin/mainmenu/content/contentdb.lua index c352a260b..963400a12 100644 --- a/builtin/mainmenu/content/contentdb.lua +++ b/builtin/mainmenu/content/contentdb.lua @@ -392,7 +392,7 @@ function contentdb.resolve_dependencies(package, game, callback) end -local function fetch_pkgs(params) +local function fetch_pkgs() local version = core.get_version() local base_url = core.settings:get("contentdb_url") local url = base_url .. @@ -429,41 +429,43 @@ local function fetch_pkgs(params) if not packages or #packages == 0 then return end - local aliases = {} + return packages +end + + +function contentdb.set_packages_from_api(packages) + contentdb.package_by_id = {} + contentdb.aliases = {} for _, package in pairs(packages) do - package.id = params.calculate_package_id(package.type, package.author, package.name) + package.id = contentdb.calculate_package_id(package.type, package.author, package.name) package.url_part = core.urlencode(package.author) .. "/" .. core.urlencode(package.name) + contentdb.package_by_id[package.id] = package + if package.aliases then for _, alias in ipairs(package.aliases) do -- We currently don't support name changing local suffix = "/" .. package.name if alias:sub(-#suffix) == suffix then - aliases[alias:lower()] = package.id + contentdb.aliases[alias:lower()] = package.id end end end end - return { packages = packages, aliases = aliases } + contentdb.load_ok = true + contentdb.load_error = false + contentdb.packages = packages + contentdb.packages_full = packages + contentdb.packages_full_unordered = packages end - function contentdb.fetch_pkgs(callback) contentdb.loading = true - core.handle_async(fetch_pkgs, { calculate_package_id = contentdb.calculate_package_id }, function(result) + core.handle_async(fetch_pkgs, nil, function(result) if result then - contentdb.load_ok = true - contentdb.load_error = false - contentdb.packages = result.packages - contentdb.packages_full = result.packages - contentdb.packages_full_unordered = result.packages - contentdb.aliases = result.aliases - - for _, package in ipairs(result.packages) do - contentdb.package_by_id[package.id] = package - end + contentdb.set_packages_from_api(result) else contentdb.load_error = true end diff --git a/builtin/mainmenu/init.lua b/builtin/mainmenu/init.lua index fc237e158..41519b2ee 100644 --- a/builtin/mainmenu/init.lua +++ b/builtin/mainmenu/init.lua @@ -133,4 +133,5 @@ local function init_globals() check_new_version() end +assert(os.execute == nil) init_globals() diff --git a/src/script/cpp_api/s_async.cpp b/src/script/cpp_api/s_async.cpp index f55082308..4cb46f6bb 100644 --- a/src/script/cpp_api/s_async.cpp +++ b/src/script/cpp_api/s_async.cpp @@ -14,10 +14,14 @@ extern "C" { #include "server.h" #include "s_async.h" #include "log.h" +#include "config.h" #include "filesys.h" #include "porting.h" #include "common/c_internal.h" #include "common/c_packer.h" +#if CHECK_CLIENT_BUILD() +#include "script/scripting_mainmenu.h" +#endif #include "lua_api/l_base.h" /******************************************************************************/ @@ -256,7 +260,6 @@ bool AsyncEngine::prepareEnvironment(lua_State* L, int top) return true; } -/******************************************************************************/ AsyncWorkerThread::AsyncWorkerThread(AsyncEngine* jobDispatcher, const std::string &name) : ScriptApiBase(ScriptingType::Async), @@ -270,6 +273,8 @@ AsyncWorkerThread::AsyncWorkerThread(AsyncEngine* jobDispatcher, if (g_settings->getBool("secure.enable_security")) initializeSecurity(); + } else { + initializeSecurity(); } // Prepare job lua environment @@ -287,13 +292,27 @@ AsyncWorkerThread::AsyncWorkerThread(AsyncEngine* jobDispatcher, lua_pop(L, 1); } -/******************************************************************************/ AsyncWorkerThread::~AsyncWorkerThread() { sanity_check(!isRunning()); } -/******************************************************************************/ +bool AsyncWorkerThread::checkPathInternal(const std::string &abs_path, + bool write_required, bool *write_allowed) +{ + auto *L = getStack(); + // dispatch to the right implementation. this should be refactored some day... + if (jobDispatcher->server) { + return ScriptApiSecurity::checkPathWithGamedef(L, abs_path, write_required, write_allowed); + } else { +#if CHECK_CLIENT_BUILD() + return MainMenuScripting::checkPathAccess(abs_path, write_required, write_allowed); +#else + FATAL_ERROR("should never get here"); +#endif + } +} + void* AsyncWorkerThread::run() { if (isErrored) diff --git a/src/script/cpp_api/s_async.h b/src/script/cpp_api/s_async.h index 8a081b2fc..d2a6913ef 100644 --- a/src/script/cpp_api/s_async.h +++ b/src/script/cpp_api/s_async.h @@ -56,10 +56,7 @@ protected: AsyncWorkerThread(AsyncEngine* jobDispatcher, const std::string &name); bool checkPathInternal(const std::string &abs_path, bool write_required, - bool *write_allowed) override { - return ScriptApiSecurity::checkPathWithGamedef(getStack(), - abs_path, write_required, write_allowed); - }; + bool *write_allowed) override; private: AsyncEngine *jobDispatcher = nullptr; diff --git a/src/script/cpp_api/s_security.cpp b/src/script/cpp_api/s_security.cpp index ef882a1f6..40a95ca1a 100644 --- a/src/script/cpp_api/s_security.cpp +++ b/src/script/cpp_api/s_security.cpp @@ -66,6 +66,7 @@ void ScriptApiSecurity::initializeSecurity() "core", "collectgarbage", "DIR_DELIM", + "PLATFORM", "error", "getfenv", "getmetatable", diff --git a/src/script/lua_api/l_mainmenu.cpp b/src/script/lua_api/l_mainmenu.cpp index 69f63d964..e8c969268 100644 --- a/src/script/lua_api/l_mainmenu.cpp +++ b/src/script/lua_api/l_mainmenu.cpp @@ -343,6 +343,8 @@ int ModApiMainMenu::l_get_content_info(lua_State *L) { std::string path = luaL_checkstring(L, 1); + CHECK_SECURE_PATH(L, path.c_str(), false) + ContentSpec spec; spec.path = path; parseContentInfo(spec); @@ -410,6 +412,8 @@ int ModApiMainMenu::l_check_mod_configuration(lua_State *L) { std::string worldpath = luaL_checkstring(L, 1); + CHECK_SECURE_PATH(L, worldpath.c_str(), false) + ModConfiguration modmgr; // Add all game mods @@ -732,15 +736,13 @@ int ModApiMainMenu::l_get_temp_path(lua_State *L) } /******************************************************************************/ -int ModApiMainMenu::l_create_dir(lua_State *L) { +int ModApiMainMenu::l_create_dir(lua_State *L) +{ const char *path = luaL_checkstring(L, 1); - if (ModApiMainMenu::mayModifyPath(path)) { - lua_pushboolean(L, fs::CreateAllDirs(path)); - return 1; - } + CHECK_SECURE_PATH(L, path, true) - lua_pushboolean(L, false); + lua_pushboolean(L, fs::CreateAllDirs(path)); return 1; } @@ -749,14 +751,9 @@ int ModApiMainMenu::l_delete_dir(lua_State *L) { const char *path = luaL_checkstring(L, 1); - std::string absolute_path = fs::RemoveRelativePathComponents(path); + CHECK_SECURE_PATH(L, path, true) - if (ModApiMainMenu::mayModifyPath(absolute_path)) { - lua_pushboolean(L, fs::RecursiveDelete(absolute_path)); - return 1; - } - - lua_pushboolean(L, false); + lua_pushboolean(L, fs::RecursiveDelete(path)); return 1; } @@ -766,24 +763,16 @@ int ModApiMainMenu::l_copy_dir(lua_State *L) const char *source = luaL_checkstring(L, 1); const char *destination = luaL_checkstring(L, 2); - bool keep_source = true; - if (!lua_isnoneornil(L, 3)) - keep_source = readParam(L, 3); + bool keep_source = readParam(L, 3, true); - std::string abs_destination = fs::RemoveRelativePathComponents(destination); - std::string abs_source = fs::RemoveRelativePathComponents(source); - - if (!ModApiMainMenu::mayModifyPath(abs_destination) || - (!keep_source && !ModApiMainMenu::mayModifyPath(abs_source))) { - lua_pushboolean(L, false); - return 1; - } + CHECK_SECURE_PATH(L, source, !keep_source) + CHECK_SECURE_PATH(L, destination, true) bool retval; if (keep_source) - retval = fs::CopyDir(abs_source, abs_destination); + retval = fs::CopyDir(source, destination); else - retval = fs::MoveDir(abs_source, abs_destination); + retval = fs::MoveDir(source, destination); lua_pushboolean(L, retval); return 1; } @@ -793,6 +782,8 @@ int ModApiMainMenu::l_is_dir(lua_State *L) { const char *path = luaL_checkstring(L, 1); + CHECK_SECURE_PATH(L, path, false) + lua_pushboolean(L, fs::IsDir(path)); return 1; } @@ -803,16 +794,12 @@ int ModApiMainMenu::l_extract_zip(lua_State *L) const char *zipfile = luaL_checkstring(L, 1); const char *destination = luaL_checkstring(L, 2); - std::string absolute_destination = fs::RemoveRelativePathComponents(destination); + CHECK_SECURE_PATH(L, zipfile, false) + CHECK_SECURE_PATH(L, destination, true) - if (ModApiMainMenu::mayModifyPath(absolute_destination)) { - auto fs = RenderingEngine::get_raw_device()->getFileSystem(); - bool ok = fs::extractZipFile(fs, zipfile, destination); - lua_pushboolean(L, ok); - return 1; - } - - lua_pushboolean(L,false); + auto fs = RenderingEngine::get_raw_device()->getFileSystem(); + bool ok = fs::extractZipFile(fs, zipfile, destination); + lua_pushboolean(L, ok); return 1; } @@ -826,40 +813,13 @@ int ModApiMainMenu::l_get_mainmenu_path(lua_State *L) return 1; } -/******************************************************************************/ -bool ModApiMainMenu::mayModifyPath(std::string path) -{ - path = fs::RemoveRelativePathComponents(path); - - if (fs::PathStartsWith(path, fs::TempPath())) - return true; - - std::string path_user = fs::RemoveRelativePathComponents(porting::path_user); - - if (fs::PathStartsWith(path, path_user + DIR_DELIM "client")) - return true; - if (fs::PathStartsWith(path, path_user + DIR_DELIM "games")) - return true; - if (fs::PathStartsWith(path, path_user + DIR_DELIM "mods")) - return true; - if (fs::PathStartsWith(path, path_user + DIR_DELIM "textures")) - return true; - if (fs::PathStartsWith(path, path_user + DIR_DELIM "worlds")) - return true; - - if (fs::PathStartsWith(path, fs::RemoveRelativePathComponents(porting::path_cache))) - return true; - - return false; -} - - /******************************************************************************/ int ModApiMainMenu::l_may_modify_path(lua_State *L) { const char *target = luaL_checkstring(L, 1); - std::string absolute_destination = fs::RemoveRelativePathComponents(target); - lua_pushboolean(L, ModApiMainMenu::mayModifyPath(absolute_destination)); + bool write_allowed = false; + bool ok = ScriptApiSecurity::checkPath(L, target, false, &write_allowed); + lua_pushboolean(L, ok && write_allowed); return 1; } @@ -892,19 +852,9 @@ int ModApiMainMenu::l_download_file(lua_State *L) const char *url = luaL_checkstring(L, 1); const char *target = luaL_checkstring(L, 2); - //check path - std::string absolute_destination = fs::RemoveRelativePathComponents(target); + CHECK_SECURE_PATH(L, target, true) - if (ModApiMainMenu::mayModifyPath(absolute_destination)) { - if (GUIEngine::downloadFile(url,absolute_destination)) { - lua_pushboolean(L,true); - return 1; - } - } else { - errorstream << "DOWNLOAD denied: " << absolute_destination - << " isn't an allowed path" << std::endl; - } - lua_pushboolean(L,false); + lua_pushboolean(L, GUIEngine::downloadFile(url, target)); return 1; } @@ -1068,16 +1018,22 @@ int ModApiMainMenu::l_open_url_dialog(lua_State *L) /******************************************************************************/ int ModApiMainMenu::l_open_dir(lua_State *L) { - std::string path = luaL_checkstring(L, 1); - lua_pushboolean(L, porting::open_directory(path)); + const char *target = luaL_checkstring(L, 1); + + CHECK_SECURE_PATH(L, target, false) + + lua_pushboolean(L, porting::open_directory(target)); return 1; } /******************************************************************************/ int ModApiMainMenu::l_share_file(lua_State *L) { + const char *path = luaL_checkstring(L, 1); + + CHECK_SECURE_PATH(L, path, false) + #ifdef __ANDROID__ - std::string path = luaL_checkstring(L, 1); porting::shareFileAndroid(path); lua_pushboolean(L, true); #else @@ -1091,19 +1047,20 @@ int ModApiMainMenu::l_do_async_callback(lua_State *L) { MainMenuScripting *script = getScriptApi(L); - size_t func_length, param_length; - const char* serialized_func_raw = luaL_checklstring(L, 1, &func_length); - const char* serialized_param_raw = luaL_checklstring(L, 2, ¶m_length); + luaL_checktype(L, 1, LUA_TFUNCTION); + call_string_dump(L, 1); + size_t func_length; + const char *serialized_func_raw = lua_tolstring(L, -1, &func_length); - sanity_check(serialized_func_raw != NULL); - sanity_check(serialized_param_raw != NULL); + size_t param_length; + const char* serialized_param_raw = luaL_checklstring(L, 2, ¶m_length); u32 jobId = script->queueAsync( std::string(serialized_func_raw, func_length), std::string(serialized_param_raw, param_length)); + lua_settop(L, 0); lua_pushinteger(L, jobId); - return 1; } diff --git a/src/script/lua_api/l_mainmenu.h b/src/script/lua_api/l_mainmenu.h index 25ea551bc..d0f72b6c4 100644 --- a/src/script/lua_api/l_mainmenu.h +++ b/src/script/lua_api/l_mainmenu.h @@ -37,14 +37,6 @@ private: */ static int getBoolData(lua_State *L, const std::string &name ,bool& valid); - /** - * Checks if a path may be modified. Paths in the temp directory or the user - * games, mods, textures, or worlds directories may be modified. - * @param path path to check - * @return true if the path may be modified - */ - static bool mayModifyPath(std::string path); - //api calls static int l_start(lua_State *L); diff --git a/src/script/scripting_mainmenu.cpp b/src/script/scripting_mainmenu.cpp index 17e2ebb1e..d5434c325 100644 --- a/src/script/scripting_mainmenu.cpp +++ b/src/script/scripting_mainmenu.cpp @@ -12,11 +12,14 @@ #include "lua_api/l_util.h" #include "lua_api/l_settings.h" #include "log.h" +#include "filesys.h" +#include "porting.h" extern "C" { #include "lualib.h" } -#define MAINMENU_NUM_ASYNC_THREADS 4 + +#define MAINMENU_NUM_ASYNC_THREADS 2 MainMenuScripting::MainMenuScripting(GUIEngine* guiengine): @@ -26,6 +29,8 @@ MainMenuScripting::MainMenuScripting(GUIEngine* guiengine): SCRIPTAPI_PRECHECKHEADER + initializeSecurity(); + lua_getglobal(L, "core"); int top = lua_gettop(L); @@ -69,6 +74,42 @@ void MainMenuScripting::registerLuaClasses(lua_State *L, int top) MainMenuSoundHandle::Register(L); } +bool MainMenuScripting::mayModifyPath(const std::string &path) +{ + if (fs::PathStartsWith(path, fs::TempPath())) + return true; + + std::string path_user = fs::RemoveRelativePathComponents(porting::path_user); + + if (fs::PathStartsWith(path, path_user + DIR_DELIM "client")) + return true; + if (fs::PathStartsWith(path, path_user + DIR_DELIM "games")) + return true; + if (fs::PathStartsWith(path, path_user + DIR_DELIM "mods")) + return true; + if (fs::PathStartsWith(path, path_user + DIR_DELIM "textures")) + return true; + if (fs::PathStartsWith(path, path_user + DIR_DELIM "worlds")) + return true; + + if (fs::PathStartsWith(path, fs::RemoveRelativePathComponents(porting::path_cache))) + return true; + + return false; +} + +bool MainMenuScripting::checkPathAccess(const std::string &abs_path, bool write_required, + bool *write_allowed) +{ + if (mayModifyPath(abs_path)) { + if (write_allowed) + *write_allowed = true; + return true; + } + // TODO?: global read access sounds too broad + return !write_required; +} + void MainMenuScripting::beforeClose() { SCRIPTAPI_PRECHECKHEADER diff --git a/src/script/scripting_mainmenu.h b/src/script/scripting_mainmenu.h index 8b7c0cfe8..f56c67727 100644 --- a/src/script/scripting_mainmenu.h +++ b/src/script/scripting_mainmenu.h @@ -6,6 +6,7 @@ #include "cpp_api/s_base.h" #include "cpp_api/s_mainmenu.h" +#include "cpp_api/s_security.h" #include "cpp_api/s_async.h" /*****************************************************************************/ @@ -14,7 +15,8 @@ class MainMenuScripting : virtual public ScriptApiBase, - public ScriptApiMainMenu + public ScriptApiMainMenu, + public ScriptApiSecurity { public: MainMenuScripting(GUIEngine* guiengine); @@ -29,6 +31,19 @@ public: u32 queueAsync(std::string &&serialized_func, std::string &&serialized_param); + // Is the main menu allowed writeable access to this path? + static bool mayModifyPath(const std::string &path); + + // (public implementation so it can be used from AsyncEngine) + static bool checkPathAccess(const std::string &abs_path, bool write_required, + bool *write_allowed); + +protected: + bool checkPathInternal(const std::string &abs_path, bool write_required, + bool *write_allowed) override { + return checkPathAccess(abs_path, write_required, write_allowed); + } + private: void initializeModApi(lua_State *L, int top); static void registerLuaClasses(lua_State *L, int top);