From d7d9348243ea203c88a5798c2cbbc70c0ee93ba4 Mon Sep 17 00:00:00 2001
From: Kari Salminen
Date: Fri, 13 Jun 2008 05:57:07 +0000
Subject: Made CineUnpacker::unpack more robust & secure. It shouldn't crash
 now with any input (Before making reading or writing operations they are
 checked to be in bounds). Also updated some comments and added some error
 message(s).

svn-id: r32687
---
 engines/cine/part.cpp   |  6 ++++--
 engines/cine/unpack.cpp | 37 ++++++++++++++++++++++++++++---------
 engines/cine/unpack.h   | 16 +++++++++++-----
 3 files changed, 43 insertions(+), 16 deletions(-)

(limited to 'engines')

diff --git a/engines/cine/part.cpp b/engines/cine/part.cpp
index 535bf841d5..b39f1eff7d 100644
--- a/engines/cine/part.cpp
+++ b/engines/cine/part.cpp
@@ -128,7 +128,7 @@ void CineEngine::readVolCnf() {
 	f.read(buf, packedSize);
 	if (packedSize != unpackedSize) {
 		CineUnpacker cineUnpacker;
-		if (!cineUnpacker.unpack(buf, buf, packedSize)) {
+		if (!cineUnpacker.unpack(buf, packedSize, buf, unpackedSize)) {
 			error("Error while unpacking 'vol.cnf' data");
 		}
 	}
@@ -226,7 +226,9 @@ byte *readBundleFile(int16 foundFileIdx) {
 		byte *unpackBuffer = (byte *)malloc(partBuffer[foundFileIdx].packedSize);
 		readFromPart(foundFileIdx, unpackBuffer);
 		CineUnpacker cineUnpacker;
-		cineUnpacker.unpack(dataPtr, unpackBuffer, partBuffer[foundFileIdx].packedSize);
+		if (!cineUnpacker.unpack(unpackBuffer, partBuffer[foundFileIdx].packedSize, dataPtr, partBuffer[foundFileIdx].unpackedSize)) {
+			warning("Error unpacking '%s' from bundle file '%s'", partBuffer[foundFileIdx].partName, currentPartName);
+		}
 		free(unpackBuffer);
 	} else {
 		readFromPart(foundFileIdx, dataPtr);
diff --git a/engines/cine/unpack.cpp b/engines/cine/unpack.cpp
index 364331e439..a27387a1e3 100644
--- a/engines/cine/unpack.cpp
+++ b/engines/cine/unpack.cpp
@@ -31,6 +31,10 @@
 namespace Cine {
 
 uint32 CineUnpacker::readSource() {
+	if (_src < _srcBegin || _src + 4 > _srcEnd) {
+		_error = true;
+		return 0; // The source pointer is out of bounds, returning a default value
+	}
 	uint32 value = READ_BE_UINT32(_src);
 	_src -= 4;
 	return value;
@@ -67,7 +71,10 @@ uint16 CineUnpacker::getBits(byte numBits) {
 }
 
 void CineUnpacker::unpackRawBytes(uint16 numBytes) {
-	_datasize -= numBytes;
+	if (_dst >= _dstEnd || _dst - numBytes + 1 < _dstBegin) {
+		_error = true;
+		return; // Destination pointer is out of bounds for this operation
+	}
 	while (numBytes--) {
 		*_dst = (byte)getBits(8);
 		--_dst;
@@ -75,21 +82,33 @@ void CineUnpacker::unpackRawBytes(uint16 numBytes) {
 }
 
 void CineUnpacker::copyRelocatedBytes(uint16 offset, uint16 numBytes) {
-	_datasize -= numBytes;
+	if (_dst + offset >= _dstEnd || _dst - numBytes + 1 < _dstBegin) {
+		_error = true;
+		return; // Destination pointer is out of bounds for this operation
+	}
 	while (numBytes--) {
 		*_dst = *(_dst + offset);
 		--_dst;
 	}
 }
 
-bool CineUnpacker::unpack(byte *dst, const byte *src, int srcLen) {
-	_src = src + srcLen - 4;
-	_datasize = readSource(); // Unpacked length in bytes
-	_dst = dst + _datasize - 1;
+bool CineUnpacker::unpack(const byte *src, uint srcLen, byte *dst, uint dstLen) {
+	// Initialize variables used for detecting errors during unpacking
+	_error    = false;
+	_srcBegin = src;
+	_srcEnd   = src + srcLen;
+	_dstBegin = dst;
+	_dstEnd   = dst + dstLen;
+
+	// Initialize other variables
+	_src = _srcBegin + srcLen - 4;
+	uint32 unpackedLength = readSource(); // Unpacked length in bytes
+	_dst = _dstBegin + unpackedLength - 1;
 	_crc = readSource();
 	_chunk32b = readSource();
 	_crc ^= _chunk32b;
-	do {
+
+	while (_dst >= _dstBegin && !_error) {
 		/*
 		Bits  => Action:
 		0 0   => unpackRawBytes(3 bits + 1)              i.e. unpackRawBytes(1..9)
@@ -123,8 +142,8 @@ bool CineUnpacker::unpack(byte *dst, const byte *src, int srcLen) {
 				copyRelocatedBytes(offset, numBytes);
 			}
 		}
-	} while (_datasize > 0 && _src >= src - 4);
-	return _crc == 0;
+	}
+	return !_error && (_crc == 0);
 }
 
 } // End of namespace Cine
diff --git a/engines/cine/unpack.h b/engines/cine/unpack.h
index f0a7ee3804..5bb7bfc8f9 100644
--- a/engines/cine/unpack.h
+++ b/engines/cine/unpack.h
@@ -40,7 +40,7 @@ namespace Cine {
 class CineUnpacker {
 public:
 	/** Returns true if unpacking was successful, otherwise false. */
-	bool unpack(byte *dst, const byte *src, int srcLen);
+	bool unpack(const byte *src, uint srcLen, byte *dst, uint dstLen);
 private:
 	/** Reads a single big endian 32-bit integer from the source and goes backwards 4 bytes. */
 	uint32 readSource();
@@ -69,11 +69,17 @@ private:
 	 */
 	void copyRelocatedBytes(uint16 offset, uint16 numBytes);
 private:
-	int _datasize;    //!< Bytes left to write into the unpacked data stream
-	uint32 _crc;      //!< Error-detecting code
+	uint32 _crc;      //!< Error-detecting code (This should be zero after successful unpacking)
 	uint32 _chunk32b; //!< The current internal 32-bit chunk
-	byte *_dst;       //!< Destination buffer pointer
-	const byte *_src; //!< Source buffer pointer
+	byte *_dst;       //!< Pointer to the current position in the destination buffer
+	const byte *_src; //!< Pointer to the current position in the source buffer
+
+	// These are used for detecting errors (e.g. out of bounds issues) during unpacking
+	bool _error;           //!< Did an error occur during unpacking?
+	const byte *_srcBegin; //!< Source buffer's beginning
+	const byte *_srcEnd;   //!< Source buffer's end
+	byte *_dstBegin;       //!< Destination buffer's beginning
+	byte *_dstEnd;         //!< Destination buffer's end
 };
 
 } // End of namespace Cine
-- 
cgit v1.2.3