Add bounds checking to savestate deserialization

PointerWrap tracked no end-of-buffer, so DoState() implementations could
read past the end of a crafted or truncated savestate via DoVoid's
unchecked memcpy, and DoVector could resize to an attacker-controlled
size before reading.

- PointerWrap now tracks a read end; DoVoid/ExpectVoid fail (MODE_NOOP)
  before reading out of bounds.
- String reads are bounds-checked for the whole string including NUL.
- DoVector rejects sizes that can't fit in the remaining buffer.
- LoadPtr takes the buffer size and sets the read end.
- Capping the decompression buffer allocation in LoadFile.

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 @@ 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<SerializeCheckpoint> 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<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 @@ 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 {

Core/SaveState.cpp

@@ -125,7 +125,7 @@ int g_screenshotFailures;
 
 	CChunkFileReader::Error LoadFromRam(std::vector<u8> &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?

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)