Add mod security

Due to compatibility concerns, this is temporarily disabled.
minetest · May 16, 2015 · 3a8c788 · 3a8c788 · 4aiman · Jun 3, 2015
1 parent f264212
commit 3a8c788
Show file tree

Hide file tree

Showing 22 changed files with 809 additions and 80 deletions.
diff --git a/build/android/jni/Android.mk b/build/android/jni/Android.mk
@@ -262,6 +262,7 @@ LOCAL_SRC_FILES +=                                \
 		jni/src/script/common/c_converter.cpp     \
 		jni/src/script/common/c_internal.cpp      \
 		jni/src/script/common/c_types.cpp         \
+		jni/src/script/cpp_api/s_async.cpp        \
 		jni/src/script/cpp_api/s_base.cpp         \
 		jni/src/script/cpp_api/s_entity.cpp       \
 		jni/src/script/cpp_api/s_env.cpp          \
@@ -271,8 +272,8 @@ LOCAL_SRC_FILES +=                                \
 		jni/src/script/cpp_api/s_node.cpp         \
 		jni/src/script/cpp_api/s_nodemeta.cpp     \
 		jni/src/script/cpp_api/s_player.cpp       \
+		jni/src/script/cpp_api/s_security.cpp     \
 		jni/src/script/cpp_api/s_server.cpp       \
-		jni/src/script/cpp_api/s_async.cpp        \
 		jni/src/script/lua_api/l_base.cpp         \
 		jni/src/script/lua_api/l_craft.cpp        \
 		jni/src/script/lua_api/l_env.cpp          \

diff --git a/doc/lua_api.txt b/doc/lua_api.txt
@@ -1825,8 +1825,12 @@ Call these functions only at load time!
 
 ### Setting-related
 * `minetest.setting_set(name, value)`
+    * Setting names can't contain whitespace or any of `="{}#`.
+    * Setting values can't contain the sequence `\n"""`.
+    * Setting names starting with "secure." can't be set.
 * `minetest.setting_get(name)`: returns string or `nil`
 * `minetest.setting_setbool(name, value)`
+    * See documentation on `setting_set` for restrictions.
 * `minetest.setting_getbool(name)`: returns boolean or `nil`
 * `minetest.setting_get_pos(name)`: returns position or nil
 * `minetest.setting_save()`, returns `nil`, save all settings to config file

diff --git a/minetest.conf.example b/minetest.conf.example
@@ -569,3 +569,6 @@
 #mgv7_np_cave1 = 0, 12, (100, 100, 100), 52534, 4, 0.5, 2.0
 #mgv7_np_cave2 = 0, 12, (100, 100, 100), 10325, 4, 0.5, 2.0
 
+#    Prevent mods from doing insecure things like running shell commands.
+#secure.enable_security = false
+
diff --git a/src/defaultsettings.cpp b/src/defaultsettings.cpp
@@ -272,6 +272,7 @@ void set_default_settings(Settings *settings)
 	settings->setDefault("emergequeue_limit_diskonly", "32");
 	settings->setDefault("emergequeue_limit_generate", "32");
 	settings->setDefault("num_emerge_threads", "1");
+	settings->setDefault("secure.enable_security", "false");
 
 	// physics stuff
 	settings->setDefault("movement_acceleration_default", "3");

diff --git a/src/filesys.cpp b/src/filesys.cpp
@@ -662,6 +662,19 @@ std::string RemoveRelativePathComponents(std::string path)
 	return path.substr(0, pos);
 }
 
+std::string AbsolutePath(const std::string &path)
+{
+#ifdef _WIN32
+	char *abs_path = _fullpath(NULL, path.c_str(), MAX_PATH);
+#else
+	char *abs_path = realpath(path.c_str(), NULL);
+#endif
+	if (!abs_path) return "";
+	std::string abs_path_str(abs_path);
+	free(abs_path);
+	return abs_path_str;
+}
+
 const char *GetFilenameFromPath(const char *path)
 {
 	const char *filename = strrchr(path, DIR_DELIM_CHAR);

diff --git a/src/filesys.h b/src/filesys.h
@@ -103,13 +103,17 @@ std::string RemoveLastPathComponent(const std::string &path,
 // this does not resolve symlinks and check for existence of directories.
 std::string RemoveRelativePathComponents(std::string path);
 
-// Return the filename from a path or the entire path if no directory delimiter
-// is found.
+// Returns the absolute path for the passed path, with "." and ".." path
+// components and symlinks removed.  Returns "" on error.
+std::string AbsolutePath(const std::string &path);
+
+// Returns the filename from a path or the entire path if no directory
+// delimiter is found.
 const char *GetFilenameFromPath(const char *path);
 
 bool safeWriteToFile(const std::string &path, const std::string &content);
 
-}//fs
+} // namespace fs
 
 #endif
 
diff --git a/src/script/common/c_internal.cpp b/src/script/common/c_internal.cpp
@@ -63,10 +63,8 @@ int script_exception_wrapper(lua_State *L, lua_CFunction f)
 		return f(L);  // Call wrapped function and return result.
 	} catch (const char *s) {  // Catch and convert exceptions.
 		lua_pushstring(L, s);
-	} catch (std::exception& e) {
+	} catch (std::exception &e) {
 		lua_pushstring(L, e.what());
-	} catch (...) {
-		lua_pushliteral(L, "caught (...)");
 	}
 	return lua_error(L);  // Rethrow as a Lua error.
 }

