Vulnerability fixes (2/3)

.github/workflows/appimage.yml

@@ -212,7 +212,7 @@ jobs:
           path: artifacts/
 
       - name: Upload release
-        uses: softprops/action-gh-release@718ea10b132b3b2eba29c1007bb80653f286566b # v3.0.1
+        uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3.0.2
         if: github.ref_type == 'tag'
         with:
           token: ${{ secrets.GITHUB_TOKEN }}

.github/workflows/build.yml

@@ -113,7 +113,7 @@ jobs:
         Compress-Archive -Path "ppsspp/*" -Update -DestinationPath "releases/PPSSPP-${{ github.ref_name }}-Windows-${{ matrix.platform }}.zip"
 
     - name: Upload release
-      uses: softprops/action-gh-release@718ea10b132b3b2eba29c1007bb80653f286566b # v3.0.1
+      uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3.0.2
       if: github.ref_type == 'tag'
       with:
         files: releases/*.zip
@@ -406,7 +406,7 @@ jobs:
       run: mv PPSSPPSDL.zip PPSSPPSDL-macOS-${GITHUB_REF_NAME}.zip
 
     - name: Upload macOS & iOS release
-      uses: softprops/action-gh-release@718ea10b132b3b2eba29c1007bb80653f286566b # v3.0.1
+      uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3.0.2
       if: github.ref_type == 'tag' && (matrix.id == 'macos' || matrix.id == 'ios')
       with:
         files: |

.github/workflows/manual_generate_apk.yml

@@ -41,7 +41,7 @@ jobs:
           git fetch --deepen=15000 --no-recurse-submodules --tags --force upstream || exit 0
 
       - name: Setup JDK
-        uses: actions/setup-java@v5
+        uses: actions/setup-java@v5.6.0
         with:
           distribution: 'temurin'
           java-version: '17'

.github/workflows/tarball.yml

@@ -39,7 +39,7 @@ jobs:
         echo "tarball=$TARBALL" >> $GITHUB_OUTPUT
 
     - name: Upload tarball
-      uses: softprops/action-gh-release@718ea10b132b3b2eba29c1007bb80653f286566b # v3.0.1
+      uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3.0.2
       with:
         files: ${{ steps.archive.outputs.tarball }}
         token: ${{ secrets.GITHUB_TOKEN }}

Common/Data/Format/PNGLoad.cpp

@@ -49,7 +49,7 @@ struct PngReadContext {
 };
 
 
-int pngLoadPtr(const unsigned char *input_ptr, size_t input_len, int *pwidth, int *pheight, unsigned char **image_data_ptr) {
+int pngLoadPtr(const unsigned char *input_ptr, size_t input_len, int *pwidth, int *pheight, unsigned char **image_data_ptr, int maxWidth, int maxHeight) {
 	png_structp png = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, pngErrorHandler, pngWarningHandler);
 	if (!png) {
 		return 0;
@@ -118,6 +118,15 @@ int pngLoadPtr(const unsigned char *input_ptr, size_t input_len, int *pwidth, in
 	*pwidth = png_get_image_width(png, info);
 	*pheight = png_get_image_height(png, info);
 
+	// Reject images larger than the caller's limits to avoid decompression
+	// bombs (attacker-controlled dimensions would otherwise drive a huge
+	// allocation here).
+	if (*pwidth > maxWidth || *pheight > maxHeight) {
+		DEBUG_LOG(Log::IO, "PNG too large: %dx%d (max %dx%d)", *pwidth, *pheight, maxWidth, maxHeight);
+		png_destroy_read_struct(&png, &info, NULL);
+		return 0;
+	}
+
 	size_t row_bytes = png_get_rowbytes(png, info);
 	*image_data_ptr = (unsigned char *)malloc(row_bytes * (*pheight));
 	if (!*image_data_ptr) {

Common/Data/Format/PNGLoad.h

@@ -11,7 +11,8 @@ int pngLoad(const char *file, int *pwidth,
             int *pheight, unsigned char **image_data_ptr);
 
 int pngLoadPtr(const unsigned  char *input_ptr, size_t input_len, int *pwidth,
-            int *pheight, unsigned char **image_data_ptr);
+            int *pheight, unsigned char **image_data_ptr,
+            int maxWidth = 8192, int maxHeight = 8192);
 
 // PNG peeker - just read the start of a PNG straight into this struct, in order to
 // look at basic parameters like width and height. Note that while PNG is a chunk-based

Common/Render/ManagedTexture.cpp

@@ -87,7 +87,7 @@ ImageFileType DetectImageFileType(const uint8_t *data, size_t size) {
 	}
 }
 
-bool TempImage::LoadTextureLevelsFromFileData(const uint8_t *data, size_t size, ImageFileType typeSuggestion) {
+bool TempImage::LoadTextureLevelsFromFileData(const uint8_t *data, size_t size, ImageFileType typeSuggestion, int maxWidth, int maxHeight) {
 	if (typeSuggestion == ImageFileType::DETECT) {
 		typeSuggestion = DetectImageFileType(data, size);
 	}
@@ -108,7 +108,7 @@ bool TempImage::LoadTextureLevelsFromFileData(const uint8_t *data, size_t size,
 		break;
 
 	case ImageFileType::PNG:
-		if (1 == pngLoadPtr((const unsigned char *)data, size, &width[0], &height[0], &levels[0])) {
+		if (1 == pngLoadPtr((const unsigned char *)data, size, &width[0], &height[0], &levels[0], maxWidth, maxHeight)) {
 			numLevels = 1;
 			fmt = Draw::DataFormat::R8G8B8A8_UNORM;
 			if (!levels[0]) {
@@ -169,24 +169,24 @@ Draw::Texture *CreateTextureFromTempImage(Draw::DrawContext *draw, const TempIma
 	return draw->CreateTexture(desc);
 }
 
-Draw::Texture *CreateTextureFromFileData(Draw::DrawContext *draw, const uint8_t *data, size_t dataSize, ImageFileType type, bool generateMips, const char *name) {
+Draw::Texture *CreateTextureFromFileData(Draw::DrawContext *draw, const uint8_t *data, size_t dataSize, ImageFileType type, bool generateMips, const char *name, int maxWidth, int maxHeight) {
 	TempImage image;
-	if (!image.LoadTextureLevelsFromFileData(data, dataSize, type)) {
+	if (!image.LoadTextureLevelsFromFileData(data, dataSize, type, maxWidth, maxHeight)) {
 		return nullptr;
 	}
 	Draw::Texture *texture = CreateTextureFromTempImage(draw, image, generateMips, name);
 	image.Free();
 	return texture;
 }
 
-Draw::Texture *CreateTextureFromFile(Draw::DrawContext *draw, const char *filename, ImageFileType type, bool generateMips) {
+Draw::Texture *CreateTextureFromFile(Draw::DrawContext *draw, const char *filename, ImageFileType type, bool generateMips, int maxWidth, int maxHeight) {
 	size_t fileSize;
 	uint8_t *buffer = g_VFS.ReadFile(filename, &fileSize);
 	if (!buffer) {
 		ERROR_LOG(Log::IO, "Failed to read file '%s'", filename);
 		return nullptr;
 	}
-	Draw::Texture *texture = CreateTextureFromFileData(draw, buffer, fileSize, type, generateMips, filename);
+	Draw::Texture *texture = CreateTextureFromFileData(draw, buffer, fileSize, type, generateMips, filename, maxWidth, maxHeight);
 	delete[] buffer;
 	return texture;
 }

Common/Render/ManagedTexture.h

@@ -32,7 +32,7 @@ struct TempImage {
 	int height[16]{};
 	int numLevels = 0;
 
-	bool LoadTextureLevelsFromFileData(const uint8_t *data, size_t size, ImageFileType typeSuggestion = ImageFileType::DETECT);
+	bool LoadTextureLevelsFromFileData(const uint8_t *data, size_t size, ImageFileType typeSuggestion = ImageFileType::DETECT, int maxWidth = 8192, int maxHeight = 8192);
 	void Free() {
 		if (levels[0]) {
 			free(levels[0]);
@@ -76,8 +76,8 @@ class ManagedTexture {
 	LoadState state_ = LoadState::PENDING;
 };
 
-Draw::Texture *CreateTextureFromFileData(Draw::DrawContext *draw, const uint8_t *data, size_t dataSize, ImageFileType type, bool generateMips, const char *name);
-Draw::Texture *CreateTextureFromFile(Draw::DrawContext *draw, const char *filename, ImageFileType type, bool generateMips);
+Draw::Texture *CreateTextureFromFileData(Draw::DrawContext *draw, const uint8_t *data, size_t dataSize, ImageFileType type, bool generateMips, const char *name, int maxWidth = 8192, int maxHeight = 8192);
+Draw::Texture *CreateTextureFromFile(Draw::DrawContext *draw, const char *filename, ImageFileType type, bool generateMips, int maxWidth = 8192, int maxHeight = 8192);
 Draw::Texture *CreateTextureFromTempImage(Draw::DrawContext *draw, const TempImage &image, bool generateMips, const char *name);
 
 ImageFileType DetectImageFileType(const uint8_t *data, size_t size);

Common/Serialize/SerializeFuncs.h

@@ -94,6 +94,15 @@ template<class T>
 void DoVector(PointerWrap &p, std::vector<T> &x, T &default_val) {
 	u32 vec_size = (u32)x.size();
 	Do(p, vec_size);
+	// Guard against an attacker-controlled size that would both resize the
+	// vector hugely and read past the end of the buffer. sizeof(T) is a lower
+	// bound on the bytes consumed per element for most uses.
+	if (p.mode == PointerWrap::MODE_READ || p.mode == PointerWrap::MODE_VERIFY) {
+		if (vec_size > p.Remaining() / sizeof(T)) {
+			p.SetError(PointerWrap::ERROR_FAILURE);
+			return;
+		}
+	}
 	if (vec_size != x.size())
 		x.resize(vec_size, default_val);
 	if (vec_size > 0)

Common/Serialize/Serializer.cpp

@@ -130,11 +130,21 @@ void PointerWrap::SetError(Error error_) {
 }
 
 bool PointerWrap::ExpectVoid(void *data, int size) {
+	if (size < 0) {
+		SetError(ERROR_FAILURE);
+		return false;
+	}
 	switch (mode) {
-	case MODE_READ:	if (memcmp(data, *ptr, size) != 0) return false; break;
+	case MODE_READ:
+		if (!CheckRead(size))
+			return false;
+		if (memcmp(data, *ptr, size) != 0) return false;
+		break;
 	case MODE_WRITE: memcpy(*ptr, data, size); break;
 	case MODE_MEASURE: break;  // MODE_MEASURE - don't need to do anything
 	case MODE_VERIFY:
+		if (!CheckRead(size))
+			return false;
 		for (int i = 0; i < size; i++)
 			_dbg_assert_msg_(((u8*)data)[i] == (*ptr)[i], "Savestate verification failure: %d (0x%X) (at %p) != %d (0x%X) (at %p).\n", ((u8*)data)[i], ((u8*)data)[i], &((u8*)data)[i], (*ptr)[i], (*ptr)[i], &(*ptr)[i]);
 		break;
@@ -145,11 +155,21 @@ bool PointerWrap::ExpectVoid(void *data, int size) {
 }
 
 void PointerWrap::DoVoid(void *data, int size) {
+	if (size < 0) {
+		SetError(ERROR_FAILURE);
+		return;
+	}
 	switch (mode) {
-	case MODE_READ:	memcpy(data, *ptr, size); break;
+	case MODE_READ:
+		if (!CheckRead(size))
+			return;
+		memcpy(data, *ptr, size);
+		break;
 	case MODE_WRITE: memcpy(*ptr, data, size); break;
 	case MODE_MEASURE: break;  // MODE_MEASURE - don't need to do anything
 	case MODE_VERIFY:
+		if (!CheckRead(size))
+			return;
 		for (int i = 0; i < size; i++)
 			_dbg_assert_msg_(((u8*)data)[i] == (*ptr)[i], "Savestate verification failure: %d (0x%X) (at %p) != %d (0x%X) (at %p).\n", ((u8*)data)[i], ((u8*)data)[i], &((u8*)data)[i], (*ptr)[i], (*ptr)[i], &(*ptr)[i]);
 		break;
@@ -170,6 +190,11 @@ void Do(PointerWrap &p, std::string &x) {
 		p.SetError(PointerWrap::ERROR_FAILURE);
 		return;
 	}
+	// Ensure the whole string (including NUL terminator) is within bounds before reading.
+	if (p.mode == PointerWrap::MODE_READ || p.mode == PointerWrap::MODE_VERIFY) {
+		if (!p.CheckRead(stringLen))
+			return;
+	}
 
 	switch (p.mode) {
 	case PointerWrap::MODE_READ: x = (char*)*p.ptr; break;
@@ -190,6 +215,11 @@ void Do(PointerWrap &p, std::wstring &x) {
 		p.SetError(PointerWrap::ERROR_FAILURE);
 		return;
 	}
+	// Ensure the whole string is within bounds before reading.
+	if (p.mode == PointerWrap::MODE_READ || p.mode == PointerWrap::MODE_VERIFY) {
+		if (!p.CheckRead(stringLen))
+			return;
+	}
 
 	auto read = [&]() {
 		std::wstring r;
@@ -218,6 +248,11 @@ void Do(PointerWrap &p, std::u16string &x) {
 		p.SetError(PointerWrap::ERROR_FAILURE);
 		return;
 	}
+	// Ensure the whole string is within bounds before reading.
+	if (p.mode == PointerWrap::MODE_READ || p.mode == PointerWrap::MODE_VERIFY) {
+		if (!p.CheckRead(stringLen))
+			return;
+	}
 
 	auto read = [&]() {
 		std::u16string r;
@@ -370,6 +405,13 @@ CChunkFileReader::Error CChunkFileReader::LoadFile(const Path &filename, std::st
 	}
 
 	if (header.Compress) {
+		// Sanity cap on the decompressed size to avoid a giant allocation from
+		// an attacker-controlled header field. Real savestates are well under this.
+		if (header.UncompressedSize > 0x40000000) {
+			ERROR_LOG(Log::SaveState, "ChunkReader: UncompressedSize too large: %u", header.UncompressedSize);
+			delete [] buffer;
+			return ERROR_BAD_FILE;
+		}
 		u8 *uncomp_buffer = new u8[header.UncompressedSize];
 		size_t uncomp_size = header.UncompressedSize;
 		bool success = false;

Common/Serialize/Serializer.h

@@ -33,6 +33,7 @@
 #include <cstring>
 #include <vector>
 #include <cstdlib>
+#include <cstdint>
 
 #include "Common/CommonTypes.h"
 #include "Common/Log.h"
@@ -153,10 +154,37 @@ class PointerWrap
 
 	size_t Offset() const { return *ptr - ptrStart_; }
 
+	// Restrict reads (MODE_READ / MODE_VERIFY) to not go past the end of the
+	// buffer. Not required for write/measure, but harmless to set.
+	void SetReadEnd(u8 *end) { end_ = end; }
+
+	// Number of bytes left before the end of the read buffer, or SIZE_MAX if
+	// no end was set. Only meaningful in MODE_READ/MODE_VERIFY.
+	size_t Remaining() const {
+		if (!end_) {
+			return SIZE_MAX;
+		}
+		if (*ptr >= end_) {
+			return 0;
+		}
+		return (size_t)(end_ - *ptr);
+	}
+
+	// Returns true if we can safely read/compare 'size' more bytes. On
+	// failure, marks an error and switches to MODE_NOOP.
+	bool CheckRead(size_t size) {
+		if (end_ && size > Remaining()) {
+			SetError(ERROR_FAILURE);
+			return false;
+		}
+		return true;
+	}
+
 private:
 	const char *firstBadSectionTitle_ = nullptr;
 	const char *curTitle_;
 	u8 *ptrStart_;
+	u8 *end_ = nullptr;
 	std::vector<SerializeCheckpoint> checkpoints_;
 	size_t curCheckpoint_ = 0;
 	size_t measuredSize_ = 0;
@@ -174,9 +202,10 @@ class CChunkFileReader
 
 	// May fail badly if ptr doesn't point to valid data.
 	template<class T>
-	static Error LoadPtr(u8 *ptr, T &_class, std::string *errorString)
+	static Error LoadPtr(u8 *ptr, size_t size, T &_class, std::string *errorString)
 	{
 		PointerWrap p(&ptr, PointerWrap::MODE_READ);
+		p.SetReadEnd(ptr + size);
 		_class.DoState(p);
 
 		if (p.error != p.ERROR_FAILURE) {
@@ -267,7 +296,7 @@ class CChunkFileReader
 		Error error = LoadFile(filename, gitVersion, ptr, sz, failureReason);
 		if (error == ERROR_NONE) {
 			failureReason->clear();
-			error = LoadPtr(ptr, _class, failureReason);
+			error = LoadPtr(ptr, sz, _class, failureReason);
 			delete [] ptr;
 			INFO_LOG(Log::SaveState, "ChunkReader: Done loading '%s'", filename.c_str());
 		} else {
@@ -311,6 +340,9 @@ class CChunkFileReader
 		p.SetMode(PointerWrap::MODE_VERIFY);
 		_class.DoState(p);
 
+		if (p.error == PointerWrap::ERROR_FAILURE) {
+			return ERROR_BROKEN_STATE;
+		}
 		return ERROR_NONE;
 	}
 

Common/UI/IconCache.cpp

@@ -312,7 +312,7 @@ Draw::Texture *IconCache::BindIconTexture(UIContext *context, std::string_view k
 	case IconFormat::PNG:
 	{
 		const std::string &data = entry.data;
-		int result = pngLoadPtr((const unsigned char *)data.data(), data.size(), &width, &height, &buffer);
+		int result = pngLoadPtr((const unsigned char *)data.data(), data.size(), &width, &height, &buffer, 256, 128);
 
 		if (result != 1) {
 			ERROR_LOG(Log::G3D, "IconCache: Failed to load png (%d bytes) for key %.*s", (int)data.size(), STR_VIEW(key));

Core/FileLoaders/CachingFileLoader.cpp

@@ -229,7 +229,7 @@ bool CachingFileLoader::MakeCacheSpaceFor(size_t blocks, bool readingAhead) {
 			// 0 means it was never used yet or was the first read (e.g. block descriptor.)
 			if (it->second.generation == oldestGeneration_ || it->second.generation == 0) {
 				s64 pos = it->first;
-				delete it->second.ptr;
+				delete [] it->second.ptr;
 				blocks_.erase(it);
 				--cacheSize_;
 

Core/FileLoaders/HTTPFileLoader.cpp

@@ -254,7 +254,10 @@ size_t HTTPFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags f
 		return 0;
 	}
 
-	size_t readBytes = output.size();
+	// Never trust the entity length: a malicious/MITM'd server can claim a
+	// matching Content-Range but send a larger body. Clamp to what we
+	// requested so we can't overflow the caller's fixed-size buffer.
+	size_t readBytes = std::min(output.size(), (size_t)(absoluteEnd - absolutePos));
 	output.Take(readBytes, (char *)data);
 	filepos_ = absolutePos + readBytes;
 	return readBytes;