From c0f81aced7e8939682abbbf89f036df002233d81 Mon Sep 17 00:00:00 2001 From: Mm2PL Date: Sun, 10 Dec 2023 23:52:06 +0100 Subject: [PATCH 1/6] Use require() instead of a custom import() --- src/controllers/plugins/LuaAPI.cpp | 77 +++++--------------- src/controllers/plugins/LuaAPI.hpp | 4 +- src/controllers/plugins/PluginController.cpp | 58 +++++++++++---- src/controllers/plugins/PluginController.hpp | 3 +- 4 files changed, 66 insertions(+), 76 deletions(-) diff --git a/src/controllers/plugins/LuaAPI.cpp b/src/controllers/plugins/LuaAPI.cpp index e34e063b434..7defbb408f0 100644 --- a/src/controllers/plugins/LuaAPI.cpp +++ b/src/controllers/plugins/LuaAPI.cpp @@ -282,69 +282,28 @@ int g_load(lua_State *L) # endif } -int g_import(lua_State *L) +int safeluasearcher(lua_State *L) { - auto countArgs = lua_gettop(L); - // Lua allows dofile() which loads from stdin, but this is very useless in our case - if (countArgs == 0) - { - lua_pushnil(L); - luaL_error(L, "it is not allowed to call import() without arguments"); - return 1; - } - - auto *pl = getApp()->plugins->getPluginByStatePtr(L); - QString fname; - if (!lua::pop(L, &fname)) - { - lua_pushnil(L); - luaL_error(L, "chatterino g_import: expected a string for a filename"); - return 1; - } - auto dir = QUrl(pl->loadDirectory().canonicalPath() + "/"); - auto file = dir.resolved(fname); - - qCDebug(chatterinoLua) << "plugin" << pl->id << "is trying to load" << file - << "(its dir is" << dir << ")"; - if (!dir.isParentOf(file)) - { - lua_pushnil(L); - luaL_error(L, "chatterino g_import: filename must be inside of the " - "plugin directory"); - return 1; - } - - auto path = file.path(QUrl::FullyDecoded); - QFile qf(path); - qf.open(QIODevice::ReadOnly); - if (qf.size() > 10'000'000) + const char *name = luaL_checkstring(L, 1); + QString str; + lua::peek(L, &str, lua_upvalueindex(1)); + str += QDir::separator(); + str += name; + str += ".lua"; + + auto temp = str.toStdString(); + + const auto *filename = temp.c_str(); + auto res = luaL_loadfilex(L, filename, "t"); + // Yoinked from checkload lib/lua/src/loadlib.c + if (res == LUA_OK) { - lua_pushnil(L); - luaL_error(L, "chatterino g_import: size limit of 10MB exceeded, what " - "the hell are you doing"); - return 1; + lua_pushstring(L, filename); + return 2; } - // validate utf-8 to block bytecode exploits - auto data = qf.readAll(); - auto *utf8 = QTextCodec::codecForName("UTF-8"); - QTextCodec::ConverterState state; - utf8->toUnicode(data.constData(), data.size(), &state); - if (state.invalidChars != 0) - { - lua_pushnil(L); - luaL_error(L, "invalid utf-8 in import() target (%s) is not allowed", - fname.toStdString().c_str()); - return 1; - } - - // fetch dofile and call it - lua_getfield(L, LUA_REGISTRYINDEX, "real_dofile"); - // maybe data race here if symlink was swapped? - lua::push(L, path); - lua_call(L, 1, LUA_MULTRET); - - return lua_gettop(L); + return luaL_error(L, "error loading module '%s' from file '%s':\n\t%s", + lua_tostring(L, 1), filename, lua_tostring(L, -1)); } int g_print(lua_State *L) diff --git a/src/controllers/plugins/LuaAPI.hpp b/src/controllers/plugins/LuaAPI.hpp index 95a9bd1927f..86dbee3bae0 100644 --- a/src/controllers/plugins/LuaAPI.hpp +++ b/src/controllers/plugins/LuaAPI.hpp @@ -20,9 +20,11 @@ int c2_log(lua_State *L); // These ones are global int g_load(lua_State *L); int g_print(lua_State *L); -int g_import(lua_State *L); // NOLINTEND(readability-identifier-naming) +// This is for require() exposed as an element of package.searchers +int safeluasearcher(lua_State *L); + // Exposed as c2.LogLevel // Represents "calls" to qCDebug, qCInfo ... enum class LogLevel { Debug, Info, Warning, Critical }; diff --git a/src/controllers/plugins/PluginController.cpp b/src/controllers/plugins/PluginController.cpp index f3829ac26a1..cddd83f94fc 100644 --- a/src/controllers/plugins/PluginController.cpp +++ b/src/controllers/plugins/PluginController.cpp @@ -103,9 +103,10 @@ bool PluginController::tryLoadFromDir(const QDir &pluginDir) return true; } -void PluginController::openLibrariesFor(lua_State *L, - const PluginMeta & /*meta*/) +void PluginController::openLibrariesFor(lua_State *L, const PluginMeta &meta, + const QDir &pluginDir) { + lua::StackGuard guard(L); // Stuff to change, remove or hide behind a permission system: static const std::vector loadedlibs = { luaL_Reg{LUA_GNAME, luaopen_base}, @@ -123,6 +124,7 @@ void PluginController::openLibrariesFor(lua_State *L, luaL_Reg{LUA_STRLIBNAME, luaopen_string}, luaL_Reg{LUA_MATHLIBNAME, luaopen_math}, luaL_Reg{LUA_UTF8LIBNAME, luaopen_utf8}, + luaL_Reg{LUA_LOADLIBNAME, luaopen_package}, }; // Warning: Do not add debug library to this, it would make the security of // this a living nightmare due to stuff like registry access @@ -144,7 +146,7 @@ void PluginController::openLibrariesFor(lua_State *L, {nullptr, nullptr}, }; lua_pushglobaltable(L); - auto global = lua_gettop(L); + auto gtable = lua_gettop(L); // count of elements in C2LIB + LogLevel + EventType auto c2libIdx = lua::pushEmptyTable(L, 8); @@ -157,14 +159,11 @@ void PluginController::openLibrariesFor(lua_State *L, lua::pushEnumTable(L); lua_setfield(L, c2libIdx, "EventType"); - lua_setfield(L, global, "c2"); + lua_setfield(L, gtable, "c2"); // ban functions // Note: this might not be fully secure? some kind of metatable fuckery might come up? - lua_pushglobaltable(L); - auto gtable = lua_gettop(L); - // possibly randomize this name at runtime to prevent some attacks? # ifndef NDEBUG @@ -172,16 +171,10 @@ void PluginController::openLibrariesFor(lua_State *L, lua_setfield(L, LUA_REGISTRYINDEX, "real_load"); # endif - lua_getfield(L, gtable, "dofile"); - lua_setfield(L, LUA_REGISTRYINDEX, "real_dofile"); - // NOLINTNEXTLINE(*-avoid-c-arrays) static const luaL_Reg replacementFuncs[] = { {"load", lua::api::g_load}, {"print", lua::api::g_print}, - - // This function replaces both `dofile` and `require`, see docs/wip-plugins.md for more info - {"import", lua::api::g_import}, {nullptr, nullptr}, }; luaL_setfuncs(L, replacementFuncs, 0); @@ -192,7 +185,42 @@ void PluginController::openLibrariesFor(lua_State *L, lua_pushnil(L); lua_setfield(L, gtable, "dofile"); - lua_pop(L, 1); + // set up package lib + lua_getfield(L, gtable, "package"); + + auto package = lua_gettop(L); + lua_pushstring(L, ""); + lua_setfield(L, package, "cpath"); + + // we don't use path + lua_pushstring(L, ""); + lua_setfield(L, package, "path"); + + { + lua_getfield(L, gtable, "table"); + auto table = lua_gettop(L); + lua_getfield(L, -1, "remove"); + lua_remove(L, table); + } + auto remove = lua_gettop(L); + + // remove searcher_Croot, searcher_C and searcher_Lua leaving only searcher_preload + for (int i = 0; i < 3; i++) + { + lua_pushvalue(L, remove); + lua_getfield(L, package, "searchers"); + lua_pcall(L, 1, 0, 0); + } + lua_pop(L, 1); // get rid of remove + + lua_getfield(L, package, "searchers"); + + // this includes trailing `/` or `\` + lua::push(L, QString(pluginDir.absolutePath())); + lua_pushcclosure(L, lua::api::safeluasearcher, 1); + + lua_seti(L, -2, 2); + lua_pop(L, 3); // remove gtable, package, package.searchers } void PluginController::load(const QFileInfo &index, const QDir &pluginDir, @@ -211,7 +239,7 @@ void PluginController::load(const QFileInfo &index, const QDir &pluginDir, << " because safe mode is enabled."; return; } - PluginController::openLibrariesFor(l, meta); + PluginController::openLibrariesFor(l, meta, pluginDir); if (!PluginController::isPluginEnabled(pluginName) || !getSettings()->pluginsEnabled) diff --git a/src/controllers/plugins/PluginController.hpp b/src/controllers/plugins/PluginController.hpp index bc3eca738b8..8dc698b406d 100644 --- a/src/controllers/plugins/PluginController.hpp +++ b/src/controllers/plugins/PluginController.hpp @@ -63,7 +63,8 @@ class PluginController : public Singleton const PluginMeta &meta); // This function adds lua standard libraries into the state - static void openLibrariesFor(lua_State *L, const PluginMeta & /*meta*/); + static void openLibrariesFor(lua_State *L, const PluginMeta & /*meta*/, + const QDir &pluginDir); static void loadChatterinoLib(lua_State *l); bool tryLoadFromDir(const QDir &pluginDir); std::map> plugins_; From 43614e970b5481f50b6c434d3e3204c9723ec9a7 Mon Sep 17 00:00:00 2001 From: Mm2PL Date: Mon, 11 Dec 2023 00:21:37 +0100 Subject: [PATCH 2/6] Also search relative to the current file --- src/controllers/plugins/LuaAPI.cpp | 76 +++++++++++++++++--- src/controllers/plugins/LuaAPI.hpp | 3 +- src/controllers/plugins/PluginController.cpp | 7 +- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/controllers/plugins/LuaAPI.cpp b/src/controllers/plugins/LuaAPI.cpp index 7defbb408f0..127412178ac 100644 --- a/src/controllers/plugins/LuaAPI.cpp +++ b/src/controllers/plugins/LuaAPI.cpp @@ -15,6 +15,7 @@ # include # include # include +# include namespace { using namespace chatterino; @@ -282,18 +283,32 @@ int g_load(lua_State *L) # endif } -int safeluasearcher(lua_State *L) +int loadfile(lua_State *L, const QString &str) { - const char *name = luaL_checkstring(L, 1); - QString str; - lua::peek(L, &str, lua_upvalueindex(1)); - str += QDir::separator(); - str += name; - str += ".lua"; + auto *pl = getApp()->plugins->getPluginByStatePtr(L); + if (pl == nullptr) + { + return luaL_error(L, "loadfile: internal error: no plugin?"); + } + auto dir = QUrl(pl->loadDirectory().canonicalPath() + "/"); - auto temp = str.toStdString(); + if (!dir.isParentOf(str)) + { + // XXX: This intentionally hides the resolved path to not leak it + lua::push( + L, QString("requested module is outside of the plugin directory")); + return 1; + } + QFileInfo info(str); + if (!info.exists()) + { + lua::push(L, QString("no file '%1'").arg(str)); + return 1; + } + auto temp = str.toStdString(); const auto *filename = temp.c_str(); + auto res = luaL_loadfilex(L, filename, "t"); // Yoinked from checkload lib/lua/src/loadlib.c if (res == LUA_OK) @@ -306,6 +321,51 @@ int safeluasearcher(lua_State *L) lua_tostring(L, 1), filename, lua_tostring(L, -1)); } +int searcherAbsolute(lua_State *L) +{ + auto name = QString::fromUtf8(luaL_checkstring(L, 1)); + name = name.replace('.', QDir::separator()); + + QString filename; + auto *pl = getApp()->plugins->getPluginByStatePtr(L); + if (pl == nullptr) + { + return luaL_error(L, "searcherAbsolute: internal error: no plugin?"); + } + + QFileInfo file = pl->loadDirectory().filePath(name + ".lua"); + return loadfile(L, file.canonicalFilePath()); +} + +int searcherRelative(lua_State *L) +{ + lua_Debug dbg; + lua_getstack(L, 1, &dbg); + lua_getinfo(L, "S", &dbg); + auto currentFile = QString::fromUtf8(dbg.source, dbg.srclen); + if (currentFile.startsWith("@")) + { + currentFile = currentFile.mid(1); + } + if (currentFile == "=[C]" || currentFile == "") + { + lua::push( + L, + QString( + "Unable to load relative to file:caller has no source file")); + return 1; + } + + auto parent = QFileInfo(currentFile).dir(); + + auto name = QString::fromUtf8(luaL_checkstring(L, 1)); + name = name.replace('.', QDir::separator()); + QString filename = + parent.canonicalPath() + QDir::separator() + name + ".lua"; + + return loadfile(L, filename); +} + int g_print(lua_State *L) { auto *pl = getApp()->plugins->getPluginByStatePtr(L); diff --git a/src/controllers/plugins/LuaAPI.hpp b/src/controllers/plugins/LuaAPI.hpp index 86dbee3bae0..155afc7074b 100644 --- a/src/controllers/plugins/LuaAPI.hpp +++ b/src/controllers/plugins/LuaAPI.hpp @@ -23,7 +23,8 @@ int g_print(lua_State *L); // NOLINTEND(readability-identifier-naming) // This is for require() exposed as an element of package.searchers -int safeluasearcher(lua_State *L); +int searcherAbsolute(lua_State *L); +int searcherRelative(lua_State *L); // Exposed as c2.LogLevel // Represents "calls" to qCDebug, qCInfo ... diff --git a/src/controllers/plugins/PluginController.cpp b/src/controllers/plugins/PluginController.cpp index cddd83f94fc..55def19cee8 100644 --- a/src/controllers/plugins/PluginController.cpp +++ b/src/controllers/plugins/PluginController.cpp @@ -214,12 +214,13 @@ void PluginController::openLibrariesFor(lua_State *L, const PluginMeta &meta, lua_pop(L, 1); // get rid of remove lua_getfield(L, package, "searchers"); + lua_pushcclosure(L, lua::api::searcherRelative, 0); + lua_seti(L, -2, 2); - // this includes trailing `/` or `\` lua::push(L, QString(pluginDir.absolutePath())); - lua_pushcclosure(L, lua::api::safeluasearcher, 1); + lua_pushcclosure(L, lua::api::searcherAbsolute, 1); + lua_seti(L, -2, 3); - lua_seti(L, -2, 2); lua_pop(L, 3); // remove gtable, package, package.searchers } From 5f84db77cee0dabc5140ee213525d80df6aa69dc Mon Sep 17 00:00:00 2001 From: Mm2PL Date: Mon, 11 Dec 2023 01:29:51 +0100 Subject: [PATCH 3/6] Update documentation --- docs/wip-plugins.md | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/docs/wip-plugins.md b/docs/wip-plugins.md index 8bb0cf61c0d..2c2f08f7571 100644 --- a/docs/wip-plugins.md +++ b/docs/wip-plugins.md @@ -158,18 +158,24 @@ It achieves this by forcing all inputs to be encoded with `UTF-8`. See [official documentation](https://www.lua.org/manual/5.4/manual.html#pdf-load) -#### `import(filename)` +#### `require(modname)` -This function mimics Lua's `dofile` however relative paths are relative to your plugin's directory. -You are restricted to loading files in your plugin's directory. You cannot load files with bytecode inside. +This is Lua's [`require()`](https://www.lua.org/manual/5.3/manual.html#pdf-require) function. +However the searcher and load configuration is notably different than default: + - Lua's built-in dynamic library searcher is removed, + - `package.path` is not used, in its place are two searchers, + - when `require()` is used, first a file relative to the currently executing + file will be checked, then a file relative to the plugin directory, + - binary chunks are never loaded + +As in normal Lua, dots are converted to the path separators (`'/'` on Linux and Mac, `'\'` on Windows). Example: ```lua -import("stuff.lua") -- executes Plugins/name/stuff.lua -import("./stuff.lua") -- executes Plugins/name/stuff.lua -import("../stuff.lua") -- tries to load Plugins/stuff.lua and errors -import("luac.out") -- tried to load Plugins/name/luac.out and errors because it contains non-utf8 data +require("stuff") -- executes Plugins/name/stuff.lua or $(dirname $CURR_FILE)/stuff.lua +require("dir.name") -- executes Plugins/name/dir/name.lua or $(dirname $CURR_FILE)/dir/name.lua +require("binary") -- tried to load Plugins/name/binary.lua and errors because binary is not a text file ``` #### `print(Args...)` From 4eed25a2841b889b8f999d69ecb503ade39bcbae Mon Sep 17 00:00:00 2001 From: Mm2PL Date: Mon, 11 Dec 2023 01:32:51 +0100 Subject: [PATCH 4/6] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd00604d3bd..c3fed2b820a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,7 @@ - Dev: Refactored the Image Uploader feature. (#4971) - Dev: Fixed deadlock and use-after-free in tests. (#4981) - Dev: Load less message history upon reconnects. (#5001) +- Dev: BREAKING: Replace custom `import()` with normal Lua `require()`. (#5014) ## 2.4.6 From 55ed116d362c74717741f0bdea9bc79756dcba68 Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sat, 16 Dec 2023 12:50:16 +0100 Subject: [PATCH 5/6] Run prettier --- docs/wip-plugins.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/wip-plugins.md b/docs/wip-plugins.md index 2c2f08f7571..9a3ee25cea8 100644 --- a/docs/wip-plugins.md +++ b/docs/wip-plugins.md @@ -162,11 +162,12 @@ See [official documentation](https://www.lua.org/manual/5.4/manual.html#pdf-load This is Lua's [`require()`](https://www.lua.org/manual/5.3/manual.html#pdf-require) function. However the searcher and load configuration is notably different than default: - - Lua's built-in dynamic library searcher is removed, - - `package.path` is not used, in its place are two searchers, - - when `require()` is used, first a file relative to the currently executing - file will be checked, then a file relative to the plugin directory, - - binary chunks are never loaded + +- Lua's built-in dynamic library searcher is removed, +- `package.path` is not used, in its place are two searchers, +- when `require()` is used, first a file relative to the currently executing + file will be checked, then a file relative to the plugin directory, +- binary chunks are never loaded As in normal Lua, dots are converted to the path separators (`'/'` on Linux and Mac, `'\'` on Windows). From 9beaab4597d86dbb8aa655019bd7279bd9492ac9 Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sat, 16 Dec 2023 13:00:02 +0100 Subject: [PATCH 6/6] fix QFileInfo ctor use --- src/controllers/plugins/LuaAPI.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/plugins/LuaAPI.cpp b/src/controllers/plugins/LuaAPI.cpp index 127412178ac..d8c1b676c87 100644 --- a/src/controllers/plugins/LuaAPI.cpp +++ b/src/controllers/plugins/LuaAPI.cpp @@ -333,7 +333,7 @@ int searcherAbsolute(lua_State *L) return luaL_error(L, "searcherAbsolute: internal error: no plugin?"); } - QFileInfo file = pl->loadDirectory().filePath(name + ".lua"); + QFileInfo file(pl->loadDirectory().filePath(name + ".lua")); return loadfile(L, file.canonicalFilePath()); }