diff --git a/Common/Serialize/SerializeFuncs.h b/Common/Serialize/SerializeFuncs.h index ef99b6db89..6d2ff1d914 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 868231d2d2..f572457c80 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 e15ffd9704..3c91230d6c 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 @@ public: 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 @@ public: // 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 @@ public: 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 { diff --git a/Core/SaveState.cpp b/Core/SaveState.cpp index cdc7843f5d..a8d0c6c630 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/libretro/libretro.cpp b/libretro/libretro.cpp index 0130f4b93b..b44c55cc7b 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)