Fix potential security issue(s), documentation on minetest.deserialize() (#9369)

Also adds an unittest

diff --git a/builtin/common/serialize.lua b/builtin/common/serialize.lua
index cf00107c2e17862550fadaa5b7dc623f146b7afc..163aa67addb3552abada9479982bd0df581d8d7a 100644 (file)
--- a/builtin/common/serialize.lua
+++ b/builtin/common/serialize.lua
@@ -177,13 +177,16 @@ end
 
 -- Deserialization
 
-local env = {
-       loadstring = loadstring,
-}
+local function safe_loadstring(...)
+       local func, err = loadstring(...)
+       if func then
+               setfenv(func, {})
+               return func
+       end
+       return nil, err
+end
 
-local safe_env = {
-       loadstring = function() end,
-}
+local function dummy_func() end
 
 function core.deserialize(str, safe)
        if type(str) ~= "string" then
@@ -195,7 +198,10 @@ function core.deserialize(str, safe)
        end
        local f, err = loadstring(str)
        if not f then return nil, err end
-       setfenv(f, safe and safe_env or env)
+
+       -- The environment is recreated every time so deseralized code cannot
+       -- pollute it with permanent references.
+       setfenv(f, {loadstring = safe and dummy_func or safe_loadstring})
 
        local good, data = pcall(f)
        if good then

diff --git a/builtin/common/tests/serialize_spec.lua b/builtin/common/tests/serialize_spec.lua
index 321d2766aaf85cfe7056cc73815803255a30961d..c41b0a3728e1bede9afcaf5aa4a564d13f90d0cc 100644 (file)
--- a/builtin/common/tests/serialize_spec.lua
+++ b/builtin/common/tests/serialize_spec.lua
@@ -1,6 +1,6 @@
 _G.core = {}
 
-_G.setfenv = function() end
+_G.setfenv = require 'busted.compatibility'.setfenv
 
 dofile("builtin/common/serialize.lua")
 
@@ -25,4 +25,20 @@ describe("serialize", function()
                local test_out = core.deserialize(core.serialize(test_in))
                assert.same(test_in, test_out)
        end)
+
+       it("strips functions in safe mode", function()
+               local test_in = {
+                       func = function(a, b)
+                               error("test")
+                       end,
+                       foo = "bar"
+               }
+
+               local str = core.serialize(test_in)
+               assert.not_nil(str:find("loadstring"))
+
+               local test_out = core.deserialize(str, true)
+               assert.is_nil(test_out.func)
+               assert.equals(test_out.foo, "bar")
+       end)
 end)

diff --git a/doc/lua_api.txt b/doc/lua_api.txt
index 10ede51685269114dd9ba6b926531276b774aff6..acf2f77c15c1b91bbf84516f7c7b040c7d3e7f81 100644 (file)
--- a/doc/lua_api.txt
+++ b/doc/lua_api.txt
@@ -5275,10 +5275,16 @@ Misc.
     * Convert a table containing tables, strings, numbers, booleans and `nil`s
       into string form readable by `minetest.deserialize`
     * Example: `serialize({foo='bar'})`, returns `'return { ["foo"] = "bar" }'`
-* `minetest.deserialize(string)`: returns a table
-    * Convert a string returned by `minetest.deserialize` into a table
+* `minetest.deserialize(string[, safe])`: returns a table
+    * Convert a string returned by `minetest.serialize` into a table
     * `string` is loaded in an empty sandbox environment.
-    * Will load functions, but they cannot access the global environment.
+    * Will load functions if safe is false or omitted. Although these functions
+      cannot directly access the global environment, they could bypass this
+      restriction with maliciously crafted Lua bytecode if mod security is
+      disabled.
+    * This function should not be used on untrusted data, regardless of the
+     value of `safe`. It is fine to serialize then deserialize user-provided
+     data, but directly providing user input to deserialize is always unsafe.
     * Example: `deserialize('return { ["foo"] = "bar" }')`,
       returns `{foo='bar'}`
     * Example: `deserialize('print("foo")')`, returns `nil`