diff --git a/.github/workflows/appimage.yml b/.github/workflows/appimage.yml index b55122a2421e..1630f0f7f9f8 100644 --- a/.github/workflows/appimage.yml +++ b/.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 }} diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 4f337fe837dd..925cefd16eac 100644 --- a/.github/workflows/build.yml +++ b/.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: | diff --git a/.github/workflows/manual_generate_apk.yml b/.github/workflows/manual_generate_apk.yml index e42754511f03..7a36d50c36d6 100644 --- a/.github/workflows/manual_generate_apk.yml +++ b/.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' diff --git a/.github/workflows/tarball.yml b/.github/workflows/tarball.yml index ce2a4bdaef55..5d2ba78712e7 100644 --- a/.github/workflows/tarball.yml +++ b/.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 }} diff --git a/Common/Data/Format/PNGLoad.cpp b/Common/Data/Format/PNGLoad.cpp index 8a0834b480c0..6f843ba319c8 100644 --- a/Common/Data/Format/PNGLoad.cpp +++ b/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) { diff --git a/Common/Data/Format/PNGLoad.h b/Common/Data/Format/PNGLoad.h index 85057d2278a2..4bd7f3c2b5de 100644 --- a/Common/Data/Format/PNGLoad.h +++ b/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 diff --git a/Common/Render/ManagedTexture.cpp b/Common/Render/ManagedTexture.cpp index 90de360d79f2..bc36a2aca543 100644 --- a/Common/Render/ManagedTexture.cpp +++ b/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,9 +169,9 @@ 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); @@ -179,14 +179,14 @@ Draw::Texture *CreateTextureFromFileData(Draw::DrawContext *draw, const uint8_t 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; } diff --git a/Common/Render/ManagedTexture.h b/Common/Render/ManagedTexture.h index 0ac621179695..af030b15c749 100644 --- a/Common/Render/ManagedTexture.h +++ b/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); diff --git a/Common/Serialize/SerializeFuncs.h b/Common/Serialize/SerializeFuncs.h index ef99b6db8906..6d2ff1d914cf 100644 --- a/Common/Serialize/SerializeFuncs.h +++ b/Common/Serialize/SerializeFuncs.h @@ -94,6 +94,15 @@ template void DoVector(PointerWrap &p, std::vector &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) diff --git a/Common/Serialize/Serializer.cpp b/Common/Serialize/Serializer.cpp index 868231d2d2d4..f572457c80c9 100644 --- a/Common/Serialize/Serializer.cpp +++ b/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; diff --git a/Common/Serialize/Serializer.h b/Common/Serialize/Serializer.h index e15ffd970418..89a512c26d20 100644 --- a/Common/Serialize/Serializer.h +++ b/Common/Serialize/Serializer.h @@ -33,6 +33,7 @@ #include #include #include +#include #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 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 - 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; } diff --git a/Common/UI/IconCache.cpp b/Common/UI/IconCache.cpp index 85e027009731..598aad75aa6e 100644 --- a/Common/UI/IconCache.cpp +++ b/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)); diff --git a/Core/FileLoaders/CachingFileLoader.cpp b/Core/FileLoaders/CachingFileLoader.cpp index 472d10973ebf..c35b87956594 100644 --- a/Core/FileLoaders/CachingFileLoader.cpp +++ b/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_; diff --git a/Core/FileLoaders/HTTPFileLoader.cpp b/Core/FileLoaders/HTTPFileLoader.cpp index 6e4489bb8cf0..8830994ee6a6 100644 --- a/Core/FileLoaders/HTTPFileLoader.cpp +++ b/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; diff --git a/Core/Font/PGF.cpp b/Core/Font/PGF.cpp index 89d6d019df69..1b6ef4809f38 100644 --- a/Core/Font/PGF.cpp +++ b/Core/Font/PGF.cpp @@ -187,12 +187,37 @@ bool PGF::ReadPtr(const u8 *ptr, size_t dataSize) { fileName = header.fontName; if (header.revision == 3) { + if (dataSize < sizeof(header) + sizeof(rev3extra)) { + return false; + } memcpy(&rev3extra, ptr, sizeof(rev3extra)); rev3extra.compCharMapLength1 &= 0xFFFF; rev3extra.compCharMapLength2 &= 0xFFFF; ptr += sizeof(rev3extra); } + // Validate that all tables fit in the input buffer before reading any of + // them. Use 64-bit arithmetic: the original 32-bit signed size math could + // overflow for crafted lengths. + const u64 headerSize = (u64)(ptr - startPtr); + const u64 tablesSize = ((u64)header.dimTableLength + header.xAdjustTableLength + header.yAdjustTableLength + header.advanceTableLength) * 8; + const u64 shadowCharMapSize = (((u64)header.shadowMapLength * header.shadowMapBpe + 31) & ~31ull) / 8; + const u64 compTableSize = header.revision == 3 ? ((u64)rev3extra.compCharMapLength1 + rev3extra.compCharMapLength2) * 4 : 0; + const u64 charMapSize = (((u64)header.charMapLength * header.charMapBpe + 31) & ~31ull) / 8; + const u64 charPointerSize = (((u64)header.charPointerLength * header.charPointerBpe + 31) & ~31ull) / 8; + + // Also cap the lengths so a crafted font can't force absurd allocations + // or loops downstream. Real PGF fonts are tiny. + if (header.charPointerLength < 0 || header.charPointerLength > 0x100000 || + header.charMapLength < 0 || header.charMapLength > 0x100000 || + header.shadowMapLength < 0 || header.shadowMapLength > 0x10000) { + return false; + } + + if (headerSize + tablesSize + shadowCharMapSize + compTableSize + charMapSize + charPointerSize > dataSize) { + return false; + } + const u32_le *wptr = (const u32_le *)ptr; dimensionTable[0].resize(header.dimTableLength); dimensionTable[1].resize(header.dimTableLength); @@ -224,7 +249,6 @@ bool PGF::ReadPtr(const u8 *ptr, size_t dataSize) { const u8 *uptr = (const u8 *)wptr; - int shadowCharMapSize = ((header.shadowMapLength * header.shadowMapBpe + 31) & ~31) / 8; const u8 *shadowCharMap = uptr; uptr += shadowCharMapSize; @@ -251,11 +275,9 @@ bool PGF::ReadPtr(const u8 *ptr, size_t dataSize) { uptr = (const u8 *)sptr; - int charMapSize = ((header.charMapLength * header.charMapBpe + 31) & ~31) / 8; const u8 *charMap = uptr; uptr += charMapSize; - int charPointerSize = (((header.charPointerLength * header.charPointerBpe + 31) & ~31) / 8); const u8 *charPointerTable = uptr; uptr += charPointerSize; @@ -290,9 +312,14 @@ bool PGF::ReadPtr(const u8 *ptr, size_t dataSize) { std::vector charPointers = getTable(charPointerTable, header.charPointerBpe, glyphs.size()); std::vector shadowMap = getTable(shadowCharMap, header.shadowMapBpe, (s32)header.shadowMapLength); - // Pregenerate glyphs. + // Pregenerate glyphs. charPointers come from the (attacker-controlled) + // char pointer table, so their offsets into fontData must be validated. + // ReadCharGlyph/ReadShadowGlyph bounds-check charPtr internally. for (size_t i = 0; i < glyphs.size(); i++) { - ReadCharGlyph(fontData, charPointers[i] * 4 * 8 /* ??? */, glyphs[i]); + if (charPointers[i] < 0) + continue; + size_t charPtr = (size_t)charPointers[i] * 4 * 8; + ReadCharGlyph(fontData, charPtr, glyphs[i]); } // And shadow glyphs. @@ -300,9 +327,10 @@ bool PGF::ReadPtr(const u8 *ptr, size_t dataSize) { size_t shadowId = glyphs[i].shadowID; if (shadowId < shadowMap.size()) { size_t charId = shadowMap[shadowId]; - if (charId < shadowGlyphs.size()) { + if (charId < shadowGlyphs.size() && charPointers[charId] >= 0) { + size_t charPtr = (size_t)charPointers[charId] * 4 * 8; // TODO: check for pre existing shadow glyph - ReadShadowGlyph(fontData, charPointers[charId] * 4 * 8 /* ??? */, shadowGlyphs[charId]); + ReadShadowGlyph(fontData, charPtr, shadowGlyphs[charId]); } } } @@ -419,6 +447,13 @@ bool PGF::ReadShadowGlyph(const u8 *fontdata, size_t charPtr, Glyph &glyph) { } bool PGF::ReadCharGlyph(const u8 *fontdata, size_t charPtr, Glyph &glyph) { + // The glyph header reads below stay within a few hundred bits, but + // validate the offset here so this function is safe regardless of caller. + // charPtr is a bit offset; fontDataSize is in bytes. + if (charPtr + 1024 > (size_t)fontDataSize * 8) { + return false; + } + // Skip size. charPtr += 14; diff --git a/Core/HLE/AtracCtx2.cpp b/Core/HLE/AtracCtx2.cpp index 2a41b15e706c..8a6162fd2798 100644 --- a/Core/HLE/AtracCtx2.cpp +++ b/Core/HLE/AtracCtx2.cpp @@ -217,6 +217,11 @@ int InitContextFromTrackInfo(SceAtracContext *ctx, const TrackInfo *wave, u32 bu (ctx->info).curBuffer = 0; (ctx->info).bufferByte = bufferSize; (ctx->info).streamOff = dataOff; + // A packet larger than the buffer can't be streamed or assembled into the + // SAS assembly buffer. Reject it early, as sampleSize is file-derived. + if ((ctx->info).sampleSize > (u32)bufferSize) { + return SCE_ERROR_ATRAC_BAD_CODEC_PARAMS; + } if ((ctx->info).loopEnd > endSample) { return SCE_ERROR_ATRAC_BAD_CODEC_PARAMS; } @@ -949,9 +954,13 @@ u32 Atrac2::DecodeInternal(u32 outbufAddr, int *SamplesNum, int *finish) { info.curBuffer = 1; info.streamDataByte = info.secondBufferByte; info.secondStreamOff = 0; + // Clamp the copy to the main buffer size; sampleSize is file-derived and could be larger. + size_t copyLen = info.secondBufferByte % info.sampleSize; + if (copyLen > info.bufferByte) + copyLen = info.bufferByte; memcpy(Memory::GetPointerWrite(info.buffer), Memory::GetPointer(info.secondBuffer + (info.secondBufferByte - info.secondBufferByte % info.sampleSize)), - info.secondBufferByte % info.sampleSize); + copyLen); } } } @@ -966,7 +975,9 @@ int Atrac2::SetData(const Track &track, u32 bufferAddr, u32 readSize, u32 buffer // Turns out that games can abuse bufferSize, so we can't verify that it's a valid length with GetPointerRange. const u8 *bufferPtr = Memory::GetPointerUnchecked(bufferAddr); if (!Memory::IsValidRange(bufferAddr, readSize)) { - WARN_LOG(Log::Atrac, "Atrac2::SetData: Bad buffer range %08x+%08x - however, proceeeding.", bufferAddr, readSize); + WARN_LOG(Log::Atrac, "Atrac2::SetData: Bad buffer range %08x+%08x - clamping to mapped size.", bufferAddr, readSize); + // Clamp so the parsers below can't read past the mapped region. + readSize = Memory::ClampValidSizeAt(bufferAddr, readSize); } if (!isAA3) { int retval = ParseWaveAT3(bufferPtr, readSize, &trackInfo); @@ -1215,7 +1226,10 @@ void Atrac2::DecodeForSas(s16 *dstData, int *bytesWritten, int *finish) { 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. + // assembly buffer below. InitContextFromTrackInfo already rejects + // sampleSize > bufferByte, but a crafted file can still pass that with + // a large buffer, so also guard against sampleSize exceeding the fixed + // assembly buffer here. if ((u32)info.sampleSize > sizeof(assembly)) { ERROR_LOG(Log::Atrac, "SAS packet too large for assembly buffer: %d", info.sampleSize); *bytesWritten = 0; diff --git a/Core/HLE/sceAtrac.cpp b/Core/HLE/sceAtrac.cpp index 37885144cd6a..abf0f40af245 100644 --- a/Core/HLE/sceAtrac.cpp +++ b/Core/HLE/sceAtrac.cpp @@ -1093,6 +1093,7 @@ u32 AtracSasAddStreamData(int atracID, u32 bufPtr, u32 bytesToAdd) { AtracBase *atrac = getAtrac(atracID); if (!atrac) { WARN_LOG(Log::Atrac, "bad atrac ID"); + return 0; } return atrac->EnqueueForSas(bufPtr, bytesToAdd); } diff --git a/Core/HLE/sceKernelModule.cpp b/Core/HLE/sceKernelModule.cpp index 5f43e9d2735d..714b3c48310e 100644 --- a/Core/HLE/sceKernelModule.cpp +++ b/Core/HLE/sceKernelModule.cpp @@ -835,8 +835,8 @@ static bool KernelImportModuleFuncs(PSPModule *module, u32 *firstImportStubAddr, entryPos += entry->size; const char *modulename; - if (Memory::IsValidAddress(entry->name)) { - modulename = Memory::GetCharPointer(entry->name); + if (Memory::IsValidNullTerminatedString(entry->name)) { + modulename = Memory::GetCharPointerUnchecked(entry->name); } else { modulename = "(invalidname)"; needReport = true; @@ -932,7 +932,9 @@ static bool KernelImportModuleFuncs(PSPModule *module, u32 *firstImportStubAddr, char temp[512]; const char *modulename; - if (Memory::IsValidAddress(entry->name)) { + // Check for NUL termination within the mapped region so %s below + // can't read past guest RAM on a crafted, unterminated name. + if (Memory::IsValidNullTerminatedString(entry->name)) { modulename = Memory::GetCharPointerUnchecked(entry->name); } else { modulename = "(invalidname)"; diff --git a/Core/HLE/sceKernelThread.cpp b/Core/HLE/sceKernelThread.cpp index 52f33bd72499..567377b6240f 100644 --- a/Core/HLE/sceKernelThread.cpp +++ b/Core/HLE/sceKernelThread.cpp @@ -2625,15 +2625,14 @@ int sceKernelReleaseWaitThread(SceUID threadID) { if (!t) { return hleLogError(Log::sceKernel, error, "bad thread ID"); } else { - if (!t->isWaiting()) - return hleLogError(Log::sceKernel, SCE_KERNEL_ERROR_NOT_WAIT); - if (t->nt.waitType == WAITTYPE_HLEDELAY) - { + if (!t->isWaiting()) { + return hleLogInfo(Log::sceKernel, SCE_KERNEL_ERROR_NOT_WAIT); + } + if (t->nt.waitType == WAITTYPE_HLEDELAY) { WARN_LOG_REPORT_ONCE(rwt_delay, Log::sceKernel, "sceKernelReleaseWaitThread(): Refusing to wake HLE-delayed thread, right thing to do?"); return hleNoLog(SCE_KERNEL_ERROR_NOT_WAIT); } - if (t->nt.waitType == WAITTYPE_MODULE) - { + if (t->nt.waitType == WAITTYPE_MODULE) { WARN_LOG_REPORT_ONCE(rwt_sm, Log::sceKernel, "sceKernelReleaseWaitThread(): Refusing to wake start_module thread, right thing to do?"); return hleNoLog(SCE_KERNEL_ERROR_NOT_WAIT); } diff --git a/Core/HLE/scePsmf.cpp b/Core/HLE/scePsmf.cpp index a8f3bf48b6f8..fcb57013b984 100644 --- a/Core/HLE/scePsmf.cpp +++ b/Core/HLE/scePsmf.cpp @@ -290,9 +290,21 @@ class PsmfStream { videoHeight_ = addr[13] * 16; const u32 EP_MAP_STRIDE = 1 + 1 + 4 + 4; - if (psmf->headerOffset != 0 && !Memory::IsValidRange(psmf->headerOffset, psmf->EPMapOffset + EP_MAP_STRIDE * psmf->EPMapEntriesNum)) { - ERROR_LOG(Log::ME, "Invalid PSMF EP map entry count: %d", psmf->EPMapEntriesNum); - psmf->EPMapEntriesNum = Memory::ClampValidSizeAt(psmf->headerOffset + psmf->EPMapOffset, EP_MAP_STRIDE * psmf->EPMapEntriesNum) / EP_MAP_STRIDE; + // Compute in 64-bit so EP_MAP_STRIDE * EPMapEntriesNum can't overflow + // and pass a wrapped range check while the loop below still iterates + // the unwrapped count. + const u64 epMapBytes = EP_MAP_STRIDE * (u64)psmf->EPMapEntriesNum; + if (psmf->headerOffset != 0) { + if (epMapBytes > 0xFFFFFFFFull - psmf->EPMapOffset || !Memory::IsValidRange(psmf->headerOffset, psmf->EPMapOffset + (u32)epMapBytes)) { + ERROR_LOG(Log::ME, "Invalid PSMF EP map entry count: %d", psmf->EPMapEntriesNum); + psmf->EPMapEntriesNum = Memory::ClampValidSizeAt(psmf->headerOffset + psmf->EPMapOffset, (u32)epMapBytes) / EP_MAP_STRIDE; + } + } else { + // No guest address to validate against (player tempbuf path, + // where the buffer is 64KB): cap so the reads below stay in bounds. + if (epMapBytes > 0x10000) { + psmf->EPMapEntriesNum = 0x10000 / EP_MAP_STRIDE; + } } psmf->EPMap.clear(); diff --git a/Core/PSPLoaders.cpp b/Core/PSPLoaders.cpp index f81ce2e220fe..3be69e371b02 100644 --- a/Core/PSPLoaders.cpp +++ b/Core/PSPLoaders.cpp @@ -425,8 +425,16 @@ bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string_view discId, bool load // Migrate old save states from old versions of fake game IDs. // Ugh, this might actually be slow on Android. + // The strings here are attacker-controlled (from PARAM.SFO / filenames), so + // if any of them contain a path separator, skip the migration to avoid + // building traversal paths. + const bool anyPathSeparator = + discID.find('/') != std::string::npos || discID.find('\\') != std::string::npos || + discVersion.find('/') != std::string::npos || discVersion.find('\\') != std::string::npos || + homebrewName.find('/') != std::string::npos || homebrewName.find('\\') != std::string::npos || + madeUpID.find('/') != std::string::npos || madeUpID.find('\\') != std::string::npos; const Path savestateDir = GetSysDirectory(DIRECTORY_SAVESTATE); - for (int i = 0; i < 5; ++i) { + for (int i = 0; i < 5 && !anyPathSeparator; ++i) { Path newPrefix = savestateDir / StringFromFormat("%s_%s_%d", discID.c_str(), discVersion.c_str(), i); Path oldNamePrefix = savestateDir / StringFromFormat("%s_%d", homebrewName.c_str(), i); Path oldIDPrefix = savestateDir / StringFromFormat("%s_1.00_%d", madeUpID.c_str(), i); diff --git a/Core/SaveState.cpp b/Core/SaveState.cpp index cdc7843f5d1e..a8d0c6c630c5 100644 --- a/Core/SaveState.cpp +++ b/Core/SaveState.cpp @@ -125,7 +125,7 @@ int g_screenshotFailures; CChunkFileReader::Error LoadFromRam(std::vector &data, std::string *errorString) { SaveStart state; - return CChunkFileReader::LoadPtr(&data[0], state, errorString); + return CChunkFileReader::LoadPtr(&data[0], data.size(), state, errorString); } // TODO: Should this be configurable? diff --git a/Core/Util/AtracTrack.cpp b/Core/Util/AtracTrack.cpp index 65044fd05595..60fecd5eea67 100644 --- a/Core/Util/AtracTrack.cpp +++ b/Core/Util/AtracTrack.cpp @@ -84,7 +84,14 @@ int AnalyzeAtracTrack(const u8 *buffer, u32 size, Track *track, std::string *err track->fileSize = Read32(buffer, offset - 8) + 8; // Even if the RIFF size is too low, it may simply be incorrect. This works on real firmware. + // But the reads below must stay within mapped guest memory: clamp the + // parse bound to the actual mapped region at the buffer, so a crafted, + // inflated RIFF size can't push the reads past the end of RAM. u32 maxSize = std::max(track->fileSize, size); + const u32 bufferAddr = Memory::GetAddressFromHostPointer(buffer); + if (bufferAddr != 0) { + maxSize = std::min(maxSize, Memory::MaxSizeAtAddress(bufferAddr)); + } bool bfoundData = false; u32 dataChunkSize = 0; @@ -375,6 +382,12 @@ int ParseWaveAT3(const u8 *data, u32 dataLength, TrackInfo *track) { // We found the WAVE header. break; } + // Guard against underflow (blockSize < 4) making the offset negative + // and bypassing the loop bounds check, and against advancing past the + // end of the buffer. + if (blockSize < 4 || (u64)offset + blockSize - 4 > dataLength) { + return SCE_ERROR_ATRAC_SIZE_TOO_SMALL; + } offset += blockSize - 4; } diff --git a/Core/Util/VideoPlayer.cpp b/Core/Util/VideoPlayer.cpp index 87444ed89efe..7d5b14034e20 100644 --- a/Core/Util/VideoPlayer.cpp +++ b/Core/Util/VideoPlayer.cpp @@ -141,6 +141,14 @@ int pmf_init(PMFPlayer* ps, const uint8_t* data, size_t size, int* out_w, int* o AVCodec* codec = avcodec_find_decoder(ps->video_ctx->codec_id); if (!codec || avcodec_open2(ps->video_ctx, codec, nullptr) < 0) return -1; + // Reject absurd dimensions: PMF video on PSP never exceeds 720x480, and + // larger values could overflow the caller's frame buffer allocation + // (width * height * 4) and drive huge sws_scale writes. + if (ps->video_ctx->width <= 0 || ps->video_ctx->height <= 0 || + ps->video_ctx->width > 720 || ps->video_ctx->height > 480) { + return -1; + } + ps->frame = av_frame_alloc(); ps->rgb_frame = av_frame_alloc(); ps->last_pts = -1.0; diff --git a/UI/GameInfoCache.cpp b/UI/GameInfoCache.cpp index 2b7d22dfcf82..2031c8635d23 100644 --- a/UI/GameInfoCache.cpp +++ b/UI/GameInfoCache.cpp @@ -418,7 +418,7 @@ void GameInfo::FinishPendingTextureLoads(Draw::DrawContext *draw) { return; } if (icon.dataLoaded && !icon.texture) { - SetupTexture(draw, icon); + SetupTexture(draw, icon, 256, 128); } if (pic0.dataLoaded && !pic0.texture) { SetupTexture(draw, pic0); @@ -428,7 +428,7 @@ void GameInfo::FinishPendingTextureLoads(Draw::DrawContext *draw) { } } -void GameInfo::SetupTexture(Draw::DrawContext *thin3d, GameInfoTex &tex) { +void GameInfo::SetupTexture(Draw::DrawContext *thin3d, GameInfoTex &tex, int maxWidth, int maxHeight) { if (tex.timeLoaded) { // Failed before, skip. return; @@ -440,7 +440,7 @@ void GameInfo::SetupTexture(Draw::DrawContext *thin3d, GameInfoTex &tex) { using namespace Draw; // TODO: Use TempImage to semi-load the image in the worker task, then here we // could just call CreateTextureFromTempImage. - tex.texture = CreateTextureFromFileData(thin3d, (const uint8_t *)tex.data.data(), tex.data.size(), ImageFileType::DETECT, false, GetTitle().c_str()); + tex.texture = CreateTextureFromFileData(thin3d, (const uint8_t *)tex.data.data(), tex.data.size(), ImageFileType::DETECT, false, GetTitle().c_str(), maxWidth, maxHeight); tex.timeLoaded = time_now_d(); if (!tex.texture) { ERROR_LOG(Log::G3D, "Failed creating texture (%s) from %d-byte file", GetTitle().c_str(), (int)tex.data.size()); diff --git a/UI/GameInfoCache.h b/UI/GameInfoCache.h index f3fa0737aec1..39cf389e1782 100644 --- a/UI/GameInfoCache.h +++ b/UI/GameInfoCache.h @@ -181,7 +181,7 @@ class GameInfo { std::shared_ptr fileLoader; Path filePath_; - void SetupTexture(Draw::DrawContext *draw, GameInfoTex &tex); + void SetupTexture(Draw::DrawContext *draw, GameInfoTex &tex, int maxWidth = 8192, int maxHeight = 8192); private: DISALLOW_COPY_AND_ASSIGN(GameInfo); diff --git a/UI/GameScreen.cpp b/UI/GameScreen.cpp index d7c55d218ce4..a7c4584e2af0 100644 --- a/UI/GameScreen.cpp +++ b/UI/GameScreen.cpp @@ -113,7 +113,9 @@ class PMFView : public UI::InertView { Draw::DrawContext *draw = dc.GetDrawContext(); - std::vector frame(width_ * height_ * 4); + // Dimensions are capped by pmf_init, but use size_t arithmetic anyway + // so a regression can't overflow the allocation. + std::vector frame((size_t)width_ * (size_t)height_ * 4); if (pmf_update(player_, startTime_.ElapsedSeconds(), frame.data())) { if (curFrame_) { curFrame_->Release(); diff --git a/libretro/libretro.cpp b/libretro/libretro.cpp index 0130f4b93b86..b44c55cc7ba5 100644 --- a/libretro/libretro.cpp +++ b/libretro/libretro.cpp @@ -1802,7 +1802,7 @@ bool retro_unserialize(const void *data, size_t size) std::string errorString; SaveState::SaveStart state; - bool retVal = CChunkFileReader::LoadPtr((u8 *)data, state, &errorString) + bool retVal = CChunkFileReader::LoadPtr((u8 *)data, size, state, &errorString) == CChunkFileReader::ERROR_NONE; if (useEmuThread)