From 3731580b6b80c586322cadc6bc4be2b8b2bbb206 Mon Sep 17 00:00:00 2001 From: meir Date: Sun, 6 Feb 2022 17:41:55 +0200 Subject: [PATCH] Protect globals of both evals scripts and functions. Use the new `lua_enablereadonlytable` Lua API to protect the global tables of both evals scripts and functions. For eval scripts, the implemetation is easy, We simply call `lua_enablereadonlytable` on the global table to turn it into a readonly table. On functions its more complecated, we want to be able to switch globals between load run and function run. To achieve this, we create a new empty table that acts as the globals table for function, we control the actual globals using metatable manipulation. Notice that even if the user gets a pointer to the original tables, all the tables are set to be readonly (using `lua_enablereadonlytable` Lua API) so he can not change them. The following inlustration better explain the solution: ``` Global table {} <- global table metatable {.__index = __real_globals__} ``` The `__real_globals__` is set depends on the run context (function load or function call). Why this solution is needed and its not enough to simply switch globals? When we run in the context of function load and create our functions, our function gets the current globals that was set when they were created. Replacing the globals after the creation will not effect them. This is why this trick it mandatory. --- src/eval.c | 8 +-- src/function_lua.c | 115 +++++++++++++++------------------- src/script_lua.c | 132 +++++++++++---------------------------- src/script_lua.h | 1 - tests/unit/functions.tcl | 65 ++++++++++--------- tests/unit/scripting.tcl | 39 +++++++++++- 6 files changed, 166 insertions(+), 194 deletions(-) diff --git a/src/eval.c b/src/eval.c index 1be0d42f2..f9998b9ed 100644 --- a/src/eval.c +++ b/src/eval.c @@ -266,10 +266,10 @@ void scriptingInit(int setup) { lctx.lua_client->flags |= CLIENT_DENY_BLOCKING; } - /* Lua beginners often don't use "local", this is likely to introduce - * subtle bugs in their code. To prevent problems we protect accesses - * to global variables. */ - luaEnableGlobalsProtection(lua); + /* Lock the global table from any changes */ + lua_pushvalue(lua, LUA_GLOBALSINDEX); + luaSetGlobalProtection(lua); + lua_pop(lua, 1); lctx.lua = lua; } diff --git a/src/function_lua.c b/src/function_lua.c index 1e6363fd4..f8ca51f05 100644 --- a/src/function_lua.c +++ b/src/function_lua.c @@ -50,6 +50,7 @@ #define REGISTRY_ERROR_HANDLER_NAME "__ERROR_HANDLER__" #define REGISTRY_LOAD_CTX_NAME "__LIBRARY_CTX__" #define LIBRARY_API_NAME "__LIBRARY_API__" +#define GLOBALS_API_NAME "__GLOBALS_API__" #define LOAD_TIMEOUT_MS 500 /* Lua engine ctx */ @@ -99,42 +100,23 @@ static void luaEngineLoadHook(lua_State *lua, lua_Debug *ar) { * Return NULL on compilation error and set the error to the err variable */ static int luaEngineCreate(void *engine_ctx, functionLibInfo *li, sds blob, sds *err) { + int ret = C_ERR; luaEngineCtx *lua_engine_ctx = engine_ctx; lua_State *lua = lua_engine_ctx->lua; - /* Each library will have its own global distinct table. - * We will create a new fresh Lua table and use - * lua_setfenv to set the table as the library globals - * (https://www.lua.org/manual/5.1/manual.html#lua_setfenv) - * - * At first, populate this new table with only the 'library' API - * to make sure only 'library' API is available at start. After the - * initial run is finished and all functions are registered, add - * all the default globals to the library global table and delete - * the library API. - * - * There are 2 ways to achieve the last part (add default - * globals to the new table): - * - * 1. Initialize the new table with all the default globals - * 2. Inheritance using metatable (https://www.lua.org/pil/14.3.html) - * - * For now we are choosing the second, we can change it in the future to - * achieve a better isolation between functions. */ - lua_newtable(lua); /* Global table for the library */ - lua_pushstring(lua, REDIS_API_NAME); - lua_pushstring(lua, LIBRARY_API_NAME); - lua_gettable(lua, LUA_REGISTRYINDEX); /* get library function from registry */ - lua_settable(lua, -3); /* push the library table to the new global table */ - - /* Set global protection on the new global table */ - luaSetGlobalProtection(lua_engine_ctx->lua); + /* set load library globals */ + lua_getmetatable(lua, LUA_GLOBALSINDEX); + lua_enablereadonlytable(lua, -1, 0); /* disable global protection */ + lua_getfield(lua, LUA_REGISTRYINDEX, LIBRARY_API_NAME); + lua_setfield(lua, -2, "__index"); + lua_enablereadonlytable(lua, LUA_GLOBALSINDEX, 1); /* enable global protection */ + lua_pop(lua, 1); /* pop the metatable */ /* compile the code */ if (luaL_loadbuffer(lua, blob, sdslen(blob), "@user_function")) { *err = sdscatprintf(sdsempty(), "Error compiling function: %s", lua_tostring(lua, -1)); - lua_pop(lua, 2); /* pops the error and globals table */ - return C_ERR; + lua_pop(lua, 1); /* pops the error */ + goto done; } serverAssert(lua_isfunction(lua, -1)); @@ -144,45 +126,31 @@ static int luaEngineCreate(void *engine_ctx, functionLibInfo *li, sds blob, sds }; luaSaveOnRegistry(lua, REGISTRY_LOAD_CTX_NAME, &load_ctx); - /* set the function environment so only 'library' API can be accessed. */ - lua_pushvalue(lua, -2); /* push global table to the front */ - lua_setfenv(lua, -2); - lua_sethook(lua,luaEngineLoadHook,LUA_MASKCOUNT,100000); /* Run the compiled code to allow it to register functions */ if (lua_pcall(lua,0,0,0)) { errorInfo err_info = {0}; luaExtractErrorInformation(lua, &err_info); *err = sdscatprintf(sdsempty(), "Error registering functions: %s", err_info.msg); - lua_pop(lua, 2); /* pops the error and globals table */ - lua_sethook(lua,NULL,0,0); /* Disable hook */ - luaSaveOnRegistry(lua, REGISTRY_LOAD_CTX_NAME, NULL); + lua_pop(lua, 1); /* pops the error */ luaErrorInformationDiscard(&err_info); - return C_ERR; + goto done; } + + ret = C_OK; + +done: + /* restore original globals */ + lua_getmetatable(lua, LUA_GLOBALSINDEX); + lua_enablereadonlytable(lua, -1, 0); /* disable global protection */ + lua_getfield(lua, LUA_REGISTRYINDEX, GLOBALS_API_NAME); + lua_setfield(lua, -2, "__index"); + lua_enablereadonlytable(lua, LUA_GLOBALSINDEX, 1); /* enable global protection */ + lua_pop(lua, 1); /* pop the metatable */ + lua_sethook(lua,NULL,0,0); /* Disable hook */ luaSaveOnRegistry(lua, REGISTRY_LOAD_CTX_NAME, NULL); - - /* stack contains the global table, lets rearrange it to contains the entire API. */ - /* delete 'redis' API */ - lua_pushstring(lua, REDIS_API_NAME); - lua_pushnil(lua); - lua_settable(lua, -3); - - /* create metatable */ - lua_newtable(lua); - lua_pushstring(lua, "__index"); - lua_pushvalue(lua, LUA_GLOBALSINDEX); /* push original globals */ - lua_settable(lua, -3); - lua_pushstring(lua, "__newindex"); - lua_pushvalue(lua, LUA_GLOBALSINDEX); /* push original globals */ - lua_settable(lua, -3); - - lua_setmetatable(lua, -2); - - lua_pop(lua, 1); /* pops the global table */ - - return C_OK; + return ret; } /* @@ -458,8 +426,8 @@ int luaEngineInitEngine() { luaRegisterRedisAPI(lua_engine_ctx->lua); /* Register the library commands table and fields and store it to registry */ - lua_pushstring(lua_engine_ctx->lua, LIBRARY_API_NAME); - lua_newtable(lua_engine_ctx->lua); + lua_newtable(lua_engine_ctx->lua); /* load library globals */ + lua_newtable(lua_engine_ctx->lua); /* load library `redis` table */ lua_pushstring(lua_engine_ctx->lua, "register_function"); lua_pushcfunction(lua_engine_ctx->lua, luaRegisterFunction); @@ -468,7 +436,12 @@ int luaEngineInitEngine() { luaRegisterLogFunction(lua_engine_ctx->lua); luaRegisterVersion(lua_engine_ctx->lua); - lua_settable(lua_engine_ctx->lua, LUA_REGISTRYINDEX); + luaSetGlobalProtection(lua_engine_ctx->lua); /* protect redis */ + + lua_setfield(lua_engine_ctx->lua, -2, REDIS_API_NAME); + + luaSetGlobalProtection(lua_engine_ctx->lua); /* protect load library globals */ + lua_setfield(lua_engine_ctx->lua, LUA_REGISTRYINDEX, LIBRARY_API_NAME); /* Save error handler to registry */ lua_pushstring(lua_engine_ctx->lua, REGISTRY_ERROR_HANDLER_NAME); @@ -492,17 +465,29 @@ int luaEngineInitEngine() { lua_pcall(lua_engine_ctx->lua,0,1,0); lua_settable(lua_engine_ctx->lua, LUA_REGISTRYINDEX); - /* Save global protection to registry */ - luaRegisterGlobalProtectionFunction(lua_engine_ctx->lua); - - /* Set global protection on globals */ lua_pushvalue(lua_engine_ctx->lua, LUA_GLOBALSINDEX); luaSetGlobalProtection(lua_engine_ctx->lua); lua_pop(lua_engine_ctx->lua, 1); + /* Save default globals to registry */ + lua_pushvalue(lua_engine_ctx->lua, LUA_GLOBALSINDEX); + lua_setfield(lua_engine_ctx->lua, LUA_REGISTRYINDEX, GLOBALS_API_NAME); + /* save the engine_ctx on the registry so we can get it from the Lua interpreter */ luaSaveOnRegistry(lua_engine_ctx->lua, REGISTRY_ENGINE_CTX_NAME, lua_engine_ctx); + /* Create new empty table to be the new globals, we will be able to control the real globals + * using metatable */ + lua_newtable(lua_engine_ctx->lua); /* new globals */ + lua_newtable(lua_engine_ctx->lua); /* new globals metatable */ + lua_pushvalue(lua_engine_ctx->lua, LUA_GLOBALSINDEX); + lua_setfield(lua_engine_ctx->lua, -2, "__index"); + lua_enablereadonlytable(lua_engine_ctx->lua, -1, 1); /* protect the metatable */ + lua_setmetatable(lua_engine_ctx->lua, -2); + lua_enablereadonlytable(lua_engine_ctx->lua, -1, 1); /* protect the new global table */ + lua_replace(lua_engine_ctx->lua, LUA_GLOBALSINDEX); /* set new global table as the new globals */ + + engine *lua_engine = zmalloc(sizeof(*lua_engine)); *lua_engine = (engine) { .engine_ctx = lua_engine_ctx, diff --git a/src/script_lua.c b/src/script_lua.c index 4e1f17649..406706b7f 100644 --- a/src/script_lua.c +++ b/src/script_lua.c @@ -1135,107 +1135,39 @@ sds luaGetStringSds(lua_State *lua, int index) { return str_sds; } -/* This function installs metamethods in the global table _G that prevent - * the creation of globals accidentally. - * - * It should be the last to be called in the scripting engine initialization - * sequence, because it may interact with creation of globals. - * - * On Legacy Lua (eval) we need to check 'w ~= \"main\"' otherwise we will not be able - * to create the global 'function ()' variable. On Functions Lua engine we do not use - * this trick so it's not needed. */ -void luaEnableGlobalsProtection(lua_State *lua) { - char *s[32]; - sds code = sdsempty(); - int j = 0; - - /* strict.lua from: http://metalua.luaforge.net/src/lib/strict.lua.html. - * Modified to be adapted to Redis. */ - s[j++]="local dbg=debug\n"; - s[j++]="local mt = {}\n"; - s[j++]="setmetatable(_G, mt)\n"; - s[j++]="mt.__newindex = function (t, n, v)\n"; - s[j++]=" if dbg.getinfo(2) then\n"; - s[j++]=" local w = dbg.getinfo(2, \"S\").what\n"; - s[j++]=" if w ~= \"C\" then\n"; - s[j++]=" error(\"Script attempted to create global variable '\"..tostring(n)..\"'\", 2)\n"; - s[j++]=" end\n"; - s[j++]=" end\n"; - s[j++]=" rawset(t, n, v)\n"; - s[j++]="end\n"; - s[j++]="mt.__index = function (t, n)\n"; - s[j++]=" if dbg.getinfo(2) and dbg.getinfo(2, \"S\").what ~= \"C\" then\n"; - s[j++]=" error(\"Script attempted to access nonexistent global variable '\"..tostring(n)..\"'\", 2)\n"; - s[j++]=" end\n"; - s[j++]=" return rawget(t, n)\n"; - s[j++]="end\n"; - s[j++]="debug = nil\n"; - s[j++]=NULL; - - for (j = 0; s[j] != NULL; j++) code = sdscatlen(code,s[j],strlen(s[j])); - luaL_loadbuffer(lua,code,sdslen(code),"@enable_strict_lua"); - lua_pcall(lua,0,0,0); - sdsfree(code); +static int luaProtectedTableError(lua_State *lua) { + int argc = lua_gettop(lua); + if (argc != 2) { + serverLog(LL_WARNING, "malicious code trying to call luaProtectedTableError with wrong arguments"); + luaL_error(lua, "Wrong number of arguments to luaProtectedTableError"); + } + if (!lua_isstring(lua, -1) && !lua_isnumber(lua, -1)) { + luaL_error(lua, "Second argument to luaProtectedTableError must be a string or number"); + } + const char* variable_name = lua_tostring(lua, -1); + luaL_error(lua, "Script attempted to access nonexistent global variable '%s'", variable_name); + return 0; } -/* Create a global protection function and put it to registry. - * This need to be called once in the lua_State lifetime. - * After called it is possible to use luaSetGlobalProtection - * to set global protection on a give table. - * - * The function assumes the Lua stack have a least enough - * space to push 2 element, its up to the caller to verify - * this before calling this function. - * - * Notice, the difference between this and luaEnableGlobalsProtection - * is that luaEnableGlobalsProtection is enabling global protection - * on the current Lua globals. This registering a global protection - * function that later can be applied on any table. */ -void luaRegisterGlobalProtectionFunction(lua_State *lua) { - lua_pushstring(lua, REGISTRY_SET_GLOBALS_PROTECTION_NAME); - char *global_protection_func = "local dbg = debug\n" - "local globals_protection = function (t)\n" - " local mt = {}\n" - " setmetatable(t, mt)\n" - " mt.__newindex = function (t, n, v)\n" - " if dbg.getinfo(2) then\n" - " local w = dbg.getinfo(2, \"S\").what\n" - " if w ~= \"C\" then\n" - " error(\"Script attempted to create global variable '\"..tostring(n)..\"'\", 2)\n" - " end" - " end" - " rawset(t, n, v)\n" - " end\n" - " mt.__index = function (t, n)\n" - " if dbg.getinfo(2) and dbg.getinfo(2, \"S\").what ~= \"C\" then\n" - " error(\"Script attempted to access nonexistent global variable '\"..tostring(n)..\"'\", 2)\n" - " end\n" - " return rawget(t, n)\n" - " end\n" - "end\n" - "return globals_protection"; - int res = luaL_loadbuffer(lua, global_protection_func, strlen(global_protection_func), "@global_protection_def"); - serverAssert(res == 0); - res = lua_pcall(lua,0,1,0); - serverAssert(res == 0); - lua_settable(lua, LUA_REGISTRYINDEX); -} - -/* Set global protection on a given table. - * The table need to be located on the top of the lua stack. +/* Set global protection on table on the top of the stack. * After called, it will no longer be possible to set - * new items on the table. The function is not removing - * the table from the top of the stack! + * new items on the table. Any attempt to access a protected + * table for write will result in an error. Any attempt to + * get a none existing element from a locked table will result + * in an error. * * The function assumes the Lua stack have a least enough * space to push 2 element, its up to the caller to verify * this before calling this function. */ void luaSetGlobalProtection(lua_State *lua) { - lua_pushstring(lua, REGISTRY_SET_GLOBALS_PROTECTION_NAME); - lua_gettable(lua, LUA_REGISTRYINDEX); - lua_pushvalue(lua, -2); - int res = lua_pcall(lua, 1, 0, 0); - serverAssert(res == 0); + lua_newtable(lua); /* push metatable */ + lua_pushcfunction(lua, luaProtectedTableError); /* push get error handler */ + lua_setfield(lua, -2, "__index"); + lua_enablereadonlytable(lua, -1, 1); /* protect the metatable */ + + lua_setmetatable(lua, -2); + + lua_enablereadonlytable(lua, -1, 1); } void luaRegisterVersion(lua_State* lua) { @@ -1504,9 +1436,19 @@ void luaCallFunction(scriptRunCtx* run_ctx, lua_State *lua, robj** keys, size_t * EVAL received. */ luaCreateArray(lua,keys,nkeys); /* On eval, keys and arguments are globals. */ - if (run_ctx->flags & SCRIPT_EVAL_MODE) lua_setglobal(lua,"KEYS"); + if (run_ctx->flags & SCRIPT_EVAL_MODE){ + /* open global protection to set KEYS */ + lua_enablereadonlytable(lua, LUA_GLOBALSINDEX, 0); + lua_setglobal(lua,"KEYS"); + lua_enablereadonlytable(lua, LUA_GLOBALSINDEX, 1); + } luaCreateArray(lua,args,nargs); - if (run_ctx->flags & SCRIPT_EVAL_MODE) lua_setglobal(lua,"ARGV"); + if (run_ctx->flags & SCRIPT_EVAL_MODE){ + /* open global protection to set ARGV */ + lua_enablereadonlytable(lua, LUA_GLOBALSINDEX, 0); + lua_setglobal(lua,"ARGV"); + lua_enablereadonlytable(lua, LUA_GLOBALSINDEX, 1); + } /* At this point whether this script was never seen before or if it was * already defined, we can call it. diff --git a/src/script_lua.h b/src/script_lua.h index f39c01744..e6ca4f730 100644 --- a/src/script_lua.h +++ b/src/script_lua.h @@ -67,7 +67,6 @@ typedef struct errorInfo { void luaRegisterRedisAPI(lua_State* lua); sds luaGetStringSds(lua_State *lua, int index); -void luaEnableGlobalsProtection(lua_State *lua); void luaRegisterGlobalProtectionFunction(lua_State *lua); void luaSetGlobalProtection(lua_State *lua); void luaRegisterLogFunction(lua_State* lua); diff --git a/tests/unit/functions.tcl b/tests/unit/functions.tcl index 7b781c588..62c070f90 100644 --- a/tests/unit/functions.tcl +++ b/tests/unit/functions.tcl @@ -624,16 +624,16 @@ start_server {tags {"scripting"}} { } } e set _ $e - } {*attempt to call field 'call' (a nil value)*} + } {*attempted to access nonexistent global variable 'call'*} - test {LIBRARIES - redis.call from function load} { + test {LIBRARIES - redis.setresp from function load} { catch { r function load replace {#!lua name=lib2 return redis.setresp(3) } } e set _ $e - } {*attempt to call field 'setresp' (a nil value)*} + } {*attempted to access nonexistent global variable 'setresp'*} test {LIBRARIES - redis.set_repl from function load} { catch { @@ -642,7 +642,7 @@ start_server {tags {"scripting"}} { } } e set _ $e - } {*attempt to call field 'set_repl' (a nil value)*} + } {*attempted to access nonexistent global variable 'set_repl'*} test {LIBRARIES - malicious access test} { # the 'library' API is not exposed inside a @@ -669,37 +669,18 @@ start_server {tags {"scripting"}} { end) end) } - assert_equal {OK} [r fcall f1 0] + catch {[r fcall f1 0]} e + assert_match {*Attempt to modify a readonly table*} $e catch {[r function load {#!lua name=lib2 redis.math.random() }]} e - assert_match {*can only be called inside a script invocation*} $e - - catch {[r function load {#!lua name=lib2 - redis.math.randomseed() - }]} e - assert_match {*can only be called inside a script invocation*} $e + assert_match {*Script attempted to access nonexistent global variable 'math'*} $e catch {[r function load {#!lua name=lib2 redis.redis.call('ping') }]} e - assert_match {*can only be called inside a script invocation*} $e - - catch {[r function load {#!lua name=lib2 - redis.redis.pcall('ping') - }]} e - assert_match {*can only be called inside a script invocation*} $e - - catch {[r function load {#!lua name=lib2 - redis.redis.setresp(3) - }]} e - assert_match {*can only be called inside a script invocation*} $e - - catch {[r function load {#!lua name=lib2 - redis.redis.set_repl(redis.redis.REPL_NONE) - }]} e - assert_match {*can only be called inside a script invocation*} $e + assert_match {*Script attempted to access nonexistent global variable 'redis'*} $e catch {[r fcall f2 0]} e assert_match {*can only be called on FUNCTION LOAD command*} $e @@ -756,7 +737,7 @@ start_server {tags {"scripting"}} { } } e set _ $e - } {*attempted to create global variable 'a'*} + } {*Attempt to modify a readonly table*} test {LIBRARIES - named arguments} { r function load {#!lua name=lib @@ -1198,4 +1179,32 @@ start_server {tags {"scripting"}} { redis.register_function('foo', function() return 1 end) } } {foo} + + test {FUNCTION - trick global protection 1} { + r FUNCTION FLUSH + + r FUNCTION load {#!lua name=test1 + redis.register_function('f1', function() + mt = getmetatable(_G) + original_globals = mt.__index + original_globals['redis'] = function() return 1 end + end) + } + + catch {[r fcall f1 0]} e + set _ $e + } {*Attempt to modify a readonly table*} + + test {FUNCTION - test getmetatable on script load} { + r FUNCTION FLUSH + + catch { + r FUNCTION load {#!lua name=test1 + mt = getmetatable(_G) + } + } e + + set _ $e + } {*Script attempted to access nonexistent global variable 'getmetatable'*} + } diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl index a5a253325..9e2a4c4b2 100644 --- a/tests/unit/scripting.tcl +++ b/tests/unit/scripting.tcl @@ -452,7 +452,7 @@ start_server {tags {"scripting"}} { test {Globals protection setting an undeclared global*} { catch {run_script {a=10} 0} e set e - } {ERR *attempted to create global*} + } {ERR *Attempt to modify a readonly table*} test {Test an example script DECR_IF_GT} { set decr_if_gt { @@ -741,6 +741,43 @@ start_server {tags {"scripting"}} { return loadstring(string.dump(function() return 1 end))() } 0} } + + test "Try trick global protection 1" { + catch { + run_script { + setmetatable(_G, {}) + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} + + test "Try trick global protection 2" { + catch { + run_script { + local g = getmetatable(_G) + g.__index = {} + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} + + test "Try trick global protection 3" { + catch { + run_script { + redis = function() return 1 end + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} + + test "Try trick global protection 4" { + catch { + run_script { + _G = {} + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} } # Start a new server since the last test in this stanza will kill the