File bsc1198952-4.patch of Package redis.28077
From 11b602fbf8f9cdf8fc741c24625ab6287ab998a9 Mon Sep 17 00:00:00 2001
From: meir <meir@redis.com>
Date: Tue, 22 Mar 2022 12:15:57 +0200
Subject: [PATCH] Protect any table which is reachable from globals and added
globals allow list.
The allow 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 allow list before adding the field to the table. Fields which is not on the
allow 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 | 8 ++
deps/lua/src/lua.h | 1 +
src/scripting.c | 211 +++++++++++++++++++++++++++++++++++----
tests/unit/scripting.tcl | 63 ++++++++++++
4 files changed, 262 insertions(+), 21 deletions(-)
diff --git a/deps/lua/src/lapi.c b/deps/lua/src/lapi.c
index 1a9455629450..e8ef41ea248b 100644
--- a/deps/lua/src/lapi.c
+++ b/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;
+}
+
diff --git a/deps/lua/src/lua.h b/deps/lua/src/lua.h
index e478d14c02e3..223bd99dc398 100644
--- a/deps/lua/src/lua.h
+++ b/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 objindex);
/* }====================================================================== */
diff --git a/src/scripting.c b/src/scripting.c
index 94a600997d16..b43c0088058f 100644
--- a/src/scripting.c
+++ b/src/scripting.c
@@ -39,6 +39,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,
+};
+
char *redisProtocolToLuaType_Int(lua_State *lua, char *reply);
char *redisProtocolToLuaType_Bulk(lua_State *lua, char *reply);
char *redisProtocolToLuaType_Status(lua_State *lua, char *reply);
@@ -1077,15 +1168,6 @@ void luaLoadLibraries(lua_State *lua) {
#endif
}
-/* Remove a functions that we don't want to expose to the Redis scripting
- * environment. */
-void luaRemoveUnsupportedFunctions(lua_State *lua) {
- lua_pushnil(lua);
- lua_setglobal(lua,"loadfile");
- lua_pushnil(lua);
- lua_setglobal(lua,"dofile");
-}
-
static int luaProtectedTableError(lua_State *lua) {
int argc = lua_gettop(lua);
if (argc != 2) {
@@ -1095,30 +1177,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);
+}
+
+/* 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 */
+ }
+}
+
+static int luaNewIndexAllowList(lua_State *lua) {
+ int argc = lua_gettop(lua);
+ if (argc != 3) {
+ serverLog(LL_WARNING, "malicious code trying to call luaNewIndexAllowList 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);
}
/* Initialize the scripting environment.
@@ -1142,8 +1305,11 @@ void scriptingInit(int setup) {
ldbInit();
}
+ lua_pushvalue(lua, LUA_GLOBALSINDEX);
+ luaSetAllowListProtection(lua);
+ lua_pop(lua, 1);
+
luaLoadLibraries(lua);
- luaRemoveUnsupportedFunctions(lua);
/* Initialize a dictionary we use to map SHAs to scripts.
* This is useful for replication, as we need to replicate EVALSHA
@@ -1277,6 +1443,7 @@ void scriptingInit(int setup) {
* 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"
@@ -1306,7 +1473,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);
server.lua = lua;
diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl
index cc0344283c5d..83ced870713e 100644
--- a/tests/unit/scripting.tcl
+++ b/tests/unit/scripting.tcl
@@ -632,6 +632,69 @@ start_server {tags {"scripting"}} {
} e
set _ $e
} {*Attempt to modify a readonly table*}
+
+ test "Try trick readonly table on redis table" {
+ catch {
+ r eval {
+ 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 {
+ r eval {
+ 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 {
+ r eval {
+ 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 {
+ r eval {
+ bit.lshift = function() return 1 end
+ } 0
+ } e
+ set _ $e
+ } {*Attempt to modify a readonly table*}
+
+ test "Test loadfile are not available" {
+ catch {
+ r eval {
+ loadfile('some file')
+ } 0
+ } e
+ set _ $e
+ } {*Script attempted to access nonexistent global variable 'loadfile'*}
+
+ test "Test dofile are not available" {
+ catch {
+ r eval {
+ dofile('some file')
+ } 0
+ } e
+ set _ $e
+ } {*Script attempted to access nonexistent global variable 'dofile'*}
+
+ test "Test print are not available" {
+ catch {
+ r eval {
+ 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
