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

Also adds an unittest
This commit is contained in:
sfan5 2020-03-05 22:03:04 +01:00 committed by GitHub
parent ef09e8a4d6
commit 8d6a0b917c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 39 additions and 11 deletions

@ -177,13 +177,16 @@ end
-- Deserialization -- Deserialization
local env = { local function safe_loadstring(...)
loadstring = loadstring, local func, err = loadstring(...)
} if func then
setfenv(func, {})
return func
end
return nil, err
end
local safe_env = { local function dummy_func() end
loadstring = function() end,
}
function core.deserialize(str, safe) function core.deserialize(str, safe)
if type(str) ~= "string" then if type(str) ~= "string" then
@ -195,7 +198,10 @@ function core.deserialize(str, safe)
end end
local f, err = loadstring(str) local f, err = loadstring(str)
if not f then return nil, err end 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) local good, data = pcall(f)
if good then if good then

@ -1,6 +1,6 @@
_G.core = {} _G.core = {}
_G.setfenv = function() end _G.setfenv = require 'busted.compatibility'.setfenv
dofile("builtin/common/serialize.lua") dofile("builtin/common/serialize.lua")
@ -25,4 +25,20 @@ describe("serialize", function()
local test_out = core.deserialize(core.serialize(test_in)) local test_out = core.deserialize(core.serialize(test_in))
assert.same(test_in, test_out) assert.same(test_in, test_out)
end) 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) end)

@ -5275,10 +5275,16 @@ Misc.
* Convert a table containing tables, strings, numbers, booleans and `nil`s * Convert a table containing tables, strings, numbers, booleans and `nil`s
into string form readable by `minetest.deserialize` into string form readable by `minetest.deserialize`
* Example: `serialize({foo='bar'})`, returns `'return { ["foo"] = "bar" }'` * Example: `serialize({foo='bar'})`, returns `'return { ["foo"] = "bar" }'`
* `minetest.deserialize(string)`: returns a table * `minetest.deserialize(string[, safe])`: returns a table
* Convert a string returned by `minetest.deserialize` into a table * Convert a string returned by `minetest.serialize` into a table
* `string` is loaded in an empty sandbox environment. * `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" }')`, * Example: `deserialize('return { ["foo"] = "bar" }')`,
returns `{foo='bar'}` returns `{foo='bar'}`
* Example: `deserialize('print("foo")')`, returns `nil` * Example: `deserialize('print("foo")')`, returns `nil`