Merge pull request JACoders#881 from smcv/bounds-check

savegames: bounds-check some string lengths to prevent buffer overflow
ataceyhun · Oct 28, 2016 · 0808ae8 · 0808ae8
2 parents 3db8c60 + 9aea795
commit 0808ae8
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 15 deletions.
diff --git a/code/game/G_Timer.cpp b/code/game/G_Timer.cpp
@@ -243,12 +243,19 @@ void TIMER_Load( void )
 			const char* sg_buffer_data = static_cast<const char*>(
 				saved_game.get_buffer_data());
 
-			const int sg_buffer_size = saved_game.get_buffer_size();
+			int sg_buffer_size = saved_game.get_buffer_size();
 
-			std::uninitialized_copy_n(
-				sg_buffer_data,
-				sg_buffer_size,
-				tempBuffer);
+			if (sg_buffer_size < 0 || static_cast<size_t>(sg_buffer_size) >= sizeof(tempBuffer))
+			{
+				sg_buffer_size = 0;
+			}
+			else
+			{
+				std::uninitialized_copy_n(
+					sg_buffer_data,
+					sg_buffer_size,
+					tempBuffer);
+			}
 
 			tempBuffer[sg_buffer_size] = '\0';
 

diff --git a/code/game/Q3_Interface.cpp b/code/game/Q3_Interface.cpp
@@ -7326,6 +7326,11 @@ void CQuake3GameInterface::VariableLoadFloats( varFloat_m &fmap )
 			INT_ID('F', 'I', 'D', 'L'),
 			idSize);
 
+		if (idSize < 0 || static_cast<size_t>(idSize) >= sizeof(tempBuffer))
+		{
+			idSize = 0;
+		}
+
 		saved_game.read_chunk(
 			INT_ID('F', 'I', 'D', 'S'),
 			tempBuffer,
@@ -7371,6 +7376,11 @@ void CQuake3GameInterface::VariableLoadStrings( int type, varString_m &fmap )
 			INT_ID('S', 'I', 'D', 'L'),
 			idSize);
 
+		if (idSize < 0 || static_cast<size_t>(idSize) >= sizeof(tempBuffer))
+		{
+			idSize = 0;
+		}
+
 		saved_game.read_chunk(
 			INT_ID('S', 'I', 'D', 'S'),
 			tempBuffer,
@@ -7382,6 +7392,11 @@ void CQuake3GameInterface::VariableLoadStrings( int type, varString_m &fmap )
 			INT_ID('S', 'V', 'S', 'Z'),
 			idSize);
 
+		if (idSize < 0 || static_cast<size_t>(idSize) >= sizeof(tempBuffer2))
+		{
+			idSize = 0;
+		}
+
 		saved_game.read_chunk(
 			INT_ID('S', 'V', 'A', 'L'),
 			tempBuffer2,

diff --git a/code/game/g_roff.cpp b/code/game/g_roff.cpp
@@ -703,6 +703,9 @@ void G_LoadCachedRoffs()
 			INT_ID('S', 'L', 'E', 'N'),
 			len);
 
+		if (len < 0 || static_cast<size_t>(len) >= sizeof(buffer))
+			len = 0;
+
 		saved_game.read_chunk(
 			INT_ID('R', 'S', 'T', 'R'),
 			buffer,

diff --git a/code/icarus/IcarusImplementation.cpp b/code/icarus/IcarusImplementation.cpp
@@ -716,12 +716,19 @@ int CIcarus::Load()
 	const unsigned char* sg_buffer_data = static_cast<const unsigned char*>(
 		saved_game.get_buffer_data());
 
-	const int sg_buffer_size = saved_game.get_buffer_size();
+	int sg_buffer_size = saved_game.get_buffer_size();
 
-	std::uninitialized_copy_n(
-		sg_buffer_data,
-		sg_buffer_size,
-		m_byBuffer);
+	if (sg_buffer_size < 0 || static_cast<size_t>(sg_buffer_size) >= sizeof(m_byBuffer))
+	{
+		sg_buffer_size = 0;
+	}
+	else
+	{
+		std::uninitialized_copy_n(
+			sg_buffer_data,
+			sg_buffer_size,
+			m_byBuffer);
+	}
 
 	//Load all signals
 	if ( LoadSignals() == false )
@@ -849,12 +856,19 @@ void CIcarus::BufferRead( void *pDstBuff, unsigned long ulNumBytesToRead )
 		const unsigned char* sg_buffer_data = static_cast<const unsigned char*>(
 			saved_game.get_buffer_data());
 
-		const int sg_buffer_size = saved_game.get_buffer_size();
+		int sg_buffer_size = saved_game.get_buffer_size();
 
-		std::uninitialized_copy_n(
-			sg_buffer_data,
-			sg_buffer_size,
-			m_byBuffer);
+		if (sg_buffer_size < 0 || static_cast<size_t>(sg_buffer_size) >= sizeof(m_byBuffer))
+		{
+			sg_buffer_size = 0;
+		}
+		else
+		{
+			std::uninitialized_copy_n(
+				sg_buffer_data,
+				sg_buffer_size,
+				m_byBuffer);
+		}
 
 		m_ulBytesRead = 0;	//reset buffer
 	}

diff --git a/codeJK2/game/Q3_Registers.cpp b/codeJK2/game/Q3_Registers.cpp
@@ -408,6 +408,11 @@ void Q3_VariableLoadFloats( varFloat_m &fmap )
 			INT_ID('F', 'I', 'D', 'L'),
 			idSize);
 
+		if (idSize < 0 || static_cast<size_t>(idSize) >= sizeof(tempBuffer))
+		{
+			idSize = 0;
+		}
+
 		saved_game.read_chunk(
 			INT_ID('F', 'I', 'D', 'S'),
 			tempBuffer,
@@ -453,6 +458,11 @@ void Q3_VariableLoadStrings( int type, varString_m &fmap )
 			INT_ID('S', 'I', 'D', 'L'),
 			idSize);
 
+		if (idSize < 0 || static_cast<size_t>(idSize) >= sizeof(tempBuffer))
+		{
+			idSize = 0;
+		}
+
 		saved_game.read_chunk(
 			INT_ID('S', 'I', 'D', 'S'),
 			tempBuffer,
@@ -464,6 +474,11 @@ void Q3_VariableLoadStrings( int type, varString_m &fmap )
 			INT_ID('S', 'V', 'S', 'Z'),
 			idSize);
 
+		if (idSize < 0 || static_cast<size_t>(idSize) >= sizeof(tempBuffer2))
+		{
+			idSize = 0;
+		}
+
 		saved_game.read_chunk(
 			INT_ID('S', 'V', 'A', 'L'),
 			tempBuffer2,

diff --git a/codeJK2/game/g_roff.cpp b/codeJK2/game/g_roff.cpp
@@ -678,6 +678,11 @@ void G_LoadCachedRoffs()
 			INT_ID('S', 'L', 'E', 'N'),
 			len);
 
+		if (len < 0 || static_cast<size_t>(len) >= sizeof(buffer))
+		{
+			len = 0;
+		}
+
 		saved_game.read_chunk(
 			INT_ID('R', 'S', 'T', 'R'),
 			buffer,