Protect globals of both evals scripts and functions.

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

On functions its more complecated, we want to be able to switch globals between
load run and function run. To achieve this, we create a new empty table that
acts as the globals table for function, we control the actual globals using metatable
manipulation. Notice that even if the user gets a pointer to the original tables, all
the tables are set to be readonly (using `lua_enablereadonlytable` Lua API) so he can
not change them. The following inlustration better explain the solution:

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

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

Why this solution is needed and its not enough to simply switch globals?
When we run in the context of function load and create our functions, our function gets
the current globals that was set when they were created. Replacing the globals after
the creation will not effect them. This is why this trick it mandatory.

src/eval.c

@@ -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
+    /* 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. */
+    luaSetGlobalProtection(lua);
-    luaEnableGlobalsProtection(lua);
+    lua_pop(lua, 1);
 
     lctx.lua = lua;
 }

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,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,

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
+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) {
+    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++]="    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 global protection on table on the top of the stack.
- * 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
+ * new items on the table. Any attempt to access a protected
- * the table from the top of the stack!
+ * 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_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_enablereadonlytable(lua, -1, 1); /* protect the metatable */
-    serverAssert(res == 0);
+
+    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.

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

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,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