Skip to content

Commit

Permalink
Add mod security
Browse files Browse the repository at this point in the history
Due to compatibility concerns, this is temporarily disabled.
  • Loading branch information
ShadowNinja committed May 16, 2015
1 parent f264212 commit 3a8c788
Show file tree
Hide file tree
Showing 22 changed files with 809 additions and 80 deletions.
3 changes: 2 additions & 1 deletion build/android/jni/Android.mk
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand All @@ -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 \
Expand Down
4 changes: 4 additions & 0 deletions doc/lua_api.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions minetest.conf.example
Original file line number Diff line number Diff line change
Expand Up @@ -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

1 change: 1 addition & 0 deletions src/defaultsettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
13 changes: 13 additions & 0 deletions src/filesys.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 7 additions & 3 deletions src/filesys.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

4 changes: 1 addition & 3 deletions src/script/common/c_internal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand Down
3 changes: 2 additions & 1 deletion src/script/cpp_api/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand Down
44 changes: 22 additions & 22 deletions src/script/cpp_api/s_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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);
}
};

Expand Down Expand Up @@ -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;
Expand Down
15 changes: 11 additions & 4 deletions src/script/cpp_api/s_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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; }
Expand All @@ -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
Expand Down
Loading

5 comments on commit 3a8c788

@4aiman
Copy link
Contributor

@4aiman 4aiman commented on 3a8c788 Jun 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShadowNinja: Overall it's a good idea.

-1 for no "\n" in minetest.setting_set(name, value), though.
I want to use multi-line MOTDs sometimes :)

@ShadowNinja
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@4aiman: Multi-line setting values should still work...

@4aiman
Copy link
Contributor

@4aiman 4aiman commented on 3a8c788 Jun 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShadowNinja How should I set those via setting_set?

@ShadowNinja
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@4aiman: Just pass the string to setting_set of course!

minetest.setting_set("long_test", "line 1\nline 2\nline 3")

@4aiman
Copy link
Contributor

@4aiman 4aiman commented on 3a8c788 Jun 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShadowNinja
But... you've disabled \ns ...
Or that's only for the names of variables?

Please sign in to comment.