Merge pull request #10651 from oranagra/meir_lua_readonly_tables

# Lua readonly tables
The PR adds support for readonly tables on Lua to prevent security vulnerabilities:
* (CVE-2022-24736) An attacker attempting to load a specially crafted Lua script
  can cause NULL pointer dereference which will result with a crash of the
  redis-server process. This issue affects all versions of Redis.
* (CVE-2022-24735) By exploiting weaknesses in the Lua script execution
  environment, an attacker with access to Redis can inject Lua code that will
  execute with the (potentially higher) privileges of another Redis user.

The PR is spitted into 4 commits.

### Change Lua to support readonly tables

This PR modifies the Lua interpreter code to support a new flag on tables. The new flag indicating that the table is readonly and any attempt to perform any writes on such a table will result in an error. The new feature can be turned off and on using the new `lua_enablereadonlytable` Lua API. The new API can be used **only** from C code. Changes to support this feature was taken from https://luau-lang.org/

### Change eval script to set user code on Lua registry

Today, Redis wrap the user Lua code with a Lua function. For example, assuming the user code is:

```
return redis.call('ping')
```

The actual code that would have sent to the Lua interpreter was:

```
f_b3a02c833904802db9c34a3cf1292eee3246df3c() return redis.call('ping') end
```

The warped code would have been saved on the global dictionary with the following name: `f_<script sha>` (in our example `f_b3a02c833904802db9c34a3cf1292eee3246df3c`). This approach allows one user to easily override the implementation of another user code, example:

```
f_b3a02c833904802db9c34a3cf1292eee3246df3c = function() return 'hacked' end
```

Running the above code will cause `evalsha b3a02c833904802db9c34a3cf1292eee3246df3c 0` to return `hacked` although it should have returned `pong`. Another disadvantage is that Redis basically runs code on the loading (compiling) phase without been aware of it. User can do code injection like this:

```
return 1 end <run code on compling phase> function() return 1
```

The warped code will look like this and the entire `<run code on compiling phase>` block will run outside of eval or evalsha context:

```
f_<sha>() return 1 end <run code on compling phase> function() return 1 end
```

The commits puts the user code on a special Lua table called the registry. This table is not accessible to the user so it can not be manipulated by him. Also there is no longer a need to warp the user code so there is no risk in code injection which will cause running code in the wrong context.

### Use `lua_enablereadonlytable` to protect global tables on eval and function

The commit uses the new `lua_enablereadonlytable` Lua API to protect the global tables of both evals scripts and functions. For eval scripts, the implementation is easy, We simply call `lua_enablereadonlytable` on the global table to turn it into a readonly table.

On functions its more complected, 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 manipulations. 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 better explains the solution:

```
Global table {} <- global table metatable {.__index = __real_globals__}
```

The `__real_globals__` is depends on the run context (function load or function call).

Why is this solution 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.

### Protect the rest of the global API and add an allowed list to the provided API

The allowed list is done by setting a metatable on the global table before initialising any library. The metatable set the `__newindex` field to a function that check the allowed list before adding the field to the table. Fields which is not on the
allowed list are simply ignored.

After initialisation phase is done we protect the global table and each table that might be reachable from the global table. For each table we also protect the table metatable if exists.

### Performance

Performance tests was done on a private computer and its only purpose is to show that this fix is not causing any performance regression.

case 1: `return redis.call('ping')`
case 2: `for i=1,10000000 do redis.call('ping') end`

|                             | Unstable eval | Unstable function | lua_readonly_tables eval | lua_readonly_tables function |
|-----------------------------|---------------|-------------------|--------------------------|------------------------------|
| case1 ops/sec               | 235904.70     | 236406.62         | 232180.16               | 230574.14                   |
| case1 avg latency ms        | 0.175         | 0.164             | 0.178                    | 0.149                        |
| case2 total time in seconds | 3.373         | 3.444s            | 3.268                   | 3.278                        |

### Breaking changes

* `print` function was removed from Lua because it can potentially cause the Redis processes to get stuck (if no one reads from stdout). Users should use redis.log. An alternative is to override the `print` implementation and print the message to the log file.

All the work by @MeirShpilraien, i'm just publishing it.

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;
+}
+

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;

deps/lua/src/lobject.h