diff --git a/src/script/cpp_api/CMakeLists.txt b/src/script/cpp_api/CMakeLists.txt
@@ -1,4 +1,5 @@
 set(common_SCRIPT_CPP_API_SRCS
+	${CMAKE_CURRENT_SOURCE_DIR}/s_async.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/s_base.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/s_entity.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/s_env.cpp
@@ -7,8 +8,8 @@ set(common_SCRIPT_CPP_API_SRCS
 	${CMAKE_CURRENT_SOURCE_DIR}/s_node.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/s_nodemeta.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/s_player.cpp
+	${CMAKE_CURRENT_SOURCE_DIR}/s_security.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/s_server.cpp
-	${CMAKE_CURRENT_SOURCE_DIR}/s_async.cpp
 	PARENT_SCOPE)
 
 set(client_SCRIPT_CPP_API_SRCS

diff --git a/src/script/cpp_api/s_base.cpp b/src/script/cpp_api/s_base.cpp
@@ -19,6 +19,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #include "cpp_api/s_base.h"
 #include "cpp_api/s_internal.h"
+#include "cpp_api/s_security.h"
 #include "lua_api/l_object.h"
 #include "serverobject.h"
 #include "debug.h"
@@ -45,18 +46,18 @@ class ModNameStorer
 private:
 	lua_State *L;
 public:
-	ModNameStorer(lua_State *L_, const std::string &modname):
+	ModNameStorer(lua_State *L_, const std::string &mod_name):
 		L(L_)
 	{
-		// Store current modname in registry
-		lua_pushstring(L, modname.c_str());
-		lua_setfield(L, LUA_REGISTRYINDEX, "current_modname");
+		// Store current mod name in registry
+		lua_pushstring(L, mod_name.c_str());
+		lua_setfield(L, LUA_REGISTRYINDEX, SCRIPT_MOD_NAME_FIELD);
 	}
 	~ModNameStorer()
 	{
-		// Clear current modname in registry
+		// Clear current mod name from registry
 		lua_pushnil(L);
-		lua_setfield(L, LUA_REGISTRYINDEX, "current_modname");
+		lua_setfield(L, LUA_REGISTRYINDEX, SCRIPT_MOD_NAME_FIELD);
 	}
 };
 
@@ -112,32 +113,31 @@ ScriptApiBase::~ScriptApiBase()
 	lua_close(m_luastack);
 }
 
-bool ScriptApiBase::loadMod(const std::string &scriptpath,
-		const std::string &modname)
+bool ScriptApiBase::loadMod(const std::string &script_path,
+		const std::string &mod_name)
 {
-	ModNameStorer modnamestorer(getStack(), modname);
+	ModNameStorer mod_name_storer(getStack(), mod_name);
 
-	if (!string_allowed(modname, MODNAME_ALLOWED_CHARS)) {
-		errorstream<<"Error loading mod \""<<modname
-				<<"\": modname does not follow naming conventions: "
-				<<"Only chararacters [a-z0-9_] are allowed."<<std::endl;
-		return false;
-	}
-
-	return loadScript(scriptpath);
+	return loadScript(script_path);
 }
 
-bool ScriptApiBase::loadScript(const std::string &scriptpath)
+bool ScriptApiBase::loadScript(const std::string &script_path)
 {
-	verbosestream<<"Loading and running script from "<<scriptpath<<std::endl;
+	verbosestream << "Loading and running script from " << script_path << std::endl;
 
 	lua_State *L = getStack();
 
-	int ret = luaL_loadfile(L, scriptpath.c_str()) || lua_pcall(L, 0, 0, m_errorhandler);
-	if (ret) {
+	bool ok;
+	if (m_secure) {
+		ok = ScriptApiSecurity::safeLoadFile(L, script_path.c_str());
+	} else {
+		ok = !luaL_loadfile(L, script_path.c_str());
+	}
+	ok = ok && !lua_pcall(L, 0, 0, m_errorhandler);
+	if (!ok) {
 		errorstream << "========== ERROR FROM LUA ===========" << std::endl;
 		errorstream << "Failed to load and run script from " << std::endl;
-		errorstream << scriptpath << ":" << std::endl;
+		errorstream << script_path << ":" << std::endl;
 		errorstream << std::endl;
 		errorstream << lua_tostring(L, -1) << std::endl;
 		errorstream << std::endl;

diff --git a/src/script/cpp_api/s_base.h b/src/script/cpp_api/s_base.h
@@ -35,24 +35,31 @@ extern "C" {
 
 #define SCRIPTAPI_LOCK_DEBUG
 
+#define SCRIPT_MOD_NAME_FIELD "current_mod_name"
+// MUST be an invalid mod name so that mods can't
+// use that name to bypass security!
+#define BUILTIN_MOD_NAME "*builtin*"
+
+
 class Server;
 class Environment;
 class GUIEngine;
 class ServerActiveObject;
 
 class ScriptApiBase {
 public:
-
 	ScriptApiBase();
 	virtual ~ScriptApiBase();
 
-	bool loadMod(const std::string &scriptpath, const std::string &modname);
-	bool loadScript(const std::string &scriptpath);
+	bool loadMod(const std::string &script_path, const std::string &mod_name);
+	bool loadScript(const std::string &script_path);
 
 	/* object */
 	void addObjectReference(ServerActiveObject *cobj);
 	void removeObjectReference(ServerActiveObject *cobj);
 
+	Server* getServer() { return m_server; }
+
 protected:
 	friend class LuaABM;
 	friend class InvRef;
@@ -69,7 +76,6 @@ class ScriptApiBase {
 	void scriptError();
 	void stackDump(std::ostream &o);
 
-	Server* getServer() { return m_server; }
 	void setServer(Server* server) { m_server = server; }
 
 	Environment* getEnv() { return m_environment; }
@@ -84,6 +90,7 @@ class ScriptApiBase {
 	JMutex          m_luastackmutex;
 	// Stack index of Lua error handler
 	int             m_errorhandler;
+	bool            m_secure;
 #ifdef SCRIPTAPI_LOCK_DEBUG
 	bool            m_locked;
 #endif