diff --git a/Core/ELF/ParamSFO.cpp b/Core/ELF/ParamSFO.cpp index 74191a6816..d5c010f1b7 100644 --- a/Core/ELF/ParamSFO.cpp +++ b/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; } diff --git a/Core/Util/GameManager.cpp b/Core/Util/GameManager.cpp index 19d23555c7..628d4882bc 100644 --- a/Core/Util/GameManager.cpp +++ b/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; diff --git a/Core/Util/PathUtil.cpp b/Core/Util/PathUtil.cpp index 82cc4b0710..2c99ea7c11 100644 --- a/Core/Util/PathUtil.cpp +++ b/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] == '/') { diff --git a/Core/Util/PathUtil.h b/Core/Util/PathUtil.h index 73e4b21264..87c4a497ff 100644 --- a/Core/Util/PathUtil.h +++ b/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(); diff --git a/Core/Util/PkgUnpack.cpp b/Core/Util/PkgUnpack.cpp index 48d03a9a66..9cb7a443ae 100644 --- a/Core/Util/PkgUnpack.cpp +++ b/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; } diff --git a/Core/WebServer.cpp b/Core/WebServer.cpp index 7c30f989d2..9b511a09fb 100644 --- a/Core/WebServer.cpp +++ b/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; } diff --git a/GPU/Debugger/Playback.cpp b/GPU/Debugger/Playback.cpp index 7b1543a937..501c638728 100644 --- a/GPU/Debugger/Playback.cpp +++ b/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 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()); } diff --git a/unittest/TestZipSlip.cpp b/unittest/TestZipSlip.cpp index a9aaf416f9..40a6da70d8 100644 --- a/unittest/TestZipSlip.cpp +++ b/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;