Merge pull request #22431 from hrydgard/safe-path-component

Validate untrusted IDs before using them as directory names

Core/ELF/ParamSFO.cpp

@@ -26,6 +26,7 @@
 #include "Common/File/Path.h"
 #include "Core/ELF/ParamSFO.h"
 #include "Core/System.h"
+#include "Core/Util/PathUtil.h"
 
 struct Header
 {
@@ -220,6 +221,14 @@ bool ParamSFOData::ReadSFO(const u8 *paramsfo, size_t size) {
 		}
 	}
 
+	// The disc ID names host files and directories (configs, cheats, textures, savestates, updates),
+	// so one that could escape them is treated as missing.
+	const std::string discID(StripSpaces(GetValueString("DISC_ID")));
+	if (!discID.empty() && !IsSafePathComponent(discID)) {
+		WARN_LOG(Log::Loader, "Ignoring unsafe DISC_ID in PARAM.SFO: '%s'", discID.c_str());
+		values.erase("DISC_ID");
+	}
+
 	return true;
 }
 

Core/Util/GameManager.cpp

@@ -496,6 +496,11 @@ bool GameManager::DetectTexturePackDest(struct zip *z, int iniIndex, Path &dest)
 			}
 		}
 	}
+	// The ID comes from the archive and becomes a directory name.
+	if (!IsSafePathComponent(gameID)) {
+		SetInstallError(iz->T("Texture pack doesn't support install"));
+		return false;
+	}
 
 	Path pspTextures = GetSysDirectory(DIRECTORY_TEXTURES);
 	dest = pspTextures / gameID;

Core/Util/PathUtil.cpp

@@ -30,6 +30,19 @@ bool HasPathTraversal(std::string_view path) {
 	return path == "." || path == "..";
 }
 
