Protect any table which is reachable from globals and added globals white list.

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

After initialization 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.

deps/lua/src/lapi.c

@@ -1099,3 +1099,11 @@ LUA_API void lua_enablereadonlytable (lua_State *L, int objindex, int enabled) {
   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/lua.h

@@ -359,6 +359,7 @@ struct lua_Debug {
 };
 
 LUA_API void lua_enablereadonlytable (lua_State *L, int index, int enabled);
+LUA_API int lua_isreadonlytable (lua_State *L, int index);
 
 /* }====================================================================== */
 

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"
@@ -268,7 +257,9 @@ void scriptingInit(int setup) {
 
     /* Lock the global table from any changes */
     lua_pushvalue(lua, LUA_GLOBALSINDEX);
-    luaSetGlobalProtection(lua);
+    luaSetErrorMetatable(lua);
+    /* Recursively lock all tables that can be reached from the global table */
+    luaSetTableProtectionRecursively(lua);
     lua_pop(lua, 1);
 
     lctx.lua = lua;

src/function_lua.c

@@ -436,16 +436,17 @@ int luaEngineInitEngine() {
     luaRegisterLogFunction(lua_engine_ctx->lua);
     luaRegisterVersion(lua_engine_ctx->lua);
 
-    luaSetGlobalProtection(lua_engine_ctx->lua); /* protect redis */
-
+    luaSetErrorMetatable(lua_engine_ctx->lua);
     lua_setfield(lua_engine_ctx->lua, -2, REDIS_API_NAME);
 
-    luaSetGlobalProtection(lua_engine_ctx->lua); /* protect load library globals */
+    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"
@@ -466,7 +467,8 @@ int luaEngineInitEngine() {
     lua_settable(lua_engine_ctx->lua, LUA_REGISTRYINDEX);
 
     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 */

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) {
@@ -1144,30 +1226,111 @@ static int luaProtectedTableError(lua_State *lua) {
     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);
+    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 table on the top of the stack.
- * After called, it will no longer be possible to set
- * 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.
+/* 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) {
+void luaSetErrorMetatable(lua_State *lua) {
     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);
+}
 
+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) {
@@ -1204,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");

src/script_lua.h

@@ -68,7 +68,9 @@ typedef struct errorInfo {
 void luaRegisterRedisAPI(lua_State* lua);
 sds luaGetStringSds(lua_State *lua, int index);
 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/scripting.tcl

@@ -778,6 +778,69 @@ start_server {tags {"scripting"}} {
         } 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