diff options
| -rw-r--r-- | src/eval.c | 8 | ||||
| -rw-r--r-- | src/function_lua.c | 113 | ||||
| -rw-r--r-- | src/script_lua.c | 132 | ||||
| -rw-r--r-- | src/script_lua.h | 1 | ||||
| -rw-r--r-- | tests/unit/functions.tcl | 65 | ||||
| -rw-r--r-- | tests/unit/scripting.tcl | 39 |
6 files changed, 165 insertions, 193 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; } - 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); + ret = C_OK; - lua_pop(lua, 1); /* pops the global table */ +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 */ - return C_OK; + lua_sethook(lua,NULL,0,0); /* Disable hook */ + luaSaveOnRegistry(lua, REGISTRY_LOAD_CTX_NAME, NULL); + 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 <sha> ()' 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); -} - -/* 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); +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; } -/* 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 |