diff --git a/Common/Data/Encoding/Compression.cpp b/Common/Data/Encoding/Compression.cpp index c348ed9313..0ac584243d 100644 --- a/Common/Data/Encoding/Compression.cpp +++ b/Common/Data/Encoding/Compression.cpp @@ -54,7 +54,7 @@ bool compress_string(const std::string& str, std::string *dest, int compressionl } /** Decompress an STL string using zlib and return the original data. */ -bool decompress_string(const std::string& str, std::string *dest) { +bool decompress_string(const std::string& str, std::string *dest, size_t maxSize) { if (!str.size()) return false; @@ -85,7 +85,11 @@ bool decompress_string(const std::string& str, std::string *dest) { outstring.append(outbuffer, zs.total_out - outstring.size()); } - + if (outstring.size() > maxSize) { + inflateEnd(&zs); + ERROR_LOG(Log::IO, "Decompressed data is larger than %d bytes, giving up", (int)maxSize); + return false; + } } while (ret == Z_OK); inflateEnd(&zs); @@ -95,6 +99,6 @@ bool decompress_string(const std::string& str, std::string *dest) { return false; } - *dest = outstring; + *dest = std::move(outstring); return true; } diff --git a/Common/Data/Encoding/Compression.h b/Common/Data/Encoding/Compression.h index 5323b8f549..b66db3e16c 100644 --- a/Common/Data/Encoding/Compression.h +++ b/Common/Data/Encoding/Compression.h @@ -4,4 +4,5 @@ // inflate/deflate convenience wrapper. Uses zlib. bool compress_string(const std::string& str, std::string *dest, int compressionlevel = 9); -bool decompress_string(const std::string& str, std::string *dest); +// Fails if the output would be larger than maxSize (a small input can expand enormously). +bool decompress_string(const std::string& str, std::string *dest, size_t maxSize = 256 * 1024 * 1024); diff --git a/Common/File/VFS/DirectoryReader.cpp b/Common/File/VFS/DirectoryReader.cpp index 386568d70b..1b5b78a06d 100644 --- a/Common/File/VFS/DirectoryReader.cpp +++ b/Common/File/VFS/DirectoryReader.cpp @@ -24,13 +24,17 @@ static std::string ToLowerASCII(std::string_view str) { return lower; } -void DirectoryReader::IndexDirectory(const Path &dir, const std::string &prefix) { +void DirectoryReader::IndexDirectory(const Path &dir, const std::string &prefix, int depth) { + // A symlink loop would otherwise recurse until the stack runs out. + if (depth > 32) { + return; + } std::vector files; File::GetFilesInDir(dir, &files); for (const File::FileInfo &file : files) { const std::string relative = prefix + file.name; if (file.isDirectory) { - IndexDirectory(file.fullName, relative + "/"); + IndexDirectory(file.fullName, relative + "/", depth + 1); } else { lowerIndex_[ToLowerASCII(relative)] = relative; } @@ -44,7 +48,7 @@ bool DirectoryReader::FindIgnoringCase(std::string_view path, std::string *actua std::lock_guard guard(indexLock_); if (!indexBuilt_) { // Only done on the first miss, so a directory where every name matches never pays for it. - IndexDirectory(path_, ""); + IndexDirectory(path_, "", 0); indexBuilt_ = true; INFO_LOG(Log::IO, "Indexed %d files in %s to find names that differ in case", (int)lowerIndex_.size(), path_.c_str()); } @@ -134,13 +138,22 @@ void DirectoryReader::ReleaseFile(VFSFileReference *vfsReference) { VFSOpenFile *DirectoryReader::OpenFileForRead(VFSFileReference *vfsReference, size_t *size) { DirectoryReaderFileReference *reference = (DirectoryReaderFileReference *)vfsReference; + // On some platforms, directories can be opened as files. Their size is meaningless. + if (File::IsDirectory(reference->path)) { + return nullptr; + } FILE *file = File::OpenCFile(reference->path, "rb"); if (!file) { return nullptr; } File::Fseek(file, 0, SEEK_END); - *size = File::Ftell(file); + const int64_t fileSize = File::Ftell(file); File::Fseek(file, 0, SEEK_SET); + if (fileSize < 0) { + fclose(file); + return nullptr; + } + *size = (size_t)fileSize; DirectoryReaderOpenFile *openFile = new DirectoryReaderOpenFile(); openFile->file = file; return openFile; diff --git a/Common/File/VFS/DirectoryReader.h b/Common/File/VFS/DirectoryReader.h index b291e7610b..8e3e81c1ea 100644 --- a/Common/File/VFS/DirectoryReader.h +++ b/Common/File/VFS/DirectoryReader.h @@ -36,7 +36,7 @@ public: private: // Returns the path (relative to path_) of a file whose name matches path except for case. bool FindIgnoringCase(std::string_view path, std::string *actualPath); - void IndexDirectory(const Path &dir, const std::string &prefix); + void IndexDirectory(const Path &dir, const std::string &prefix, int depth); Path path_; bool ignoreCase_ = false; diff --git a/Common/File/VFS/ZipFileReader.cpp b/Common/File/VFS/ZipFileReader.cpp index 595083f0f3..7deade30d5 100644 --- a/Common/File/VFS/ZipFileReader.cpp +++ b/Common/File/VFS/ZipFileReader.cpp @@ -16,8 +16,10 @@ #include "Common/StringUtils.h" // Sanity cap on a single zip entry's declared uncompressed size, so a corrupt or -// malicious zip can't drive an implausible (or, near UINT64_MAX, wrapping) allocation. -static constexpr uint64_t MAX_ZIP_ENTRY_SIZE = 1ULL << 32; // 4GB, generous for any real asset. +// malicious zip can't drive an implausible allocation (which also has to fit in a +// 32-bit size_t). Entries are read whole into memory; the largest real asset, an +// uncompressed 8192x8192 replacement texture with mips, is under 512MB. +static constexpr uint64_t MAX_ZIP_ENTRY_SIZE = 512ULL * 1024 * 1024; ZipContainer::ZipContainer() noexcept : sourceData_(nullptr), zip_(nullptr) {} @@ -186,8 +188,13 @@ uint8_t *ZipFileReader::ReadFile(std::string_view path, size_t *size) { return 0; } uint8_t *contents = new uint8_t[zstat.size + 1]; - zip_fread(file, contents, zstat.size); + const zip_int64_t bytesRead = zip_fread(file, contents, zstat.size); zip_fclose(file); + if (bytesRead < 0 || (uint64_t)bytesRead != zstat.size) { + ERROR_LOG(Log::IO, "Error reading %s from ZIP (%lld of %llu bytes)", temp_path.c_str(), (long long)bytesRead, (unsigned long long)zstat.size); + delete[] contents; + return 0; + } contents[zstat.size] = 0; *size = zstat.size; @@ -402,6 +409,13 @@ VFSOpenFile *ZipFileReader::OpenFileForRead(VFSFileReference *vfsReference, size delete openFile; return nullptr; } + // Callers allocate the returned size. + if (zstat.size > MAX_ZIP_ENTRY_SIZE) { + ERROR_LOG(Log::IO, "Zip entry %d claims an implausible size (%llu), refusing to open", reference->zi, (unsigned long long)zstat.size); + lock_.unlock(); + delete openFile; + return nullptr; + } openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0); if (!openFile->zf) { @@ -429,15 +443,20 @@ void ZipFileReader::Rewind(VFSOpenFile *vfsOpenFile) { size_t ZipFileReader::Read(VFSOpenFile *vfsOpenFile, void *buffer, size_t length) { ZipFileReaderOpenFile *file = (ZipFileReaderOpenFile *)vfsOpenFile; _assert_(file); - _dbg_assert_(file->zf != nullptr); - return zip_fread(file->zf, buffer, length); + if (!file->zf) { + return 0; + } + // -1 on errors, like a bad CRC or a corrupt stream. + const zip_int64_t bytesRead = zip_fread(file->zf, buffer, length); + return bytesRead < 0 ? 0 : (size_t)bytesRead; } void ZipFileReader::CloseFile(VFSOpenFile *vfsOpenFile) { ZipFileReaderOpenFile *file = (ZipFileReaderOpenFile *)vfsOpenFile; _assert_(file); - _dbg_assert_(file->zf != nullptr); - zip_fclose(file->zf); + if (file->zf) { + zip_fclose(file->zf); + } file->zf = nullptr; vfsOpenFile = nullptr; lock_.unlock(); diff --git a/Core/FileLoaders/DiskCachingFileLoader.cpp b/Core/FileLoaders/DiskCachingFileLoader.cpp index 2e81f191f6..c40910de9e 100644 --- a/Core/FileLoaders/DiskCachingFileLoader.cpp +++ b/Core/FileLoaders/DiskCachingFileLoader.cpp @@ -87,11 +87,15 @@ size_t DiskCachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Prepare(); size_t readSize; - if (absolutePos >= filesize_) { + if (absolutePos < 0 || absolutePos >= filesize_) { bytes = 0; } else if (absolutePos + (s64)bytes >= filesize_) { bytes = (size_t)(filesize_ - absolutePos); } + // The cache computes the last block as pos + bytes - 1. + if (bytes == 0) { + return 0; + } if (cache_ && cache_->IsValid() && (flags & Flags::HINT_UNCACHED) == 0) { readSize = cache_->ReadFromCache(absolutePos, bytes, data); @@ -357,6 +361,7 @@ bool DiskCachingFileLoaderCache::MakeCacheSpaceFor(size_t blocks) { while (cacheSize_ > goal) { u16 minGeneration = generation_; + bool evicted = false; // We increment the iterator inside because we delete things inside. for (size_t i = 0; i < blockIndexLookup_.size(); ++i) { @@ -377,6 +382,7 @@ bool DiskCachingFileLoaderCache::MakeCacheSpaceFor(size_t blocks) { info.generation = 0; info.hits = 0; --cacheSize_; + evicted = true; // TODO: Doing this in chunks might be a lot better. WriteIndexData(blockIndexLookup_[i], info); @@ -389,6 +395,12 @@ bool DiskCachingFileLoaderCache::MakeCacheSpaceFor(size_t blocks) { } } + // If we found nothing to evict and the next pass would look for the same generation, + // it can't make progress either. + if (!evicted && minGeneration == oldestGeneration_) { + ERROR_LOG(Log::Loader, "Disk cache: Nothing left to evict, %d blocks counted", (int)cacheSize_); + break; + } // If we didn't find any, update to the lowest we did find. oldestGeneration_ = minGeneration; } @@ -548,6 +560,9 @@ bool DiskCachingFileLoaderCache::LoadCacheFile(const Path &path) { } else if (header.maxBlocks < MAX_BLOCKS_LOWER_BOUND || header.maxBlocks > MAX_BLOCKS_UPPER_BOUND) { // This means it's not in our safety bounds, reject. valid = false; + } else if (header.blockSize != DEFAULT_BLOCK_SIZE) { + // We only ever create these, and everything below divides by and allocates the block size. + valid = false; } // If it's valid, retain the file pointer. @@ -600,6 +615,12 @@ void DiskCachingFileLoaderCache::LoadCacheIndex() { if (index_[i].block == INVALID_BLOCK) { continue; } + // Two entries can't share a data block. Keeping both would serve one of them the wrong + // data, and count a block that eviction can't find (which then never finishes). + if (blockIndexLookup_[index_[i].block] != INVALID_INDEX) { + index_[i].block = INVALID_BLOCK; + continue; + } if (index_[i].generation < oldestGeneration_) { oldestGeneration_ = index_[i].generation; diff --git a/Core/FileLoaders/HTTPFileLoader.cpp b/Core/FileLoaders/HTTPFileLoader.cpp index 8830994ee6..c41a9c80b6 100644 --- a/Core/FileLoaders/HTTPFileLoader.cpp +++ b/Core/FileLoaders/HTTPFileLoader.cpp @@ -89,6 +89,11 @@ void HTTPFileLoader::Prepare() { } if (size_pos != header.npos) { filesize_ = atoll(&header[size_pos]); + // The caches size their indexes from this. Nothing we'd load is this big. + if (filesize_ < 0 || filesize_ > (64LL << 30)) { + ERROR_LOG(Log::Loader, "HTTP: Implausible Content-Length: %s", header.c_str()); + filesize_ = 0; + } } } if (startsWithNoCase(header, "Accept-Ranges:")) { diff --git a/Core/FileLoaders/RamCachingFileLoader.cpp b/Core/FileLoaders/RamCachingFileLoader.cpp index 2e402ff1d4..c50255e4c5 100644 --- a/Core/FileLoaders/RamCachingFileLoader.cpp +++ b/Core/FileLoaders/RamCachingFileLoader.cpp @@ -88,6 +88,11 @@ size_t RamCachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, F void RamCachingFileLoader::InitCache() { std::lock_guard guard(blocksMutex_); + // The whole file has to fit in memory (and a size_t, which may be 32 bits). + if (filesize_ <= 0 || (u64)filesize_ > (u64)(SIZE_MAX - BLOCK_SIZE) || (u64)filesize_ > (8ULL << 30)) { + ERROR_LOG(Log::IO, "Can't cache a file of %lld bytes in RAM, will fall back to regular reads.", (long long)filesize_); + return; + } u32 blockCount = (u32)((filesize_ + BLOCK_SIZE - 1) >> BLOCK_SHIFT); // Overallocate for the last block. cache_ = (u8 *)malloc((size_t)blockCount << BLOCK_SHIFT); diff --git a/Core/FileLoaders/ZipFileLoader.cpp b/Core/FileLoaders/ZipFileLoader.cpp index 57e12bcf77..8c6bf1899f 100644 --- a/Core/FileLoaders/ZipFileLoader.cpp +++ b/Core/FileLoaders/ZipFileLoader.cpp @@ -74,6 +74,12 @@ bool ZipFileLoader::Initialize(int fileIndex) { } _dbg_assert_(zstat.index == fileIndex); + // The whole file is decompressed into memory. No disc image is anywhere near 4GB, and the + // size has to fit in a (possibly 32-bit) size_t too. + if (!(zstat.valid & ZIP_STAT_SIZE) || zstat.size > (4ULL << 30) || zstat.size > (u64)SIZE_MAX) { + ERROR_LOG(Log::IO, "File index %d in ZIP has an implausible size (%llu)", fileIndex, (unsigned long long)zstat.size); + return false; + } dataFileSize_ = zstat.size; dataFile_ = zip_fopen_index(zipArchive_, zstat.index, ZIP_FL_UNCHANGED); if (!dataFile_) { diff --git a/Core/FileSystems/BlobFileSystem.cpp b/Core/FileSystems/BlobFileSystem.cpp index 7139c2fcad..586fa87982 100644 --- a/Core/FileSystems/BlobFileSystem.cpp +++ b/Core/FileSystems/BlobFileSystem.cpp @@ -92,6 +92,10 @@ size_t BlobFileSystem::SeekFile(u32 handle, s32 position, FileMove type) { entry->second = fileLoader_->FileSize() + position; break; } + // Reads take the position as a file offset. + if (entry->second < 0) { + entry->second = 0; + } return (size_t)entry->second; } return 0; diff --git a/Core/Loaders.cpp b/Core/Loaders.cpp index ecf8ba6f5a..f76ea3c6cc 100644 --- a/Core/Loaders.cpp +++ b/Core/Loaders.cpp @@ -678,6 +678,10 @@ void DetectZipFileContents(zip_t *z, ZipFileInfo *info) { // is quite convenient and makes it easy to add shared heuristics. for (int i = 0; i < numFiles; i++) { const char *fn = zip_get_name(z, i, 0); + if (!fn) { + // A corrupt central directory entry. + continue; + } zip_stat_t stat{}; zip_stat_index(z, i, 0, &stat); diff --git a/Core/Util/GameManager.cpp b/Core/Util/GameManager.cpp index 3e7fbbe935..a29b1ddf70 100644 --- a/Core/Util/GameManager.cpp +++ b/Core/Util/GameManager.cpp @@ -912,7 +912,8 @@ bool GameManager::InstallZippedISO(struct zip *z, int isoFileIndex, const Path & return false; } std::string fn = fnPtr; - size_t nameOffset = fn.rfind('/'); + // Zips made on Windows can use either separator. + size_t nameOffset = fn.find_last_of("/\\"); if (nameOffset == std::string::npos) { nameOffset = 0; } else { @@ -934,6 +935,13 @@ bool GameManager::InstallZippedISO(struct zip *z, int isoFileIndex, const Path & name = name.substr(2); } + // The name is used as a filename in the destination, so mustn't be able to point outside it. + if (!IsSafePathComponent(name)) { + ERROR_LOG(Log::IO, "Bad ISO filename in zip: '%s'", fn.c_str()); + SetInstallError(I18NCat::INSTALLZIP, "Zip archive corrupt"); + return false; + } + Path outputISOFilename = destDir; if (outputISOFilename.empty()) { outputISOFilename = Path(g_Config.currentDirectory); @@ -952,8 +960,12 @@ bool GameManager::InstallZippedISO(struct zip *z, int isoFileIndex, const Path & installProgress_ = 1.0f; InstallDone(); - ResetInstallError(); - return true; + if (success) { + ResetInstallError(); + } else { + SetInstallError(I18NCat::INSTALLZIP, "Installation failed"); + } + return success; } bool GameManager::InstallZippedPS1Disc(struct zip *z, const std::vector &files, const Path &destDir) {