diff --git a/deps/lua/src/lapi.c b/deps/lua/src/lapi.c index 5d5145d2e..e8ef41ea2 100644 --- a/deps/lua/src/lapi.c +++ b/deps/lua/src/lapi.c @@ -674,6 +674,8 @@ LUA_API void lua_rawset (lua_State *L, int idx) { api_checknelems(L, 2); t = index2adr(L, idx); api_check(L, ttistable(t)); + if (hvalue(t)->readonly) + luaG_runerror(L, "Attempt to modify a readonly table"); setobj2t(L, luaH_set(L, hvalue(t), L->top-2), L->top-1); luaC_barriert(L, hvalue(t), L->top-1); L->top -= 2; @@ -687,6 +689,8 @@ LUA_API void lua_rawseti (lua_State *L, int idx, int n) { api_checknelems(L, 1); o = index2adr(L, idx); api_check(L, ttistable(o)); + if (hvalue(o)->readonly) + luaG_runerror(L, "Attempt to modify a readonly table"); setobj2t(L, luaH_setnum(L, hvalue(o), n), L->top-1); luaC_barriert(L, hvalue(o), L->top-1); L->top--; @@ -709,6 +713,8 @@ LUA_API int lua_setmetatable (lua_State *L, int objindex) { } switch (ttype(obj)) { case LUA_TTABLE: { + if (hvalue(obj)->readonly) + luaG_runerror(L, "Attempt to modify a readonly table"); hvalue(obj)->metatable = mt; if (mt) luaC_objbarriert(L, hvalue(obj), mt); @@ -1085,3 +1091,19 @@ LUA_API const char *lua_setupvalue (lua_State *L, int funcindex, int n) { return name; } +LUA_API void lua_enablereadonlytable (lua_State *L, int objindex, int enabled) { + const TValue* o = index2adr(L, objindex); + api_check(L, ttistable(o)); + Table* t = hvalue(o); + api_check(L, t != hvalue(registry(L))); + t->readonly = enabled; +} + +LUA_API int lua_isreadonlytable (lua_State *L, int objindex) { + const TValue* o = index2adr(L, objindex); + api_check(L, ttistable(o)); + Table* t = hvalue(o); + api_check(L, t != hvalue(registry(L))); + return t->readonly; +} + diff --git a/deps/lua/src/ldebug.c b/deps/lua/src/ldebug.c index 50ad3d380..4940e65a6 100644 --- a/deps/lua/src/ldebug.c +++ b/deps/lua/src/ldebug.c @@ -80,7 +80,6 @@ LUA_API int lua_gethookcount (lua_State *L) { return L->basehookcount; } - LUA_API int lua_getstack (lua_State *L, int level, lua_Debug *ar) { int status; CallInfo *ci; diff --git a/deps/lua/src/lobject.h b/deps/lua/src/lobject.h index f1e447ef3..10e46b178 100644 --- a/deps/lua/src/lobject.h +++ b/deps/lua/src/lobject.h @@ -337,7 +337,8 @@ typedef struct Node { typedef struct Table { CommonHeader; - lu_byte flags; /* 1<

array = NULL; t->sizearray = 0; t->lsizenode = 0; + t->readonly = 0; t->node = cast(Node *, dummynode); setarrayvector(L, t, narray); setnodevector(L, t, nhash); diff --git a/deps/lua/src/lua.h b/deps/lua/src/lua.h index a4b73e743..280ef2382 100644 --- a/deps/lua/src/lua.h +++ b/deps/lua/src/lua.h @@ -358,6 +358,9 @@ struct lua_Debug { int i_ci; /* active function */ }; +LUA_API void lua_enablereadonlytable (lua_State *L, int index, int enabled); +LUA_API int lua_isreadonlytable (lua_State *L, int index); + /* }====================================================================== */ diff --git a/deps/lua/src/lvm.c b/deps/lua/src/lvm.c index e0a0cd852..5cd313b5e 100644 --- a/deps/lua/src/lvm.c +++ b/deps/lua/src/lvm.c @@ -138,6 +138,8 @@ void luaV_settable (lua_State *L, const TValue *t, TValue *key, StkId val) { const TValue *tm; if (ttistable(t)) { /* `t' is a table? */ Table *h = hvalue(t); + if (h->readonly) + luaG_runerror(L, "Attempt to modify a readonly table"); TValue *oldval = luaH_set(L, h, key); /* do a primitive set */ if (!ttisnil(oldval) || /* result is no nil? */ (tm = fasttm(L, h->metatable, TM_NEWINDEX)) == NULL) { /* or no TM? */ diff --git a/src/eval.c b/src/eval.c index 4894b83d3..332aec9ce 100644 --- a/src/eval.c +++ b/src/eval.c @@ -218,24 +218,13 @@ void scriptingInit(int setup) { lua_setglobal(lua,"redis"); - /* Add a helper function that we use to sort the multi bulk output of non - * deterministic commands, when containing 'false' elements. */ - { - char *compare_func = "function __redis__compare_helper(a,b)\n" - " if a == false then a = '' end\n" - " if b == false then b = '' end\n" - " return aflags |= 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, 1); + /* Lock the global table from any changes */ + lua_pushvalue(lua, LUA_GLOBALSINDEX); + luaSetErrorMetatable(lua); + /* Recursively lock all tables that can be reached from the global table */ + luaSetTableProtectionRecursively(lua); + lua_pop(lua, 1); lctx.lua = lua; } @@ -378,35 +369,20 @@ sds luaCreateFunction(client *c, robj *body) { sdsfreesplitres(parts, numparts); } - /* Build the lua function to be loaded */ - sds funcdef = sdsempty(); - funcdef = sdscat(funcdef,"function "); - funcdef = sdscatlen(funcdef,funcname,42); - funcdef = sdscatlen(funcdef,"() ",3); /* Note that in case of a shebang line we skip it but keep the line feed to conserve the user's line numbers */ - funcdef = sdscatlen(funcdef,(char*)body->ptr + shebang_len,sdslen(body->ptr) - shebang_len); - funcdef = sdscatlen(funcdef,"\nend",4); - - if (luaL_loadbuffer(lctx.lua,funcdef,sdslen(funcdef),"@user_script")) { + if (luaL_loadbuffer(lctx.lua,(char*)body->ptr + shebang_len,sdslen(body->ptr) - shebang_len,"@user_script")) { if (c != NULL) { addReplyErrorFormat(c, "Error compiling script (new function): %s", lua_tostring(lctx.lua,-1)); } lua_pop(lctx.lua,1); - sdsfree(funcdef); return NULL; } - sdsfree(funcdef); - if (lua_pcall(lctx.lua,0,0,0)) { - if (c != NULL) { - addReplyErrorFormat(c,"Error running script (new function): %s", - lua_tostring(lctx.lua,-1)); - } - lua_pop(lctx.lua,1); - return NULL; - } + serverAssert(lua_isfunction(lctx.lua, -1)); + + lua_setfield(lctx.lua, LUA_REGISTRYINDEX, funcname); /* We also save a SHA1 -> Original script map in a dictionary * so that we can replicate / write in the AOF all the @@ -479,7 +455,7 @@ void evalGenericCommand(client *c, int evalsha) { lua_getglobal(lua, "__redis__err__handler"); /* Try to lookup the Lua function */ - lua_getglobal(lua, funcname); + lua_getfield(lua, LUA_REGISTRYINDEX, funcname); if (lua_isnil(lua,-1)) { lua_pop(lua,1); /* remove the nil from the stack */ /* Function not defined... let's define it if we have the @@ -497,7 +473,7 @@ void evalGenericCommand(client *c, int evalsha) { return; } /* Now the following is guaranteed to return non nil */ - lua_getglobal(lua, funcname); + lua_getfield(lua, LUA_REGISTRYINDEX, funcname); serverAssert(!lua_isnil(lua,-1)); } diff --git a/src/function_lua.c b/src/function_lua.c index 1e6363fd4..2e0250ea2 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,11 +436,17 @@ int luaEngineInitEngine() { luaRegisterLogFunction(lua_engine_ctx->lua); luaRegisterVersion(lua_engine_ctx->lua); - lua_settable(lua_engine_ctx->lua, LUA_REGISTRYINDEX); + luaSetErrorMetatable(lua_engine_ctx->lua); + lua_setfield(lua_engine_ctx->lua, -2, REDIS_API_NAME); + + luaSetErrorMetatable(lua_engine_ctx->lua); + luaSetTableProtectionRecursively(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); char *errh_func = "local dbg = debug\n" + "debug = nil\n" "local error_handler = function (err)\n" " local i = dbg.getinfo(2,'nSl')\n" " if i and i.what == 'C' then\n" @@ -492,17 +466,30 @@ 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); + luaSetErrorMetatable(lua_engine_ctx->lua); + luaSetTableProtectionRecursively(lua_engine_ctx->lua); /* protect globals */ 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 9a08a7e47..36868ec2b 100644 --- a/src/script_lua.c +++ b/src/script_lua.c @@ -41,6 +41,97 @@ #include #include +/* Globals that are added by the Lua libraries */ +static char *libraries_allow_list[] = { + "string", + "cjson", + "bit", + "cmsgpack", + "math", + "table", + "struct", + NULL, +}; + +/* Redis Lua API globals */ +static char *redis_api_allow_list[] = { + "redis", + "__redis__err__handler", /* error handler for eval, currently located on globals. + Should move to registry. */ + NULL, +}; + +/* Lua builtins */ +static char *lua_builtins_allow_list[] = { + "xpcall", + "tostring", + "getfenv", + "setmetatable", + "next", + "assert", + "tonumber", + "rawequal", + "collectgarbage", + "getmetatable", + "rawset", + "pcall", + "coroutine", + "type", + "_G", + "select", + "unpack", + "gcinfo", + "pairs", + "rawget", + "loadstring", + "ipairs", + "_VERSION", + "setfenv", + "load", + "error", + NULL, +}; + +/* Lua builtins which are not documented on the Lua documentation */ +static char *lua_builtins_not_documented_allow_list[] = { + "newproxy", + NULL, +}; + +/* Lua builtins which are allowed on initialization but will be removed right after */ +static char *lua_builtins_removed_after_initialization_allow_list[] = { + "debug", /* debug will be set to nil after the error handler will be created */ + NULL, +}; + +/* Those allow lists was created from the globals that was + * available to the user when the allow lists was first introduce. + * Because we do not want to break backward compatibility we keep + * all the globals. The allow lists will prevent us from accidentally + * creating unwanted globals in the future. + * + * Also notice that the allow list is only checked on start time, + * after that the global table is locked so not need to check anything.*/ +static char **allow_lists[] = { + libraries_allow_list, + redis_api_allow_list, + lua_builtins_allow_list, + lua_builtins_not_documented_allow_list, + lua_builtins_removed_after_initialization_allow_list, + NULL, +}; + +/* Deny list contains elements which we know we do not want to add to globals + * and there is no need to print a warning message form them. We will print a + * log message only if an element was added to the globals and the element is + * not on the allow list nor on the back list. */ +static char *deny_list[] = { + "dofile", + "loadfile", + "print", + NULL, +}; + static int redis_math_random (lua_State *L); static int redis_math_randomseed (lua_State *L); static void redisProtocolToLuaType_Int(void *ctx, long long val, const char *proto, size_t proto_len); @@ -1113,15 +1204,6 @@ static void luaLoadLibraries(lua_State *lua) { #endif } -/* Remove a functions that we don't want to expose to the Redis scripting - * environment. */ -static void luaRemoveUnsupportedFunctions(lua_State *lua) { - lua_pushnil(lua); - lua_setglobal(lua,"loadfile"); - lua_pushnil(lua); - lua_setglobal(lua,"dofile"); -} - /* Return sds of the string value located on stack at the given index. * Return NULL if the value is not a string. */ sds luaGetStringSds(lua_State *lua, int index) { @@ -1135,107 +1217,120 @@ 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, int is_eval) { - 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++]= is_eval ? " if w ~= \"main\" and w ~= \"C\" then\n" : " 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. - * 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! +/* Set a special metatable on the table on the top of the stack. + * The metatable will raise an error if the user tries to fetch + * an un-existing value. * * 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); +void luaSetErrorMetatable(lua_State *lua) { + lua_newtable(lua); /* push metatable */ + lua_pushcfunction(lua, luaProtectedTableError); /* push get error handler */ + lua_setfield(lua, -2, "__index"); + lua_setmetatable(lua, -2); +} + +static int luaNewIndexAllowList(lua_State *lua) { + int argc = lua_gettop(lua); + if (argc != 3) { + serverLog(LL_WARNING, "malicious code trying to call luaProtectedTableError with wrong arguments"); + luaL_error(lua, "Wrong number of arguments to luaNewIndexAllowList"); + } + if (!lua_istable(lua, -3)) { + luaL_error(lua, "first argument to luaNewIndexAllowList must be a table"); + } + if (!lua_isstring(lua, -2) && !lua_isnumber(lua, -2)) { + luaL_error(lua, "Second argument to luaNewIndexAllowList must be a string or number"); + } + const char *variable_name = lua_tostring(lua, -2); + /* check if the key is in our allow list */ + + char ***allow_l = allow_lists; + for (; *allow_l ; ++allow_l){ + char **c = *allow_l; + for (; *c ; ++c) { + if (strcmp(*c, variable_name) == 0) { + break; + } + } + if (*c) { + break; + } + } + if (!*allow_l) { + /* Search the value on the back list, if its there we know that it was removed + * on purpose and there is no need to print a warning. */ + char **c = deny_list; + for ( ; *c ; ++c) { + if (strcmp(*c, variable_name) == 0) { + break; + } + } + if (!*c) { + serverLog(LL_WARNING, "A key '%s' was added to Lua globals which is not on the globals allow list nor listed on the deny list.", variable_name); + } + } else { + lua_rawset(lua, -3); + } + return 0; +} + +/* Set a metatable with '__newindex' function that verify that + * the new index appears on our globals while list. + * + * The metatable is set on the table which located on the top + * of the stack. + */ +void luaSetAllowListProtection(lua_State *lua) { + lua_newtable(lua); /* push metatable */ + lua_pushcfunction(lua, luaNewIndexAllowList); /* push get error handler */ + lua_setfield(lua, -2, "__newindex"); + lua_setmetatable(lua, -2); +} + +/* Set the readonly flag on the table located on the top of the stack + * and recursively call this function on each table located on the original + * table. Also, recursively call this function on the metatables.*/ +void luaSetTableProtectionRecursively(lua_State *lua) { + /* This protect us from a loop in case we already visited the table + * For example, globals has '_G' key which is pointing back to globals. */ + if (lua_isreadonlytable(lua, -1)) { + return; + } + + /* protect the current table */ + lua_enablereadonlytable(lua, -1, 1); + + lua_checkstack(lua, 2); + lua_pushnil(lua); /* Use nil to start iteration. */ + while (lua_next(lua,-2)) { + /* Stack now: table, key, value */ + if (lua_istable(lua, -1)) { + luaSetTableProtectionRecursively(lua); + } + lua_pop(lua, 1); + } + + /* protect the metatable if exists */ + if (lua_getmetatable(lua, -1)) { + luaSetTableProtectionRecursively(lua); + lua_pop(lua, 1); /* pop the metatable */ + } } void luaRegisterVersion(lua_State* lua) { @@ -1272,8 +1367,11 @@ void luaRegisterLogFunction(lua_State* lua) { } void luaRegisterRedisAPI(lua_State* lua) { + lua_pushvalue(lua, LUA_GLOBALSINDEX); + luaSetAllowListProtection(lua); + lua_pop(lua, 1); + luaLoadLibraries(lua); - luaRemoveUnsupportedFunctions(lua); lua_pushcfunction(lua,luaRedisPcall); lua_setglobal(lua, "pcall"); @@ -1504,9 +1602,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 5a4533784..4c2b34804 100644 --- a/src/script_lua.h +++ b/src/script_lua.h @@ -67,9 +67,10 @@ typedef struct errorInfo { void luaRegisterRedisAPI(lua_State* lua); sds luaGetStringSds(lua_State *lua, int index); -void luaEnableGlobalsProtection(lua_State *lua, int is_eval); void luaRegisterGlobalProtectionFunction(lua_State *lua); -void luaSetGlobalProtection(lua_State *lua); +void luaSetErrorMetatable(lua_State *lua); +void luaSetAllowListProtection(lua_State *lua); +void luaSetTableProtectionRecursively(lua_State *lua); void luaRegisterLogFunction(lua_State* lua); void luaRegisterVersion(lua_State* lua); void luaPushErrorBuff(lua_State *lua, sds err_buff); 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..439a1e71a 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,106 @@ 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*} + + test "Try trick readonly table on redis table" { + catch { + run_script { + redis.call = function() return 1 end + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} + + test "Try trick readonly table on json table" { + catch { + run_script { + cjson.encode = function() return 1 end + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} + + test "Try trick readonly table on cmsgpack table" { + catch { + run_script { + cmsgpack.pack = function() return 1 end + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} + + test "Try trick readonly table on bit table" { + catch { + run_script { + bit.lshift = function() return 1 end + } 0 + } e + set _ $e + } {*Attempt to modify a readonly table*} + + test "Test loadfile are not available" { + catch { + run_script { + loadfile('some file') + } 0 + } e + set _ $e + } {*Script attempted to access nonexistent global variable 'loadfile'*} + + test "Test dofile are not available" { + catch { + run_script { + dofile('some file') + } 0 + } e + set _ $e + } {*Script attempted to access nonexistent global variable 'dofile'*} + + test "Test print are not available" { + catch { + run_script { + print('some data') + } 0 + } e + set _ $e + } {*Script attempted to access nonexistent global variable 'print'*} } # Start a new server since the last test in this stanza will kill the