@@ -337,7 +337,8 @@ typedef struct Node {
 
 typedef struct Table {
   CommonHeader;
-  lu_byte flags;  /* 1<<p means tagmethod(p) is not present */ 
+  lu_byte flags;  /* 1<<p means tagmethod(p) is not present */
+  int readonly;
   lu_byte lsizenode;  /* log2 of size of `node' array */
   struct Table *metatable;
   TValue *array;  /* array part */

deps/lua/src/ltable.c

@@ -364,6 +364,7 @@ Table *luaH_new (lua_State *L, int narray, int nhash) {
   t->array = NULL;
   t->sizearray = 0;
   t->lsizenode = 0;
+  t->readonly = 0;
   t->node = cast(Node *, dummynode);
   setarrayvector(L, t, narray);
   setnodevector(L, t, nhash);

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);
+
 /* }====================================================================== */
 
 

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? */

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 a<b\n"
-                                "end\n";
-        luaL_loadbuffer(lua,compare_func,strlen(compare_func),"@cmp_func_def");
-        lua_pcall(lua,0,0,0);
-    }
-
     /* Add a helper function we use for pcall error reporting.
      * Note that when the error is in the C function we want to report the
      * information about the caller, that's what makes sense from the point
      * of view of the user debugging a script. */
     {
         char *errh_func =       "local dbg = debug\n"
+                                "debug = nil\n"
                                 "function __redis__err__handler(err)\n"
                                 "  local i = dbg.getinfo(2,'nSl')\n"
                                 "  if i and i.what == 'C' then\n"
@@ -266,10 +255,12 @@ void scriptingInit(int setup) {
         lctx.lua_client->flags |= CLIENT_DENY_BLOCKING;
     }
 
-    /* Lua beginners often don't use "local", this is likely to introduce
+    /* Lock the global table from any changes */
-     * subtle bugs in their code. To prevent problems we protect accesses
+    lua_pushvalue(lua, LUA_GLOBALSINDEX);
-     * to global variables. */
+    luaSetErrorMetatable(lua);
-    luaEnableGlobalsProtection(lua, 1);
+    /* 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);
+    if (luaL_loadbuffer(lctx.lua,(char*)body->ptr + shebang_len,sdslen(body->ptr) - shebang_len,"@user_script")) {
-    funcdef = sdscatlen(funcdef,"\nend",4);
-
-    if (luaL_loadbuffer(lctx.lua,funcdef,sdslen(funcdef),"@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)) {
+    serverAssert(lua_isfunction(lctx.lua, -1));
-        if (c != NULL) {
+
-            addReplyErrorFormat(c,"Error running script (new function): %s",
+    lua_setfield(lctx.lua, LUA_REGISTRYINDEX, funcname);
-                lua_tostring(lctx.lua,-1));
-        }
-        lua_pop(lctx.lua,1);
-        return NULL;
-    }
 
     /* 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));
     }
 

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.
+    /* set load library globals */
-     * We will create a new fresh Lua table and use
+    lua_getmetatable(lua, LUA_GLOBALSINDEX);
-     * lua_setfenv to set the table as the library globals
+    lua_enablereadonlytable(lua, -1, 0); /* disable global protection */
-     * (https://www.lua.org/manual/5.1/manual.html#lua_setfenv)
+    lua_getfield(lua, LUA_REGISTRYINDEX, LIBRARY_API_NAME);
-     *
+    lua_setfield(lua, -2, "__index");
-     * At first, populate this new table with only the 'library' API
+    lua_enablereadonlytable(lua, LUA_GLOBALSINDEX, 1); /* enable global protection */
-     * to make sure only 'library' API is available at start. After the
+    lua_pop(lua, 1); /* pop the metatable */
-     * 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);
 
     /* 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 */
+        lua_pop(lua, 1); /* pops the error */
-        return C_ERR;
+        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_pop(lua, 1); /* pops the error */
-        lua_sethook(lua,NULL,0,0); /* Disable hook */
-        luaSaveOnRegistry(lua, REGISTRY_LOAD_CTX_NAME, NULL);
         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);
-
+    return ret;
-    /* 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;
 }
 
 /*
@@ -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); /* load library globals */
-    lua_newtable(lua_engine_ctx->lua);
+    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,

src/script_lua.c

@@ -41,6 +41,97 @@
 #include <ctype.h>
 #include <math.h>
 
+/* 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
+static int luaProtectedTableError(lua_State *lua) {
- * the creation of globals accidentally.
+    int argc = lua_gettop(lua);
- *
+    if (argc != 2) {
- * It should be the last to be called in the scripting engine initialization
+        serverLog(LL_WARNING, "malicious code trying to call luaProtectedTableError with wrong arguments");
- * sequence, because it may interact with creation of globals.
+        luaL_error(lua, "Wrong number of arguments to luaProtectedTableError");
- *
+    }
- * On Legacy Lua (eval) we need to check 'w ~= \"main\"' otherwise we will not be able
+    if (!lua_isstring(lua, -1) && !lua_isnumber(lua, -1)) {
- * to create the global 'function <sha> ()' variable. On Functions Lua engine we do not use
+        luaL_error(lua, "Second argument to luaProtectedTableError must be a string or number");
- * this trick so it's not needed. */
+    }
-void luaEnableGlobalsProtection(lua_State *lua, int is_eval) {
+    const char *variable_name = lua_tostring(lua, -1);
-    char *s[32];
+    luaL_error(lua, "Script attempted to access nonexistent global variable '%s'", variable_name);
-    sds code = sdsempty();
+    return 0;
-    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);
 }
 
-/* Create a global protection function and put it to registry.
+/* Set a special metatable on the table on the top of the stack.
- * This need to be called once in the lua_State lifetime.
+ * The metatable will raise an error if the user tries to fetch
- * After called it is possible to use luaSetGlobalProtection
+ * an un-existing value.
- * 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!
  *
  * 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) {
+void luaSetErrorMetatable(lua_State *lua) {
-    lua_pushstring(lua, REGISTRY_SET_GLOBALS_PROTECTION_NAME);
+    lua_newtable(lua); /* push metatable */
-    lua_gettable(lua, LUA_REGISTRYINDEX);
+    lua_pushcfunction(lua, luaProtectedTableError); /* push get error handler */
-    lua_pushvalue(lua, -2);
+    lua_setfield(lua, -2, "__index");
-    int res = lua_pcall(lua, 1, 0, 0);
+    lua_setmetatable(lua, -2);
-    serverAssert(res == 0);
+}
+
+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.

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);

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
+        assert_match {*Script attempted to access nonexistent global variable 'math'*} $e
-
-        catch {[r function load {#!lua name=lib2
-            redis.math.randomseed()
-        }]} e
-        assert_match {*can only be called inside a script invocation*} $e
 
         catch {[r function load {#!lua name=lib2
             redis.redis.call('ping')
         }]} 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 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
 
         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'*}
+
 }

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