Fix script security path normalization in presence of links (#15481)

src/filesys.cpp

@@ -833,16 +833,51 @@ std::string RemoveRelativePathComponents(std::string path)
 std::string AbsolutePath(const std::string &path)
 {
 #ifdef _WIN32
+	// handle behavior differences on windows
+	if (path.empty())
+		return "";
+	else if (!PathExists(path))
+		return "";
 	char *abs_path = _fullpath(NULL, path.c_str(), MAX_PATH);
 #else
 	char *abs_path = realpath(path.c_str(), NULL);
 #endif
-	if (!abs_path) return "";
+	if (!abs_path)
+		return "";
 	std::string abs_path_str(abs_path);
 	free(abs_path);
 	return abs_path_str;
 }
 
+std::string AbsolutePathPartial(const std::string &path)
+{
+	if (path.empty())
+		return "";
+	// Try to determine absolute path
+	std::string abs_path = fs::AbsolutePath(path);
+	if (!abs_path.empty())
+		return abs_path;
+	// Remove components until it works
+	std::string cur_path = path;
+	std::string removed;
+	while (abs_path.empty() && !cur_path.empty()) {
+		std::string component;
+		cur_path = RemoveLastPathComponent(cur_path, &component);
+		removed = component + (removed.empty() ? "" : DIR_DELIM + removed);
+		abs_path = AbsolutePath(cur_path);
+	}
+	// If we had a relative path that does not exist, it needs to be joined with cwd
+	if (cur_path.empty() && !IsPathAbsolute(path))
+		abs_path = AbsolutePath(".");
+	// or there's an error
+	if (abs_path.empty())
+		return "";
+	// Put them back together and resolve the remaining relative components
+	if (!removed.empty())
+		abs_path.append(DIR_DELIM).append(removed);
+	return RemoveRelativePathComponents(abs_path);
+}
+
 const char *GetFilenameFromPath(const char *path)
 {
 	const char *filename = strrchr(path, DIR_DELIM_CHAR);

src/filesys.h

@@ -131,6 +131,12 @@ std::string RemoveRelativePathComponents(std::string path);
 // components and symlinks removed.  Returns "" on error.
 std::string AbsolutePath(const std::string &path);
 
+// This is a combination of RemoveRelativePathComponents() and AbsolutePath()
+// It will resolve symlinks for the leading path components that exist and
+// still remove "." and ".." in the rest of the path.
+// Returns "" on error.
+std::string AbsolutePathPartial(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);

src/porting.cpp

@@ -688,6 +688,10 @@ void initializePaths()
 
 #endif // RUN_IN_PLACE
 
+	assert(!path_share.empty());
+	assert(!path_user.empty());
+	assert(!path_cache.empty());
+
 	infostream << "Detected share path: " << path_share << std::endl;
 	infostream << "Detected user path: " << path_user << std::endl;
 	infostream << "Detected cache path: " << path_cache << std::endl;

src/porting.h

@@ -109,8 +109,7 @@ extern std::string path_cache;
 bool getCurrentExecPath(char *buf, size_t len);
 
 /*
-	Get full path of stuff in data directory.
+	Concatenate subpath to path_share.
-	Example: "stone.png" -> "../data/stone.png"
 */
 std::string getDataPath(const char *subpath);
 

src/script/cpp_api/s_security.cpp

@@ -566,38 +566,23 @@ bool ScriptApiSecurity::checkPath(lua_State *L, const char *path,
 	if (write_allowed)
 		*write_allowed = false;
 
-	std::string abs_path = fs::AbsolutePath(path);
+	// We can't use AbsolutePath() here since we want to allow creating paths that
-
+	// do not yet exist. But RemoveRelativePathComponents() would also be incorrect
-	// If we couldn't find the absolute path (path doesn't exist) then
+	// since that wouldn't normalize subpaths that *do* exist.
-	// try removing the last components until it works (to allow
+	// This is required so that comparisons with other normalized paths work correctly.
-	// non-existent files/folders for mkdir).
+	std::string abs_path = fs::AbsolutePathPartial(path);
-	std::string cur_path = path;
-	std::string removed;
-	while (abs_path.empty() && !cur_path.empty()) {
-		std::string component;
-		cur_path = fs::RemoveLastPathComponent(cur_path, &component);
-		if (component == "..") {
-			// Parent components can't be allowed or we could allow something like
-			// /home/user/minetest/worlds/foo/noexist/../../../../../../etc/passwd.
-			// If we have previous non-relative elements in the path we might be
-			// able to remove them so that things like worlds/foo/noexist/../auth.txt
-			// could be allowed, but those paths will be interpreted as nonexistent
-			// by the operating system anyways.
-			return false;
-		}
-		removed = component + (removed.empty() ? "" : DIR_DELIM + removed);
-		abs_path = fs::AbsolutePath(cur_path);
-	}
-	if (abs_path.empty())
-		return false;
-	// Add the removed parts back so that you can e.g. create a
-	// directory in worldmods if worldmods doesn't exist.
-	if (!removed.empty())
-		abs_path += DIR_DELIM + removed;
-
 	tracestream << "ScriptApiSecurity: path \"" << path << "\" resolved to \""
 		<< abs_path << "\"" << std::endl;
 
+	if (abs_path.empty())
+		return false;
+
+	// Note: abs_path can be a valid path while path isn't, e.g.
+	// abs_path = "/home/user/.luanti"
+	// path = "/home/user/.luanti/noexist/.."
+	// Letting this through the sandbox isn't a concern as any actual attempts to
+	// use the path would fail.
+
 	// Ask the environment-specific implementation
 	auto *sec = ModApiBase::getScriptApi<ScriptApiSecurity>(L);
 	return sec->checkPathInternal(abs_path, write_required, write_allowed);
@@ -617,9 +602,11 @@ bool ScriptApiSecurity::checkPathWithGamedef(lua_State *L,
 	if (!gamedef)
 		return false;
 
-	if (!abs_path.empty()) {
+	assert(!abs_path.empty());
+
+	if (!g_settings_path.empty()) {
 		// Don't allow accessing the settings file
-		str = fs::AbsolutePath(g_settings_path);
+		str = fs::AbsolutePathPartial(g_settings_path);
 		if (str == abs_path)
 			return false;
 	}

src/script/scripting_mainmenu.cpp

@@ -76,10 +76,11 @@ void MainMenuScripting::registerLuaClasses(lua_State *L, int top)
 
 bool MainMenuScripting::mayModifyPath(const std::string &path)
 {
-	if (fs::PathStartsWith(path, fs::TempPath()))
+	std::string path_temp = fs::AbsolutePathPartial(fs::TempPath());
+	if (fs::PathStartsWith(path, path_temp))
 		return true;
 
-	std::string path_user = fs::RemoveRelativePathComponents(porting::path_user);
+	std::string path_user = fs::AbsolutePathPartial(porting::path_user);
 
 	if (fs::PathStartsWith(path, path_user + DIR_DELIM "client"))
 		return true;
@@ -92,7 +93,7 @@ bool MainMenuScripting::mayModifyPath(const std::string &path)
 	if (fs::PathStartsWith(path, path_user + DIR_DELIM "worlds"))
 		return true;
 
-	if (fs::PathStartsWith(path, fs::RemoveRelativePathComponents(porting::path_cache)))
+	if (fs::PathStartsWith(path, fs::AbsolutePathPartial(porting::path_cache)))
 		return true;
 
 	return false;

src/unittest/test_filesys.cpp

@@ -24,6 +24,7 @@ public:
 	void testRemoveLastPathComponent();
 	void testRemoveLastPathComponentWithTrailingDelimiter();
 	void testRemoveRelativePathComponent();
+	void testAbsolutePath();
 	void testSafeWriteToFile();
 	void testCopyFileContents();
 	void testNonExist();
@@ -39,6 +40,7 @@ void TestFileSys::runTests(IGameDef *gamedef)
 	TEST(testRemoveLastPathComponent);
 	TEST(testRemoveLastPathComponentWithTrailingDelimiter);
 	TEST(testRemoveRelativePathComponent);
+	TEST(testAbsolutePath);
 	TEST(testSafeWriteToFile);
 	TEST(testCopyFileContents);
 	TEST(testNonExist);
@@ -55,7 +57,7 @@ static std::string p(std::string path)
 	for (size_t i = 0; i < path.size(); ++i) {
 		if (path[i] == '/') {
 			path.replace(i, 1, DIR_DELIM);
-			i += std::string(DIR_DELIM).size() - 1; // generally a no-op
+			i += strlen(DIR_DELIM) - 1; // generally a no-op
 		}
 	}
 
@@ -259,6 +261,46 @@ void TestFileSys::testRemoveRelativePathComponent()
 }
 
 
+void TestFileSys::testAbsolutePath()
+{
+	const auto dir_path = getTestTempDirectory();
+
+	/* AbsolutePath */
+	UASSERTEQ(auto, fs::AbsolutePath(""), ""); // empty is a not valid path
+	const auto cwd = fs::AbsolutePath(".");
+	UASSERTCMP(auto, !=, cwd, "");
+	{
+		const auto dir_path2 = getTestTempFile();
+		UASSERTEQ(auto, fs::AbsolutePath(dir_path2), ""); // doesn't exist
+		fs::CreateDir(dir_path2);
+		UASSERTCMP(auto, !=, fs::AbsolutePath(dir_path2), ""); // now it does
+		UASSERTEQ(auto, fs::AbsolutePath(dir_path2 + DIR_DELIM ".."), fs::AbsolutePath(dir_path));
+	}
+
+	/* AbsolutePathPartial */
+	// equivalent to AbsolutePath if it exists
+	UASSERTEQ(auto, fs::AbsolutePathPartial("."), cwd);
+	UASSERTEQ(auto, fs::AbsolutePathPartial(dir_path), fs::AbsolutePath(dir_path));
+	// usual usage of the function with a partially existing path
+	auto expect = cwd + DIR_DELIM + p("does/not/exist");
+	UASSERTEQ(auto, fs::AbsolutePathPartial("does/not/exist"), expect);
+	UASSERTEQ(auto, fs::AbsolutePathPartial(expect), expect);
+
+	// a nonsense combination as you couldn't actually access it, but allowed by function
+	UASSERTEQ(auto, fs::AbsolutePathPartial("bla/blub/../.."), cwd);
+	UASSERTEQ(auto, fs::AbsolutePathPartial("./bla/blub/../.."), cwd);
+
+#ifdef __unix__
+	// one way to produce the error case is to remove more components than there are
+	// but only if the path does not actually exist ("/.." does exist).
+	UASSERTEQ(auto, fs::AbsolutePathPartial("/.."), "/");
+	UASSERTEQ(auto, fs::AbsolutePathPartial("/noexist/../.."), "");
+#endif
+	// or with an empty path
+	UASSERTEQ(auto, fs::AbsolutePathPartial(""), "");
+}
+
+
 void TestFileSys::testSafeWriteToFile()
 {
 	const std::string dest_path = getTestTempFile();