From 6fb48fd45a4bcee6741ccf861531debe353d583b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 31 Jul 2026 16:22:33 +0200 Subject: [PATCH 1/7] Fix heap buffer overflow in CSO block device from crafted index table A crafted .cso compressed ISO could trigger a heap buffer overflow on any sector read, reachable via ordinary gameplay. Two root causes: 1. Unvalidated frame index deltas. ReadBlock/ReadBlocks computed a compressed read range from two adjacent frame-index-table entries. With non-monotonic entries, compressedReadEnd - compressedReadPos underflows as u64, producing a huge read size that fileLoader->ReadAt() wrote into the fixed-size readBuffer. 2. hdr.align (indexShift, 0-255) used as '1 << indexShift' was undefined behavior at >= 32, and frameSize + (1 << indexShift) could wrap, undersizing the buffer while inflate() was configured with the full frameSize. Fixes: - Validate the index table is monotonically non-decreasing in the constructor. - Reject files with indexShift > 20. - Use unsigned shift for buffer size math and store readBufferSize. - Clamp compressed read sizes to readBufferSize in both ReadBlock and ReadBlocks. --- Core/FileSystems/BlockDevices.cpp | 36 ++++++++++++++++++++++++------- Core/FileSystems/BlockDevices.h | 1 + 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/Core/FileSystems/BlockDevices.cpp b/Core/FileSystems/BlockDevices.cpp index e0762714a1..9fa60d751c 100644 --- a/Core/FileSystems/BlockDevices.cpp +++ b/Core/FileSystems/BlockDevices.cpp @@ -544,17 +544,24 @@ CISOFileBlockDevice::CISOFileBlockDevice(FileLoader *fileLoader) ++blockShift; indexShift = hdr.align; + // Sanity check the index shift: index entries are u32 masked to 31 bits, + // and are shifted up by indexShift to get byte offsets. Values above a + // couple dozen would be bogus and could overflow the buffer size math. + if (indexShift > 20) { + errorString_ = StringFromFormat("CSO index alignment %i unsupported", indexShift); + return; + } const u64 totalSize = hdr.total_bytes; numFrames = (u32)((totalSize + frameSize - 1) / frameSize); numBlocks = (u32)(totalSize / GetBlockSize()); VERBOSE_LOG(Log::Loader, "CSO numBlocks=%i numFrames=%i align=%i", numBlocks, numFrames, indexShift); // We might read a bit of alignment too, so be prepared. - if (frameSize + (1 << indexShift) < CSO_READ_BUFFER_SIZE) - readBuffer = new u8[CSO_READ_BUFFER_SIZE]; - else - readBuffer = new u8[frameSize + (1 << indexShift)]; - zlibBuffer = new u8[frameSize + (1 << indexShift)]; + readBufferSize = frameSize + (1u << indexShift); + if (readBufferSize < CSO_READ_BUFFER_SIZE) + readBufferSize = CSO_READ_BUFFER_SIZE; + readBuffer = new u8[readBufferSize]; + zlibBuffer = new u8[frameSize + (1u << indexShift)]; zlibBufferFrame = numFrames; const u32 indexSize = numFrames + 1; @@ -592,6 +599,16 @@ CISOFileBlockDevice::CISOFileBlockDevice(FileLoader *fileLoader) return; } + // Index entries must be monotonically non-decreasing, otherwise ReadBlock() + // would compute a negative (underflowed) compressed read size from two + // adjacent entries. Reject such files rather than reading into a fixed buffer. + for (u32 i = 0; i < indexSize - 1; i++) { + if ((index[i] & 0x7FFFFFFF) > (index[i + 1] & 0x7FFFFFFF)) { + errorString_ = StringFromFormat("CSO index is not monotonic at entry %d", i); + return; + } + } + // all ok. _dbg_assert_(errorString_.empty()); } @@ -619,7 +636,9 @@ bool CISOFileBlockDevice::ReadBlock(int blockNumber, u8 *outPtr, bool uncached) const u64 compressedReadPos = (u64)indexPos << indexShift; const u64 compressedReadEnd = (u64)nextIndexPos << indexShift; - const size_t compressedReadSize = (size_t)(compressedReadEnd - compressedReadPos); + // A single frame's compressed data must fit in readBuffer. Guard against + // crafted index entries with huge gaps (index[i+1] >> index[i]). + const size_t compressedReadSize = std::min((size_t)(compressedReadEnd - compressedReadPos), readBufferSize); const u32 compressedOffset = (blockNumber & ((1 << blockShift) - 1)) * GetBlockSize(); bool plain = (idx & 0x80000000) != 0; @@ -712,13 +731,14 @@ bool CISOFileBlockDevice::ReadBlocks(u32 minBlock, int count, u8 *outPtr) { const u64 frameReadPos = (u64)indexPos << indexShift; const u64 frameReadEnd = (u64)nextIndexPos << indexShift; - const u32 frameReadSize = (u32)(frameReadEnd - frameReadPos); + // A single frame's compressed data must fit in readBuffer. + const u32 frameReadSize = (u32)std::min((size_t)(frameReadEnd - frameReadPos), readBufferSize); const u32 frameBlockOffset = block & ((1 << blockShift) - 1); const u32 frameBlocks = std::min(lastBlock - block + 1, blocksPerFrame - frameBlockOffset); if (frameReadEnd > readBufferEnd) { const s64 maxNeeded = totalReadEnd - frameReadPos; - const size_t chunkSize = (size_t)std::min(maxNeeded, (s64)std::max(frameReadSize, CSO_READ_BUFFER_SIZE)); + const size_t chunkSize = (size_t)std::min(std::min(maxNeeded, (s64)std::max(frameReadSize, CSO_READ_BUFFER_SIZE)), (s64)readBufferSize); const u32 readSize = (u32)fileLoader_->ReadAt(frameReadPos, 1, chunkSize, readBuffer); if (readSize < chunkSize) { diff --git a/Core/FileSystems/BlockDevices.h b/Core/FileSystems/BlockDevices.h index 6bf39698c1..826134c0b0 100644 --- a/Core/FileSystems/BlockDevices.h +++ b/Core/FileSystems/BlockDevices.h @@ -77,6 +77,7 @@ public: private: u32 *index = nullptr; u8 *readBuffer = nullptr; + size_t readBufferSize = 0; u8 *zlibBuffer = nullptr; u32 zlibBufferFrame = 0; u8 indexShift = 0; From 135f59617f2e80cc5c4a64f405573b24ca95f5de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 31 Jul 2026 16:25:59 +0200 Subject: [PATCH 2/7] Add instructions for running the C++ unit tests to AGENTS.md The /unittest subdirectory contains a separate binary with C++ unit tests alongside the pspautotests runner. Document how to build and run them (UnitTest.exe on Windows, PPSSPPUnitTest on Linux/Mac), and that they should be run after substantial changes at the end of a chunk of work. --- AGENTS.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 74b0b1ce5d..b1e1ae64de 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,16 @@ Ignore the folder ai_instructions in the root directory, it's old stuff from con ## Build and Validation To verify that things build on Linux/Mac, use ./b.sh --debug. For Windows, use the Visual Studio solution in the Windows subdirectory. -Do not run unit test (I will add instructions for how to run them later). + +In addition to the pspautotests runner (test.py), there is a separate binary with C++ unit tests +in the /unittest subdirectory. After substantial changes (at the end of a chunk of work, not +necessarily after every edit), run these too: + +- Windows: build the `UnitTest` project (unittest/UnitTests.vcxproj), then run `Windows/x64/Debug/UnitTest.exe all` +- Linux/Mac: configure with `-DUNITTEST=ON`, then run `build/PPSSPPUnitTest all` + +This runs all tests in `availableTests` in unittest/UnitTest.cpp. You can run a single test by +passing its name instead of `all`; no arguments lists the available tests. ## Multiplatform considerations From e80619215472680a423179268fff87402c35388a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 31 Jul 2026 16:35:02 +0200 Subject: [PATCH 3/7] Delete the ability to load file handler plugins from extracted game folders If there's demand for this feature, we'll bring it back and put it behind a developer option. Fixes vuln #7 from the recent collection --- Core/FileSystems/VirtualDiscFileSystem.cpp | 156 +-------------------- Core/FileSystems/VirtualDiscFileSystem.h | 95 +------------ 2 files changed, 8 insertions(+), 243 deletions(-) diff --git a/Core/FileSystems/VirtualDiscFileSystem.cpp b/Core/FileSystems/VirtualDiscFileSystem.cpp index baa5dcdacc..be43ef48fc 100644 --- a/Core/FileSystems/VirtualDiscFileSystem.cpp +++ b/Core/FileSystems/VirtualDiscFileSystem.cpp @@ -40,9 +40,6 @@ #include #include #include -#if !PPSSPP_PLATFORM(SWITCH) -#include -#endif #endif const std::string INDEX_FILENAME = ".ppsspp-index.lst"; @@ -59,9 +56,6 @@ VirtualDiscFileSystem::~VirtualDiscFileSystem() { iter->second.Close(); } } - for (auto iter = handlers.begin(), end = handlers.end(); iter != end; ++iter) { - delete iter->second; - } } void VirtualDiscFileSystem::LoadFileListIndex() { @@ -107,39 +101,13 @@ void VirtualDiscFileSystem::LoadFileListIndex() { filename_pos++; } - // Check if there's a handler specified. - size_t handler_pos = line.find(':', filename_pos); - if (handler_pos != line.npos) { - entry.fileName = line.substr(filename_pos, handler_pos - filename_pos); - - std::string handler = line.substr(handler_pos + 1); - size_t trunc = handler.find_last_not_of("\r\n"); - if (trunc != handler.npos && trunc != handler.size()) - handler.resize(trunc + 1); - - if (handlers.find(handler) == handlers.end()) - handlers[handler] = new Handler(handler.c_str(), this); - if (handlers[handler]->IsValid()) - entry.handler = handlers[handler]; - } else { - entry.fileName = line.substr(filename_pos); - } + entry.fileName = line.substr(filename_pos); size_t trunc = entry.fileName.find_last_not_of("\r\n"); if (trunc != entry.fileName.npos && trunc != entry.fileName.size()) entry.fileName.resize(trunc + 1); entry.firstBlock = (u32)strtol(line.c_str(), NULL, 16); - if (entry.handler != NULL && entry.handler->IsValid()) { - HandlerFileHandle temp = entry.handler; - if (temp.Open(basePath.ToString(), entry.fileName, FILEACCESS_READ)) { - entry.totalSize = (u32)temp.Seek(0, FILEMOVE_END); - temp.Close(); - } else { - ERROR_LOG(Log::FileSystem, "Unable to open virtual file: %s", entry.fileName.c_str()); - } - } else { - entry.totalSize = File::GetFileSize(GetLocalPath(entry.fileName)); - } + entry.totalSize = File::GetFileSize(GetLocalPath(entry.fileName)); // Try to keep currentBlockIndex sane, in case there are other files. u32 nextBlock = entry.firstBlock + (entry.totalSize + 2047) / 2048; @@ -194,10 +162,6 @@ void VirtualDiscFileSystem::DoState(PointerWrap &p) // open file if (of.type != VFILETYPE_ISO) { - if (fileList[of.fileIndex].handler != NULL) { - of.handler = fileList[of.fileIndex].handler; - } - bool success = of.Open(basePath, fileList[of.fileIndex].fileName, FILEACCESS_READ); if (!success) { ERROR_LOG(Log::FileSystem, "Failed to create file handle for %s.", fileList[of.fileIndex].fileName.c_str()); @@ -332,19 +296,15 @@ int VirtualDiscFileSystem::OpenFile(std::string filename, FileAccess access, con entry.size = readSize; int fileIndex = getFileListIndex(sectorStart,readSize); - if (fileIndex == -1) - { + if (fileIndex == -1) { ERROR_LOG(Log::FileSystem, "VirtualDiscFileSystem: sce_lbn used without calling fileinfo."); return 0; } entry.fileIndex = (u32)fileIndex; - entry.startOffset = (sectorStart-fileList[entry.fileIndex].firstBlock)*2048; + entry.startOffset = (sectorStart-fileList[entry.fileIndex].firstBlock) * 2048; // now we just need an actual file handle - if (fileList[entry.fileIndex].handler != NULL) { - entry.handler = fileList[entry.fileIndex].handler; - } bool success = entry.Open(basePath, fileList[entry.fileIndex].fileName, FILEACCESS_READ); if (!success) { @@ -370,9 +330,6 @@ int VirtualDiscFileSystem::OpenFile(std::string filename, FileAccess access, con entry.type = VFILETYPE_NORMAL; entry.fileIndex = getFileListIndex(filename); - if (entry.fileIndex != (u32)-1 && fileList[entry.fileIndex].handler != NULL) { - entry.handler = fileList[entry.fileIndex].handler; - } bool success = entry.Open(basePath, filename, (FileAccess)(access & FILEACCESS_PSP_FLAGS)); if (!success) { @@ -461,9 +418,6 @@ size_t VirtualDiscFileSystem::ReadFile(u32 handle, u8 *pointer, s64 size, int &u } OpenFileEntry temp(Flags()); - if (fileList[fileIndex].handler != NULL) { - temp.handler = fileList[fileIndex].handler; - } bool success = temp.Open(basePath, fileList[fileIndex].fileName, FILEACCESS_READ); if (!success) @@ -571,22 +525,6 @@ PSPFileInfo VirtualDiscFileSystem::GetFileInfo(std::string filename) { } int fileIndex = getFileListIndex(filename); - if (fileIndex != -1 && fileList[fileIndex].handler != NULL) { - x.type = FILETYPE_NORMAL; - x.isOnSectorSystem = true; - x.startSector = fileList[fileIndex].firstBlock; - x.access = 0555; - - HandlerFileHandle temp = fileList[fileIndex].handler; - if (temp.Open(basePath.ToString(), filename, FILEACCESS_READ)) { - x.exists = true; - x.size = temp.Seek(0, FILEMOVE_END); - temp.Close(); - } - - // TODO: Probably should include dates or something... - return x; - } Path fullName = GetLocalPath(filename); if (!File::Exists(fullName)) { @@ -799,89 +737,3 @@ bool VirtualDiscFileSystem::RemoveFile(const std::string &filename) ERROR_LOG(Log::FileSystem,"VirtualDiscFileSystem: Cannot remove file on virtual disc"); return false; } - -void VirtualDiscFileSystem::HandlerLogger(void *arg, HandlerHandle handle, LogLevel level, const char *msg) { - VirtualDiscFileSystem *sys = static_cast(arg); - - // TODO: Probably could do this smarter / use a lookup. - const char *filename = NULL; - for (auto it = sys->entries.begin(), end = sys->entries.end(); it != end; ++it) { - if (it->second.fileIndex != (u32)-1 && it->second.handler.handle == handle) { - filename = sys->fileList[it->second.fileIndex].fileName.c_str(); - break; - } - } - - if (filename != NULL) { - GENERIC_LOG(Log::FileSystem, level, "%s: %s", filename, msg); - } else { - GENERIC_LOG(Log::FileSystem, level, "%s", msg); - } -} - -VirtualDiscFileSystem::Handler::Handler(const char *filename, VirtualDiscFileSystem *const sys) -: sys_(sys) { -#if !PPSSPP_PLATFORM(SWITCH) -#ifdef _WIN32 -#if PPSSPP_PLATFORM(UWP) -#define dlopen(name, ignore) (void *)LoadPackagedLibrary(ConvertUTF8ToWString(name).c_str(), 0) -#define dlsym(mod, name) GetProcAddress((HMODULE)mod, name) -#define dlclose(mod) FreeLibrary((HMODULE)mod) -#else -#define dlopen(name, ignore) (void *)LoadLibrary(ConvertUTF8ToWString(name).c_str()) -#define dlsym(mod, name) GetProcAddress((HMODULE)mod, name) -#define dlclose(mod) FreeLibrary((HMODULE)mod) -#endif -#endif - - library = dlopen(filename, RTLD_LOCAL | RTLD_NOW); - if (library != NULL) { - Init = (InitFunc)dlsym(library, "Init"); - Shutdown = (ShutdownFunc)dlsym(library, "Shutdown"); - Open = (OpenFunc)dlsym(library, "Open"); - Seek = (SeekFunc)dlsym(library, "Seek"); - Read = (ReadFunc)dlsym(library, "Read"); - Close = (CloseFunc)dlsym(library, "Close"); - - VersionFunc Version = (VersionFunc)dlsym(library, "Version"); - if (Version && Version() >= 2) { - ShutdownV2 = (ShutdownV2Func)Shutdown; - } - - if (!Init || !Shutdown || !Open || !Seek || !Read || !Close) { - ERROR_LOG(Log::FileSystem, "Unable to find all handler functions: %s", filename); - dlclose(library); - library = NULL; - } else if (!Init(&HandlerLogger, sys)) { - ERROR_LOG(Log::FileSystem, "Unable to initialize handler: %s", filename); - dlclose(library); - library = NULL; - } - } else { - ERROR_LOG(Log::FileSystem, "Unable to load handler '%s': %s", filename, GetLastErrorMsg().c_str()); - } -#ifdef _WIN32 -#undef dlopen -#undef dlsym -#undef dlclose -#endif -#endif -} - -VirtualDiscFileSystem::Handler::~Handler() { - if (library != NULL) { - if (ShutdownV2) - ShutdownV2(sys_); - else - Shutdown(); - -#if !PPSSPP_PLATFORM(UWP) && !PPSSPP_PLATFORM(SWITCH) -#ifdef _WIN32 - FreeLibrary((HMODULE)library); -#else - dlclose(library); -#endif -#endif - } -} - diff --git a/Core/FileSystems/VirtualDiscFileSystem.h b/Core/FileSystems/VirtualDiscFileSystem.h index 00fb4ada5d..202b338e37 100644 --- a/Core/FileSystems/VirtualDiscFileSystem.h +++ b/Core/FileSystems/VirtualDiscFileSystem.h @@ -66,73 +66,6 @@ private: int getFileListIndex(u32 accessBlock, u32 accessSize, bool blockMode = false) const; Path GetLocalPath(std::string_view localpath) const; - typedef void *HandlerLibrary; - typedef int HandlerHandle; - typedef s64 HandlerOffset; - typedef void (*HandlerLogFunc)(void *arg, HandlerHandle handle, LogLevel level, const char *msg); - - static void HandlerLogger(void *arg, HandlerHandle handle, LogLevel level, const char *msg); - - // The primary purpose of handlers is to make it easier to work with large archives. - // However, they have other uses as well, such as patching individual files. - struct Handler { - Handler(const char *filename, VirtualDiscFileSystem *const sys); - ~Handler(); - - typedef bool (*InitFunc)(HandlerLogFunc logger, void *loggerArg); - typedef void (*ShutdownFunc)(); - typedef void (*ShutdownV2Func)(void *loggerArg); - typedef HandlerHandle (*OpenFunc)(const char *basePath, const char *filename); - typedef HandlerOffset (*SeekFunc)(HandlerHandle handle, HandlerOffset offset, FileMove origin); - typedef HandlerOffset (*ReadFunc)(HandlerHandle handle, void *data, HandlerOffset size); - typedef void (*CloseFunc)(HandlerHandle handle); - typedef int (*VersionFunc)(); - - HandlerLibrary library; - VirtualDiscFileSystem *const sys_; - InitFunc Init; - ShutdownFunc Shutdown; - ShutdownV2Func ShutdownV2; - OpenFunc Open; - SeekFunc Seek; - ReadFunc Read; - CloseFunc Close; - - bool IsValid() const { return library != nullptr; } - }; - - struct HandlerFileHandle { - Handler *handler; - HandlerHandle handle; - - HandlerFileHandle() : handler(nullptr), handle(0) {} - HandlerFileHandle(Handler *handler_) : handler(handler_), handle(-1) {} - - bool Open(const std::string& basePath, const std::string& fileName, FileAccess access) { - // Ignore access, read only. - handle = handler->Open(basePath.c_str(), fileName.c_str()); - return handle > 0; - } - size_t Read(u8 *data, s64 size) { - return (size_t)handler->Read(handle, data, size); - } - size_t Seek(s32 position, FileMove type) { - return (size_t)handler->Seek(handle, position, type); - } - void Close() { - handler->Close(handle); - } - - bool IsValid() { - return handler != nullptr && handler->IsValid(); - } - - HandlerFileHandle &operator =(Handler *_handler) { - handler = _handler; - return *this; - } - }; - typedef enum { VFILETYPE_NORMAL, VFILETYPE_LBN, VFILETYPE_ISO } VirtualFileType; struct OpenFileEntry { @@ -142,7 +75,6 @@ private: } DirectoryFileHandle hFile; - HandlerFileHandle handler; VirtualFileType type = VFILETYPE_NORMAL; u32 fileIndex = 0; u64 curOffset = 0; @@ -152,32 +84,16 @@ private: bool Open(const Path &basePath, std::string& fileName, FileAccess access) { // Ignored, we're read only. u32 err; - if (handler.IsValid()) { - return handler.Open(basePath.ToString(), fileName, access); - } else { - return hFile.Open(basePath, fileName, access, err); - } + return hFile.Open(basePath, fileName, access, err); } size_t Read(u8 *data, s64 size) { - if (handler.IsValid()) { - return handler.Read(data, size); - } else { - return hFile.Read(data, size); - } + return hFile.Read(data, size); } size_t Seek(s32 position, FileMove type) { - if (handler.IsValid()) { - return handler.Seek(position, type); - } else { - return hFile.Seek(position, type); - } + return hFile.Seek(position, type); } void Close() { - if (handler.IsValid()) { - return handler.Close(); - } else { - return hFile.Close(); - } + return hFile.Close(); } }; @@ -191,12 +107,9 @@ private: std::string fileName; u32 firstBlock; u32 totalSize; - Handler *handler; }; std::vector fileList; u32 currentBlockIndex; u32 lastReadBlock_; - - std::map handlers; }; From f3d7d8bc0c57a3baa60820468ec7f932fe368cd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 31 Jul 2026 16:49:28 +0200 Subject: [PATCH 4/7] Fix Zip Slip in zip extraction and add unit test A crafted zip with a parent-directory ("..") entry name could escape the destination directory during extraction, writing arbitrary files on the host (e.g. into startup/autostart folders). ExtractZipContents built the output path by concatenating the raw zip entry name onto the destination with no traversal check. Changes: - Add HasParentDirComponent() utility in Core/Util/PathUtil and use it in GameManager::ExtractZipContents to reject entries with a ".." component. Guard both the directory-creation and file-writing passes. - Expose ExtractZipContents as public for testing. - Add unittest/TestZipSlip which crafts a zip with a "../evil.txt" entry and verifies it is not written outside the destination directory. --- CMakeLists.txt | 1 + Core/Util/GameManager.cpp | 13 +++++ Core/Util/GameManager.h | 5 +- Core/Util/PathUtil.cpp | 14 +++++ Core/Util/PathUtil.h | 5 ++ android/jni/Android.mk | 1 + unittest/TestZipSlip.cpp | 89 ++++++++++++++++++++++++++++++ unittest/UnitTest.cpp | 2 + unittest/UnitTests.vcxproj | 1 + unittest/UnitTests.vcxproj.filters | 1 + 10 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 unittest/TestZipSlip.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index c70387526e..c84467d10a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1389,6 +1389,7 @@ if(UNITTEST) unittest/TestX64Emitter.cpp unittest/TestVertexJit.cpp unittest/TestVFS.cpp + unittest/TestZipSlip.cpp unittest/TestRiscVEmitter.cpp unittest/TestLoongArch64Emitter.cpp unittest/TestSoftwareGPUJit.cpp diff --git a/Core/Util/GameManager.cpp b/Core/Util/GameManager.cpp index 696250683e..ff6fb23439 100644 --- a/Core/Util/GameManager.cpp +++ b/Core/Util/GameManager.cpp @@ -51,6 +51,7 @@ #include "Core/System.h" #include "Core/FileSystems/ISOFileSystem.h" #include "Core/Util/GameManager.h" +#include "Core/Util/PathUtil.h" #include "Core/Util/RecentFiles.h" #include "Common/Data/Text/I18n.h" @@ -630,7 +631,14 @@ bool GameManager::ExtractZipContents(struct zip *z, const Path &dest, const ZipF auto sy = GetI18NCategory(I18NCat::SYSTEM); + // Reject any entry whose path contains a parent-directory ("..") component. + // Without this, a crafted zip could write files outside the destination + // directory (Zip Slip). auto fileAllowed = [&](const char *fn) { + if (HasParentDirComponent(fn)) { + INFO_LOG(Log::HLE, "Skipping file %s due to parent directory component", fn); + return false; + } if (!allowRoot && strchr(fn, '/') == 0) { INFO_LOG(Log::HLE, "Skipping file %s in root of zip (allowRoot == false)", fn); return false; @@ -658,6 +666,11 @@ bool GameManager::ExtractZipContents(struct zip *z, const Path &dest, const ZipF if (zippedName.length() < (size_t)info.stripChars) { continue; } + // Skip entries that we'd reject when writing, so we don't create + // directories for them either (e.g. ones with parent dir components). + if (!fileAllowed(fn)) { + continue; + } Path outFilename = dest / zippedName.substr(info.stripChars); bool isDir = zippedName.empty() || zippedName.back() == '/'; diff --git a/Core/Util/GameManager.h b/Core/Util/GameManager.h index 5b17e1c5fb..9eb39f0bfd 100644 --- a/Core/Util/GameManager.h +++ b/Core/Util/GameManager.h @@ -88,10 +88,11 @@ public: // Separate kind of functionality from InstallZipOnThread, so doesn't re-use the task struct. bool UninstallGameOnThread(const std::string &name); + // Extracts the contents of an open zip archive into dest. Exposed for testing. + bool ExtractZipContents(struct zip *z, const Path &dest, const ZipFileInfo &info, bool allowRoot); + private: void InstallZipContents(ZipFileTask task); - - bool ExtractZipContents(struct zip *z, const Path &dest, const ZipFileInfo &info, bool allowRoot); bool InstallMemstickZip(const Path &zipFile, const Path &dest, const ZipFileInfo &info); bool InstallZippedISO(struct zip *z, int isoFileIndex, const Path &destDir); void UninstallGame(const std::string &name); diff --git a/Core/Util/PathUtil.cpp b/Core/Util/PathUtil.cpp index 96be0eac06..a5cb1a20d8 100644 --- a/Core/Util/PathUtil.cpp +++ b/Core/Util/PathUtil.cpp @@ -1,3 +1,4 @@ +#include #include #include "Common/File/Path.h" @@ -10,6 +11,19 @@ #include "Core/Config.h" #include "Common/VR/PPSSPPVR.h" +bool HasParentDirComponent(std::string_view path) { + for (size_t i = 0; i < path.size(); ) { + size_t end = path.find_first_of("/\\", i); + size_t len = end == std::string_view::npos ? path.size() - i : end - i; + if (len == 2 && path[i] == '.' && path[i + 1] == '.') + return true; + if (end == std::string_view::npos) + break; + i = end + 1; + } + return false; +} + 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 55be307e31..0b7c812044 100644 --- a/Core/Util/PathUtil.h +++ b/Core/Util/PathUtil.h @@ -29,6 +29,11 @@ enum PSPDirectories { COUNT, }; +// Returns true if the given path (e.g. a zip entry name) contains a parent +// directory ("..") component. Used to guard against path traversal when +// extracting or writing files to disk. +bool HasParentDirComponent(std::string_view path); + Path FindConfigFile(const Path &searchPath, std::string_view baseFilename, bool *exists); Path GetSysDirectory(PSPDirectories directoryType); bool CreateSysDirectories(); diff --git a/android/jni/Android.mk b/android/jni/Android.mk index c7bcc03370..22d33ec9cf 100644 --- a/android/jni/Android.mk +++ b/android/jni/Android.mk @@ -1042,6 +1042,7 @@ ifeq ($(UNITTEST),1) $(SRC)/unittest/TestThreadManager.cpp \ $(SRC)/unittest/TestVertexJit.cpp \ $(SRC)/unittest/TestVFS.cpp \ + $(SRC)/unittest/TestZipSlip.cpp \ $(TESTARMEMITTER_FILE) \ $(SRC)/unittest/UnitTest.cpp diff --git a/unittest/TestZipSlip.cpp b/unittest/TestZipSlip.cpp new file mode 100644 index 0000000000..a9aaf416f9 --- /dev/null +++ b/unittest/TestZipSlip.cpp @@ -0,0 +1,89 @@ +#include "ext/libzip/zip.h" + +#include "Common/File/FileUtil.h" +#include "Common/File/Path.h" +#include "Core/Loaders.h" +#include "Core/Util/GameManager.h" +#include "Core/Util/PathUtil.h" + +#include "UnitTest.h" + +static bool TestHasParentDirComponent() { + EXPECT_TRUE(HasParentDirComponent("../../evil.txt")); + EXPECT_TRUE(HasParentDirComponent("game/../../evil.txt")); + EXPECT_TRUE(HasParentDirComponent("..")); + EXPECT_TRUE(HasParentDirComponent("sub/..")); + EXPECT_TRUE(HasParentDirComponent("..\\evil.txt")); + EXPECT_TRUE(HasParentDirComponent("a/b/../..")); + EXPECT_FALSE(HasParentDirComponent("normal.txt")); + EXPECT_FALSE(HasParentDirComponent("game/evil.txt")); + EXPECT_FALSE(HasParentDirComponent("a.b/c.d")); + EXPECT_FALSE(HasParentDirComponent("")); + EXPECT_FALSE(HasParentDirComponent("/absolute/path.txt")); + return true; +} + +// Creates a zip archive at the given path with one entry of the given name. +static bool CreateZipWithEntry(const Path &zipPath, const std::string &entryName, const std::string &contents) { + int errorp = 0; + zip_t *z = zip_open(zipPath.c_str(), ZIP_CREATE | ZIP_TRUNCATE, &errorp); + if (!z) + return false; + zip_source_t *source = zip_source_buffer(z, contents.data(), contents.size(), 0); + if (!source) { + zip_close(z); + return false; + } + if (zip_file_add(z, entryName.c_str(), source, ZIP_FL_ENC_UTF_8) < 0) { + zip_source_free(source); + zip_close(z); + return false; + } + return zip_close(z) == 0; +} + +// Crafts a zip with a parent-directory entry and verifies ExtractZipContents +// refuses to write outside the destination directory (Zip Slip). +static bool TestZipSlipExtraction() { + Path tempRoot = Path("unittest_zip_slip_test"); + File::DeleteDirRecursively(tempRoot); + EXPECT_TRUE(File::CreateDir(tempRoot)); + + Path destDir = tempRoot / "dest"; + EXPECT_TRUE(File::CreateDir(destDir)); + + Path zipPath = tempRoot / "bad.zip"; + EXPECT_TRUE(CreateZipWithEntry(zipPath, "../evil.txt", "should not escape")); + + int errorp = 0; + zip_t *z = zip_open(zipPath.c_str(), 0, &errorp); + EXPECT_TRUE(z != nullptr); + + ZipFileInfo info; + info.numFiles = 1; + info.stripChars = 0; + info.ignoreMetaFiles = false; + + GameManager manager; + EXPECT_TRUE(manager.ExtractZipContents(z, destDir, info, true)); + zip_close(z); + + // The malicious file must not have been written outside destDir. + // A naive "dest / ../evil.txt" would land here. + EXPECT_FALSE(File::Exists(tempRoot / "evil.txt")); + // And the actual file should not exist inside destDir either. + EXPECT_FALSE(File::Exists(destDir / "evil.txt")); + + // Clean up. + File::Delete(zipPath); + File::DeleteDirRecursively(tempRoot); + return true; +} + +bool TestZipSlip() { + if (!TestHasParentDirComponent()) + return false; + if (!TestZipSlipExtraction()) + return false; + return true; +} diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index 85a8dd9b4a..e4e19d95d5 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -1390,6 +1390,7 @@ bool TestSoftwareGPUJit(); bool TestIRPassSimplify(); bool TestThreadManager(); bool TestVFS(); +bool TestZipSlip(); TestItem availableTests[] = { #if PPSSPP_ARCH(ARM64) || PPSSPP_ARCH(AMD64) || PPSSPP_ARCH(X86) @@ -1445,6 +1446,7 @@ TestItem availableTests[] = { TEST_ITEM(LinAlg), TEST_ITEM(Lang), TEST_ITEM(CmdLine), + TEST_ITEM(ZipSlip), }; int main(int argc, const char *argv[]) { diff --git a/unittest/UnitTests.vcxproj b/unittest/UnitTests.vcxproj index 00bbdc2bef..bba20414d9 100644 --- a/unittest/UnitTests.vcxproj +++ b/unittest/UnitTests.vcxproj @@ -295,6 +295,7 @@ + true diff --git a/unittest/UnitTests.vcxproj.filters b/unittest/UnitTests.vcxproj.filters index 6c7b2f460d..2bf9cad342 100644 --- a/unittest/UnitTests.vcxproj.filters +++ b/unittest/UnitTests.vcxproj.filters @@ -17,6 +17,7 @@ + From b7b96c3374c6c064fa0cb67421e315c9de6335ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 31 Jul 2026 17:00:17 +0200 Subject: [PATCH 5/7] Fix LZRC decompressor heap overflow and add unit test The LZRC decompressor's only bounds check for output (and input) was a debug-only _dbg_assert_msg_, which is a no-op in release builds. The NPDRM demo block device also passed a hardcoded 1 MiB output length while the real destination buffer (blockBuf_) could be as small as 2048 bytes, allowing a crafted NPDRM image to trigger an unbounded heap overflow during game load. Changes: - rc_putbyte/rc_getbyte now enforce real bounds and set an error flag instead of relying on debug asserts; decompression aborts with -1 on overflow or truncated input. - normalize() reads via rc_getbyte so it stays in bounds. - Plain-text path clamps the copy size to both the output buffer and the remaining input (and no longer interprets the size as signed). - NPDRMDemoBlockDevice::ReadBlock passes blockSize_ (the real buffer size) instead of 0x00100000 to lzrc_decompress. - Add unittest/TestLzrc (synthetic input, no test data files): checks the plain-text clamp, truncated input, and output overflow all fail safely. - AGENTS.md: note to reuse existing format handlers/decompressors before writing new ones. --- AGENTS.md | 9 +++- CMakeLists.txt | 1 + Core/FileSystems/BlockDevices.cpp | 4 +- Core/FileSystems/tlzrc.cpp | 33 +++++++++++---- android/jni/Android.mk | 1 + unittest/TestLzrc.cpp | 67 ++++++++++++++++++++++++++++++ unittest/UnitTest.cpp | 2 + unittest/UnitTests.vcxproj | 1 + unittest/UnitTests.vcxproj.filters | 1 + 9 files changed, 108 insertions(+), 11 deletions(-) create mode 100644 unittest/TestLzrc.cpp diff --git a/AGENTS.md b/AGENTS.md index b1e1ae64de..e7acd69b6d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,13 @@ Qt/main.cpp android/jni/app-android.cpp libretro/libretro.cpp +## File formats, codecs, and other format handlers + +Before implementing any file format handler, decompressor, codec, or similar from scratch, search the +codebase first - PPSSPP already has implementations of many formats (CSO, LZRC, zlib-based loaders, ISO +handlers, PBP, SevenZip, etc.), possibly in several places. Reuse or extend an existing one instead of +writing a new one (e.g. there is an LZRC decompressor in Core/FileSystems/tlzrc.cpp). + ## Headless and unittest builds We have additional PPSSPPHeadless and unit test builds (/headless and /unittest), that have their own separate @@ -80,7 +87,7 @@ small examples to copy from). A module is a `const HLEFunction []` table o first three (`Android.mk`/`Makefile.common` are plain compiled-source lists so headers don't go in them). Only the CMakeLists.txt change can be verified from a Linux/Mac build - the rest can't be build-tested here, so double check them by hand against how an existing neighboring file (e.g. `sceVaudio.cpp`) is - listed in each. + listed in each. Note: New files in the unittest project have to be updated in the unittest part in android/jni/Android.mk. ## WebSocket debugger diff --git a/CMakeLists.txt b/CMakeLists.txt index c84467d10a..d29f8c406c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1390,6 +1390,7 @@ if(UNITTEST) unittest/TestVertexJit.cpp unittest/TestVFS.cpp unittest/TestZipSlip.cpp + unittest/TestLzrc.cpp unittest/TestRiscVEmitter.cpp unittest/TestLoongArch64Emitter.cpp unittest/TestSoftwareGPUJit.cpp diff --git a/Core/FileSystems/BlockDevices.cpp b/Core/FileSystems/BlockDevices.cpp index 9fa60d751c..ccc28913e0 100644 --- a/Core/FileSystems/BlockDevices.cpp +++ b/Core/FileSystems/BlockDevices.cpp @@ -963,7 +963,9 @@ bool NPDRMDemoBlockDevice::ReadBlock(int blockNumber, u8 *outPtr, bool uncached) } if (table_[block].size < blockSize_) { - int lzsize = lzrc_decompress(blockBuf_, 0x00100000, readBuf, table_[block].size); + // The decompressed block is always blockSize_ bytes; blockBuf_ is exactly + // that big. Pass the real size so the decompressor can't write past it. + int lzsize = lzrc_decompress(blockBuf_, blockSize_, readBuf, table_[block].size); if(lzsize != blockSize_){ ERROR_LOG(Log::Loader, "LZRC decompress error! lzsize=%d\n", lzsize); NotifyReadError(); diff --git a/Core/FileSystems/tlzrc.cpp b/Core/FileSystems/tlzrc.cpp index 82749c8fce..92fbfb2888 100644 --- a/Core/FileSystems/tlzrc.cpp +++ b/Core/FileSystems/tlzrc.cpp @@ -46,6 +46,9 @@ typedef struct{ int out_ptr; int out_len; + // Set when input/output bounds are exceeded; decompression should abort. + int error; + // range decode u32 range; u32 code; @@ -63,8 +66,9 @@ typedef struct{ static u8 rc_getbyte(LZRC_DECODE *rc) { - if(rc->in_ptr == rc->in_len){ - _dbg_assert_msg_(false, "LZRC: End of input!"); + if(rc->in_ptr >= rc->in_len){ + rc->error = 1; + return 0; } return rc->input[rc->in_ptr++]; @@ -72,8 +76,9 @@ static u8 rc_getbyte(LZRC_DECODE *rc) static void rc_putbyte(LZRC_DECODE *rc, u8 byte) { - if(rc->out_ptr == rc->out_len){ - _dbg_assert_msg_(false, "LZRC: Output overflow!"); + if(rc->out_ptr >= rc->out_len){ + rc->error = 1; + return; } rc->output[rc->out_ptr++] = byte; @@ -89,6 +94,8 @@ static void rc_init(LZRC_DECODE *rc, void *out, int out_len, void *in, int in_le rc->out_len = out_len; rc->out_ptr = 0; + rc->error = 0; + rc->range = 0xffffffff; rc->lc = rc_getbyte(rc); rc->code = (rc_getbyte(rc)<<24) | @@ -115,8 +122,7 @@ static void normalize(LZRC_DECODE *rc) { if(rc->range<0x01000000){ rc->range <<= 8; - rc->code = (rc->code<<8)+rc->input[rc->in_ptr]; - rc->in_ptr++; + rc->code = (rc->code<<8)+rc_getbyte(rc); } } @@ -213,20 +219,29 @@ int lzrc_decompress(void *out, int out_len, void *in, int in_len) rc_init(&rc, out, out_len, in, in_len); + if(rc.error) + return -1; + if(rc.lc&0x80){ /* plain text */ - int copySize = rc.code; - if (copySize > out_len) { + u32 copySize = rc.code; + if (copySize > (u32)out_len) { copySize = out_len; } + if (rc.in_len >= 5 && copySize > (u32)(rc.in_len - 5)) { + copySize = rc.in_len - 5; + } memcpy(rc.output, rc.input+5, copySize); - return copySize; + return (int)copySize; } rc_state = 0; last_byte = 0; while (1) { + if(rc.error) + return -1; + round += 1; match_step = 0; diff --git a/android/jni/Android.mk b/android/jni/Android.mk index 22d33ec9cf..9210afcdcd 100644 --- a/android/jni/Android.mk +++ b/android/jni/Android.mk @@ -1042,6 +1042,7 @@ ifeq ($(UNITTEST),1) $(SRC)/unittest/TestThreadManager.cpp \ $(SRC)/unittest/TestVertexJit.cpp \ $(SRC)/unittest/TestVFS.cpp \ + $(SRC)/unittest/TestLzrc.cpp \ $(SRC)/unittest/TestZipSlip.cpp \ $(TESTARMEMITTER_FILE) \ $(SRC)/unittest/UnitTest.cpp diff --git a/unittest/TestLzrc.cpp b/unittest/TestLzrc.cpp new file mode 100644 index 0000000000..39442a6698 --- /dev/null +++ b/unittest/TestLzrc.cpp @@ -0,0 +1,67 @@ +#include +#include + +#include "Common/CommonTypes.h" +#include "UnitTest.h" + +// No header exists for the LZRC decompressor; it's declared locally by callers. +int lzrc_decompress(void *out, int out_len, void *in, int in_len); + +// The plain-text path (lc bit 0x80 set) must clamp the copy size to both the +// output buffer and the remaining input, not read past the end of input. +static bool TestLzrcPlainTextClamp() { + // lc = 0x80 (plain text), code = 0xFFFFFF (huge), then "hello". + u8 input[] = { 0x80, 0xFF, 0xFF, 0xFF, 0x00, 'h', 'e', 'l', 'l', 'o' }; + u8 output[16]; + memset(output, 0xAA, sizeof(output)); + + int result = lzrc_decompress(output, sizeof(output), input, sizeof(input)); + // copySize = min(0xFFFFFF, 16, 10 - 5 = 5) = 5. + EXPECT_EQ_INT(result, 5); + EXPECT_TRUE(memcmp(output, "hello", 5) == 0); + EXPECT_TRUE(output[5] == 0xAA); + return true; +} + +// Truncated input (fewer than the 5 header bytes) must not read past the buffer. +static bool TestLzrcTruncatedInput() { + u8 input[] = { 0x00 }; + u8 output[16]; + memset(output, 0xAA, sizeof(output)); + + int result = lzrc_decompress(output, sizeof(output), input, 1); + EXPECT_EQ_INT(result, -1); + // Nothing should have been written. + for (size_t i = 0; i < sizeof(output); ++i) { + EXPECT_TRUE(output[i] == 0xAA); + } + return true; +} + +// A compressed stream that would overflow a tiny output buffer must return an +// error and not write past the buffer. +static bool TestLzrcOutputOverflow() { + // lc = 0x00 (compressed), code = 0. Feed enough bytes for the decoder to run. + u8 input[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }; + u8 output[4]; + memset(output, 0xBB, sizeof(output)); + + int result = lzrc_decompress(output, 0, input, sizeof(input)); + // The decoder may bail out as an error (-1) or see end-of-stream (0), + // but must never write into the (zero-sized) output. + EXPECT_TRUE(result == -1 || result == 0); + for (size_t i = 0; i < sizeof(output); ++i) { + EXPECT_TRUE(output[i] == 0xBB); + } + return true; +} + +bool TestLzrc() { + if (!TestLzrcPlainTextClamp()) + return false; + if (!TestLzrcTruncatedInput()) + return false; + if (!TestLzrcOutputOverflow()) + return false; + return true; +} diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index e4e19d95d5..9882c37ec6 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -1391,6 +1391,7 @@ bool TestIRPassSimplify(); bool TestThreadManager(); bool TestVFS(); bool TestZipSlip(); +bool TestLzrc(); TestItem availableTests[] = { #if PPSSPP_ARCH(ARM64) || PPSSPP_ARCH(AMD64) || PPSSPP_ARCH(X86) @@ -1447,6 +1448,7 @@ TestItem availableTests[] = { TEST_ITEM(Lang), TEST_ITEM(CmdLine), TEST_ITEM(ZipSlip), + TEST_ITEM(Lzrc), }; int main(int argc, const char *argv[]) { diff --git a/unittest/UnitTests.vcxproj b/unittest/UnitTests.vcxproj index bba20414d9..e3fa6e17c8 100644 --- a/unittest/UnitTests.vcxproj +++ b/unittest/UnitTests.vcxproj @@ -292,6 +292,7 @@ + diff --git a/unittest/UnitTests.vcxproj.filters b/unittest/UnitTests.vcxproj.filters index 2bf9cad342..745603ecd7 100644 --- a/unittest/UnitTests.vcxproj.filters +++ b/unittest/UnitTests.vcxproj.filters @@ -15,6 +15,7 @@ + From 6d231f3f450d9a817736c457bfd3819d7747ccc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 31 Jul 2026 17:16:02 +0200 Subject: [PATCH 6/7] Guard SAS assembly buffer against oversized ATRAC packets A crafted blockAlign could overflow the fixed 1000-byte assembly stack buffer in Atrac2::DecodeForSas. Bail out if the packet can't fit. --- Core/HLE/AtracCtx2.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Core/HLE/AtracCtx2.cpp b/Core/HLE/AtracCtx2.cpp index de68b8a371..2a41b15e70 100644 --- a/Core/HLE/AtracCtx2.cpp +++ b/Core/HLE/AtracCtx2.cpp @@ -1214,6 +1214,15 @@ void Atrac2::DecodeForSas(s16 *dstData, int *bytesWritten, int *finish) { // TODO: Do we need special handling for the first buffer, since SetData will wrap around that packet? I think yes! DEBUG_LOG(Log::Atrac, "Streaming atrac through sas, and hit the end of buffer %d", sas_.curBuffer); + // The packet spans two buffers and is reassembled into the fixed + // assembly buffer. Bail out if it can't possibly fit there. + if ((u32)info.sampleSize > sizeof(assembly)) { + ERROR_LOG(Log::Atrac, "SAS packet too large for assembly buffer: %d", info.sampleSize); + *bytesWritten = 0; + *finish = 1; + return; + } + // Compute the part sizes using the current size. int part1Size = sas_.bufSize[sas_.curBuffer] - sas_.streamOffset; int part2Size = info.sampleSize - part1Size; From 5194382b7b3191dd278049651049895ffe2c30bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 31 Jul 2026 17:20:07 +0200 Subject: [PATCH 7/7] Fix out-of-bounds reads in PARAM.SFO parser ReadSFO dereferenced index table entries without checking the table fit within the buffer, and GetDataOffset had no bounds checks at all (reading attacker-controlled offsets and strcmp'ing without a terminator guard). - Validate the index table fits entirely within the buffer in ReadSFO. - Add a size parameter to GetDataOffset and validate the index table, key/data table positions, and key string termination before use. --- Core/Dialog/SavedataParam.cpp | 2 +- Core/ELF/ParamSFO.cpp | 25 ++++++++++++++++++++++++- Core/ELF/ParamSFO.h | 2 +- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/Core/Dialog/SavedataParam.cpp b/Core/Dialog/SavedataParam.cpp index 209e2382eb..08d7dcb4c0 100644 --- a/Core/Dialog/SavedataParam.cpp +++ b/Core/Dialog/SavedataParam.cpp @@ -543,7 +543,7 @@ int SavedataParam::Save(SceUtilitySavedataParam* param, const std::string &saveD // Calc SFO hash for PSP. if (cryptedData != 0 || (subWrite && wasCrypted)) { - int offset = sfoFile->GetDataOffset(sfoData, "SAVEDATA_PARAMS"); + int offset = sfoFile->GetDataOffset(sfoData, sfoSize, "SAVEDATA_PARAMS"); if (offset >= 0) UpdateHash(sfoData, (int)sfoSize, offset, DetermineCryptMode(param)); } diff --git a/Core/ELF/ParamSFO.cpp b/Core/ELF/ParamSFO.cpp index 154a517a6f..0e605c6be5 100644 --- a/Core/ELF/ParamSFO.cpp +++ b/Core/ELF/ParamSFO.cpp @@ -129,6 +129,12 @@ bool ParamSFOData::ReadSFO(const u8 *paramsfo, size_t size) { const IndexTable *indexTables = (const IndexTable *)(paramsfo + sizeof(Header)); + // The index table itself must fit entirely within the buffer before we + // can dereference entries; otherwise indexTables[i] reads out of bounds. + if (sizeof(Header) + (size_t)header->index_table_entries * sizeof(IndexTable) > size) { + return false; + } + if (header->key_table_start > size || header->data_table_start > size) { return false; } @@ -203,13 +209,21 @@ bool ParamSFOData::ReadSFO(const u8 *paramsfo, size_t size) { return true; } -int ParamSFOData::GetDataOffset(const u8 *paramsfo, const char *dataName) { +int ParamSFOData::GetDataOffset(const u8 *paramsfo, size_t size, const char *dataName) { + if (size < sizeof(Header)) + return -1; const Header *header = (const Header *)paramsfo; if (header->magic != 0x46535000) return -1; if (header->version != 0x00000101) WARN_LOG(Log::Loader, "Unexpected SFO header version: %08x", header->version); + // Both the index table and the key/data tables must fit in the buffer. + if (sizeof(Header) + (size_t)header->index_table_entries * sizeof(IndexTable) > size) + return -1; + if (header->key_table_start > size || header->data_table_start > size) + return -1; + const IndexTable *indexTables = (const IndexTable *)(paramsfo + sizeof(Header)); const u8 *key_start = paramsfo + header->key_table_start; @@ -217,7 +231,16 @@ int ParamSFOData::GetDataOffset(const u8 *paramsfo, const char *dataName) { for (u32 i = 0; i < header->index_table_entries; i++) { + size_t key_offset = header->key_table_start + indexTables[i].key_table_offset; + if (key_offset >= size) + continue; + if (data_start + indexTables[i].data_table_offset >= (int)size) + continue; + const char *key = (const char *)(key_start + indexTables[i].key_table_offset); + // Ensure the key string is NUL-terminated within the buffer before strcmp. + if (strnlen(key, size - key_offset) == size - key_offset) + continue; if (!strcmp(key, dataName)) { return data_start + indexTables[i].data_table_offset; diff --git a/Core/ELF/ParamSFO.h b/Core/ELF/ParamSFO.h index e1bb92c653..f1f48d4759 100644 --- a/Core/ELF/ParamSFO.h +++ b/Core/ELF/ParamSFO.h @@ -56,7 +56,7 @@ public: } // If not found, returns a negative value. - int GetDataOffset(const u8 *paramsfo, const char *dataName); + int GetDataOffset(const u8 *paramsfo, size_t size, const char *dataName); bool IsValid() const { return !values.empty(); } void Clear();