Fix RemoveRelativePathComponents (again) and fix mod security with paths starting with ".."

src/filesys.cpp

@@ -617,48 +617,51 @@ std::string RemoveRelativePathComponents(std::string path)
 {
 	size_t pos = path.size();
 	size_t dotdot_count = 0;
-	while(pos != 0){
+	while (pos != 0) {
 		size_t component_with_delim_end = pos;
 		// skip a dir delimiter
-		while(pos != 0 && IsDirDelimiter(path[pos-1]))
+		while (pos != 0 && IsDirDelimiter(path[pos-1]))
 			pos--;
 		// strip a path component
 		size_t component_end = pos;
-		while(pos != 0 && !IsDirDelimiter(path[pos-1]))
+		while (pos != 0 && !IsDirDelimiter(path[pos-1]))
 			pos--;
 		size_t component_start = pos;
 
 		std::string component = path.substr(component_start,
 				component_end - component_start);
 		bool remove_this_component = false;
-		if(component == "." && component_start != 0){
+		if (component == ".") {
 			remove_this_component = true;
-		}
-		else if(component == ".."){
+		} else if (component == "..") {
 			remove_this_component = true;
 			dotdot_count += 1;
-		}
-		else if(dotdot_count != 0){
+		} else if (dotdot_count != 0) {
 			remove_this_component = true;
 			dotdot_count -= 1;
 		}
 
-		if(remove_this_component){
-			while(pos != 0 && IsDirDelimiter(path[pos-1]))
+		if (remove_this_component) {
+			while (pos != 0 && IsDirDelimiter(path[pos-1]))
 				pos--;
-			path = path.substr(0, pos) + DIR_DELIM +
-				path.substr(component_with_delim_end,
-						std::string::npos);
-			pos++;
+			if (component_start == 0) {
+				// We need to remove the delemiter too
+				path = path.substr(component_with_delim_end, std::string::npos);
+			} else {
+				path = path.substr(0, pos) + DIR_DELIM +
+					path.substr(component_with_delim_end, std::string::npos);
+			}
+			if (pos > 0)
+				pos++;
 		}
 	}
 
-	if(dotdot_count > 0)
+	if (dotdot_count > 0)
 		return "";
 
 	// remove trailing dir delimiters
 	pos = path.size();
-	while(pos != 0 && IsDirDelimiter(path[pos-1]))
+	while (pos != 0 && IsDirDelimiter(path[pos-1]))
 		pos--;
 	return path.substr(0, pos);
 }

src/script/cpp_api/s_security.cpp

@@ -340,8 +340,7 @@ bool ScriptApiSecurity::checkPath(lua_State *L, const char *path)
 {
 	std::string str;  // Transient
 
-	std::string norel_path = fs::RemoveRelativePathComponents(path);
-	std::string abs_path = fs::AbsolutePath(norel_path);
+	std::string abs_path = fs::AbsolutePath(path);
 
 	if (!abs_path.empty()) {
 		// Don't allow accessing the settings file
@@ -352,18 +351,29 @@ bool ScriptApiSecurity::checkPath(lua_State *L, const char *path)
 	// If we couldn't find the absolute path (path doesn't exist) then
 	// try removing the last components until it works (to allow
 	// non-existent files/folders for mkdir).
-	std::string cur_path = norel_path;
+	std::string cur_path = path;
 	std::string removed;
 	while (abs_path.empty() && !cur_path.empty()) {
-		std::string tmp_rmed;
-		cur_path = fs::RemoveLastPathComponent(cur_path, &tmp_rmed);
-		removed = tmp_rmed + (removed.empty() ? "" : DIR_DELIM + removed);
+		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;
+	if (abs_path.empty())
+		return false;
 	// Add the removed parts back so that you can't, eg, create a
 	// directory in worldmods if worldmods doesn't exist.
-	if (!removed.empty()) abs_path += DIR_DELIM + removed;
+	if (!removed.empty())
+		abs_path += DIR_DELIM + removed;
 
 	// Get server from registry
 	lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_SCRIPTAPI);

src/unittest/test_filepath.cpp

@@ -251,7 +251,10 @@ void TestFilePath::testRemoveRelativePathComponent()
 	UASSERT(result == p("/home/user/minetest/worlds/world1"));
 	path = p(".");
 	result = fs::RemoveRelativePathComponents(path);
-	UASSERT(result == ".");
+	UASSERT(result == "");
+	path = p("../a");
+	result = fs::RemoveRelativePathComponents(path);
+	UASSERT(result == "");
 	path = p("./subdir/../..");
 	result = fs::RemoveRelativePathComponents(path);
 	UASSERT(result == "");