Merge pull request #22008 from hrydgard/vuln-fixes-1

Vulnerability fixes 1/3

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
 
@@ -36,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
@@ -71,7 +87,7 @@ small examples to copy from). A module is a `const HLEFunction <name>[]` 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
 

CMakeLists.txt

@@ -1389,6 +1389,8 @@ if(UNITTEST)
 		unittest/TestX64Emitter.cpp
 		unittest/TestVertexJit.cpp
 		unittest/TestVFS.cpp
+		unittest/TestZipSlip.cpp
+		unittest/TestLzrc.cpp
 		unittest/TestRiscVEmitter.cpp
 		unittest/TestLoongArch64Emitter.cpp
 		unittest/TestSoftwareGPUJit.cpp

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));
 	}

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;

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();

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>((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>((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<s64>(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) {
@@ -943,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();

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;

Core/FileSystems/VirtualDiscFileSystem.cpp

@@ -40,9 +40,6 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <ctype.h>
-#if !PPSSPP_PLATFORM(SWITCH)
-#include <dlfcn.h>
-#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<VirtualDiscFileSystem *>(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
-	}
-}
-

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<FileListEntry> fileList;
 	u32 currentBlockIndex;
 	u32 lastReadBlock_;
-
-	std::map<std::string, Handler *> handlers;
 };

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;
 

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;

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() == '/';

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);

Core/Util/PathUtil.cpp

@@ -1,3 +1,4 @@
+#include <algorithm>
 #include <string_view>
 
 #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] == '/') {

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();

android/jni/Android.mk

@@ -1042,6 +1042,8 @@ 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
 

unittest/TestLzrc.cpp

@@ -0,0 +1,67 @@
+#include <cstring>
+#include <cstdio>
+
+#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;
+}

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;
+}

unittest/UnitTest.cpp

@@ -1390,6 +1390,8 @@ bool TestSoftwareGPUJit();
 bool TestIRPassSimplify();
 bool TestThreadManager();
 bool TestVFS();
+bool TestZipSlip();
+bool TestLzrc();
 
 TestItem availableTests[] = {
 #if PPSSPP_ARCH(ARM64) || PPSSPP_ARCH(AMD64) || PPSSPP_ARCH(X86)
@@ -1445,6 +1447,8 @@ TestItem availableTests[] = {
 	TEST_ITEM(LinAlg),
 	TEST_ITEM(Lang),
 	TEST_ITEM(CmdLine),
+	TEST_ITEM(ZipSlip),
+	TEST_ITEM(Lzrc),
 };
 
 int main(int argc, const char *argv[]) {

unittest/UnitTests.vcxproj

@@ -292,9 +292,11 @@
     <ClCompile Include="TestRiscVEmitter.cpp" />
     <ClCompile Include="TestShaderGenerators.cpp" />
     <ClCompile Include="TestSoftwareGPUJit.cpp" />
+    <ClCompile Include="TestLzrc.cpp" />
     <ClCompile Include="TestThreadManager.cpp" />
     <ClCompile Include="TestVertexJit.cpp" />
     <ClCompile Include="TestVFS.cpp" />
+    <ClCompile Include="TestZipSlip.cpp" />
     <ClCompile Include="UnitTest.cpp" />
     <ClCompile Include="TestArmEmitter.cpp">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|ARM64'">true</ExcludedFromBuild>

unittest/UnitTests.vcxproj.filters

@@ -15,8 +15,10 @@
     <ClCompile Include="TestThreadManager.cpp" />
     <ClCompile Include="TestSoftwareGPUJit.cpp" />
     <ClCompile Include="TestIRPassSimplify.cpp" />
+    <ClCompile Include="TestLzrc.cpp" />
     <ClCompile Include="TestRiscVEmitter.cpp" />
     <ClCompile Include="TestVFS.cpp" />
+    <ClCompile Include="TestZipSlip.cpp" />
     <ClCompile Include="TestLoongArch64Emitter.cpp" />
   </ItemGroup>
   <ItemGroup>