+bool IsSafePathComponent(std::string_view name) {
+	// The trailing dot check also catches "." and "..".
+	if (name.empty() || name.back() == ' ' || name.back() == '.') {
+		return false;
+	}
+	for (unsigned char c : name) {
+		if (c < 0x20 || c == 0x7F || c == '/' || c == '\\' || c == ':') {
+			return false;
+		}
+	}
+	return true;
+}
+
 Path FindConfigFile(const Path &searchPath, std::string_view baseFilename, bool *exists) {
 	// Don't search for an absolute path.
 	if (baseFilename.size() > 1 && baseFilename[0] == '/') {

Core/Util/PathUtil.h

@@ -40,6 +40,12 @@ bool HasParentDirComponent(std::string_view path);
 // strings that would otherwise become host filesystem path components.
 bool HasPathTraversal(std::string_view path);
 
+// Returns true if an untrusted string (a disc ID, a texture pack's game ID) can be used as a single
+// directory or file name. Stricter than HasPathTraversal: also rejects empty names, control
+// characters, ':' (drive letters, NTFS streams) and the trailing dots and spaces Win32 strips,
+// which would turn "..." into "..".
+bool IsSafePathComponent(std::string_view name);
+
 Path FindConfigFile(const Path &searchPath, std::string_view baseFilename, bool *exists);
 Path GetSysDirectory(PSPDirectories directoryType);
 bool CreateSysDirectories();

Core/Util/PkgUnpack.cpp

@@ -29,6 +29,7 @@
 #include "Core/ELF/PBPReader.h"
 #include "Core/Loaders.h"
 #include "Core/System.h"
+#include "Core/Util/PathUtil.h"
 #include "Core/Util/PkgUnpack.h"
 
 #include "ext/libkirk/AES.h"
@@ -353,8 +354,9 @@ bool PkgReader::ReadPBOOTInfo(const PkgItem &pboot) {
 	if (info_.discVersion.empty()) {
 		info_.discVersion = "1.00";
 	}
-	// A disc ID is what the install is keyed on, so without one there's nowhere to put this.
-	info_.isGameUpdate = !info_.discId.empty();
+	// The install goes in a directory named after the disc ID, so without a usable one there's
+	// nowhere to put this.
+	info_.isGameUpdate = IsSafePathComponent(info_.discId);
 	return info_.isGameUpdate;
 }
 

Core/WebServer.cpp

@@ -580,11 +580,6 @@ enum class MultiPartResult {
 	Done,
 };
 
-static bool IsSafeUploadFilename(std::string_view filename) {
-	return !filename.empty() && filename != "." && filename != ".." &&
-		filename.find_first_of("/\\") == std::string_view::npos;
-}
-
 static MultiPartResult HandleMultipartPart(const http::ServerRequest &request, std::string boundary, const Path &uploadPath, ProgressTracker &progress) {
 	std::string firstBoundary = request.In()->ReadLine();
 	if (firstBoundary != "--" + boundary) {
@@ -625,7 +620,7 @@ static MultiPartResult HandleMultipartPart(const http::ServerRequest &request, s
 		}
 	}
 
-	if (!IsSafeUploadFilename(filename)) {
+	if (!IsSafePathComponent(filename)) {
 		ERROR_LOG(Log::HTTP, "Invalid upload filename");
 		return MultiPartResult::RequestError;
 	}

GPU/Debugger/Playback.cpp

@@ -47,6 +47,7 @@
 #include "Core/MIPS/MIPSCodeUtils.h"
 #include "Core/System.h"
 #include "Core/Util/GameDB.h"
+#include "Core/Util/PathUtil.h"
 #include "GPU/GPUCommon.h"
 #include "GPU/GPUState.h"
 #include "GPU/ge_constants.h"
@@ -1110,13 +1111,14 @@ static u32 LoadReplay(const std::string &filename) {
 		memset(header.gameID, 0, sizeof(header.gameID));
 	}
 
-	size_t gameIDLength = strnlen(header.gameID, sizeof(header.gameID));
-	if (gameIDLength != 0) {
-		g_paramSFO.SetValue("DISC_ID", std::string(header.gameID, gameIDLength), (int)sizeof(header.gameID));
+	const std::string gameID(header.gameID, strnlen(header.gameID, sizeof(header.gameID)));
+	// The ID picks the texture pack directory, among other things.
+	if (IsSafePathComponent(gameID)) {
+		g_paramSFO.SetValue("DISC_ID", gameID, (int)sizeof(header.gameID));
 		std::vector<GameDBInfo> info;
 		std::string gameTitle = "(unknown title)";
 #if !defined(__LIBRETRO__)
-		if (g_gameDB.GetGameInfos(header.gameID, &info)) {
+		if (g_gameDB.GetGameInfos(gameID, &info)) {
 			gameTitle = info[0].title;
 			g_paramSFO.SetValue("TITLE", gameTitle, (int)gameTitle.size());
 		}

unittest/TestZipSlip.cpp

@@ -80,9 +80,30 @@ static bool TestZipSlipExtraction() {
 	return true;
 }
 
+static bool TestIsSafePathComponent() {
+	EXPECT_TRUE(IsSafePathComponent("ULUS10041"));
+	EXPECT_TRUE(IsSafePathComponent("NPJH-50465"));
+	EXPECT_TRUE(IsSafePathComponent("a.b"));
+	EXPECT_TRUE(IsSafePathComponent("My Game"));
+	EXPECT_FALSE(IsSafePathComponent(""));
+	EXPECT_FALSE(IsSafePathComponent("."));
+	EXPECT_FALSE(IsSafePathComponent(".."));
+	EXPECT_FALSE(IsSafePathComponent("..."));
+	EXPECT_FALSE(IsSafePathComponent(".. "));
+	EXPECT_FALSE(IsSafePathComponent("../ULUS10041"));
+	EXPECT_FALSE(IsSafePathComponent("..\\ULUS10041"));
+	EXPECT_FALSE(IsSafePathComponent("C:"));
+	EXPECT_FALSE(IsSafePathComponent("ULUS10041:stream"));
+	EXPECT_FALSE(IsSafePathComponent(std::string_view("ULUS\0/..", 8)));
+	EXPECT_FALSE(IsSafePathComponent("ULUS\n10041"));
+	return true;
+}
+
 bool TestZipSlip() {
 	if (!TestHasParentDirComponent())
 		return false;
+	if (!TestIsSafePathComponent())
+		return false;
 	if (!TestZipSlipExtraction())
 		return false;
 	return true;