From 3289134ae36f4389fe8f5c229f79617fab27cd0f Mon Sep 17 00:00:00 2001
From: thilo <thilo@edf5b092-35ff-0310-97b2-ce42778d08ea>
Date: Fri, 16 Jun 2006 20:38:08 +0000
Subject: - Fix bug that allows a malicious server to write and overwrite any
 files in the quake3 directory.   Reported by Luigi Auriemma. - Moved
 directory traversal check to a more proper location. - Added a few sanity
 checks for checksum/pakname storage to fix a crash that can occur under
 certain circumstances.

git-svn-id: svn://svn.icculus.org/quake3/trunk@804 edf5b092-35ff-0310-97b2-ce42778d08ea
---
 code/client/cl_main.c |  7 -------
 code/qcommon/files.c  | 51 ++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 40 insertions(+), 18 deletions(-)

(limited to 'code')

diff --git a/code/client/cl_main.c b/code/client/cl_main.c
index 12b102d..a2b93b9 100644
--- a/code/client/cl_main.c
+++ b/code/client/cl_main.c
@@ -1444,13 +1444,6 @@ void CL_NextDownload(void) {
 		else
 			s = localName + strlen(localName); // point at the nul byte
 		
-		// Make sure the server cannot make us write to non-quake3 directories.
-		if(strstr(localName, "../") || strstr(localName, "..\\"))
-		{
-			Com_Error(ERR_DROP, "CL_NextDownload: Invalid download name %s", localName);
-			return;
-		}
-
 		CL_BeginDownload( localName, remoteName );
 
 		clc.downloadRestart = qtrue;
diff --git a/code/qcommon/files.c b/code/qcommon/files.c
index 31b9b66..7692079 100644
--- a/code/qcommon/files.c
+++ b/code/qcommon/files.c
@@ -2597,15 +2597,16 @@ we are not interested in a download string format, we want something human-reada
 qboolean FS_ComparePaks( char *neededpaks, int len, qboolean dlstring ) {
 	searchpath_t	*sp;
 	qboolean havepak, badchecksum;
+	char *origpos = neededpaks;
 	int i;
 
-	if ( !fs_numServerReferencedPaks ) {
+	if (!fs_numServerReferencedPaks)
 		return qfalse; // Server didn't send any pack information along
-	}
 
 	*neededpaks = 0;
 
-	for ( i = 0 ; i < fs_numServerReferencedPaks ; i++ ) {
+	for ( i = 0 ; i < fs_numServerReferencedPaks ; i++ )
+	{
 		// Ok, see if we have this pak file
 		badchecksum = qfalse;
 		havepak = qfalse;
@@ -2615,6 +2616,13 @@ qboolean FS_ComparePaks( char *neededpaks, int len, qboolean dlstring ) {
 			continue;
 		}
 
+		// Make sure the server cannot make us write to non-quake3 directories.
+		if(strstr(fs_serverReferencedPakNames[i], "../") || strstr(fs_serverReferencedPakNames[i], "..\\"))
+                {
+			Com_Printf("WARNING: Invalid download name %s\n", fs_serverReferencedPakNames[i]);
+                        continue;
+                }
+
 		for ( sp = fs_searchpaths ; sp ; sp = sp->next ) {
 			if ( sp->pack && sp->pack->checksum == fs_serverReferencedPaks[i] ) {
 				havepak = qtrue; // This is it!
@@ -2627,6 +2635,12 @@ qboolean FS_ComparePaks( char *neededpaks, int len, qboolean dlstring ) {
 
       if (dlstring)
       {
+	// We need this to make sure we won't hit the end of the buffer or the server could
+	// overwrite non-pk3 files on clients by writing so much crap into neededpaks that
+	// Q_strcat cuts off the .pk3 extension.
+	
+	origpos += strlen(origpos);
+	
         // Remote name
         Q_strcat( neededpaks, len, "@");
         Q_strcat( neededpaks, len, fs_serverReferencedPakNames[i] );
@@ -2647,6 +2661,14 @@ qboolean FS_ComparePaks( char *neededpaks, int len, qboolean dlstring ) {
           Q_strcat( neededpaks, len, fs_serverReferencedPakNames[i] );
           Q_strcat( neededpaks, len, ".pk3" );
         }
+        
+        // Find out whether it might have overflowed the buffer and don't add this file to the
+        // list if that is the case.
+        if(strlen(origpos) + (origpos - neededpaks) >= len - 1)
+	{
+		*origpos = '\0';
+		break;
+	}
       }
       else
       {
@@ -3209,7 +3231,7 @@ checksums to see if any pk3 files need to be auto-downloaded.
 =====================
 */
 void FS_PureServerSetReferencedPaks( const char *pakSums, const char *pakNames ) {
-	int		i, c, d;
+	int		i, c, d = 0;
 
 	Cmd_TokenizeString( pakSums );
 
@@ -3218,30 +3240,37 @@ void FS_PureServerSetReferencedPaks( const char *pakSums, const char *pakNames )
 		c = MAX_SEARCH_PATHS;
 	}
 
-	fs_numServerReferencedPaks = c;
-
 	for ( i = 0 ; i < c ; i++ ) {
 		fs_serverReferencedPaks[i] = atoi( Cmd_Argv( i ) );
 	}
 
-	for ( i = 0 ; i < c ; i++ ) {
-		if (fs_serverReferencedPakNames[i]) {
+	for (i = 0 ; i < sizeof(fs_serverReferencedPakNames) / sizeof(*fs_serverReferencedPakNames); i++)
+	{
+		if(fs_serverReferencedPakNames[i])
 			Z_Free(fs_serverReferencedPakNames[i]);
-		}
+
 		fs_serverReferencedPakNames[i] = NULL;
 	}
+
 	if ( pakNames && *pakNames ) {
 		Cmd_TokenizeString( pakNames );
 
 		d = Cmd_Argc();
-		if ( d > MAX_SEARCH_PATHS ) {
+		if(d > MAX_SEARCH_PATHS)
 			d = MAX_SEARCH_PATHS;
-		}
+		else if(d > c)
+			d = c;
 
 		for ( i = 0 ; i < d ; i++ ) {
 			fs_serverReferencedPakNames[i] = CopyString( Cmd_Argv( i ) );
 		}
 	}
+	
+	// ensure that there are as many checksums as there are pak names.
+	if(d < c)
+		c = d;
+	
+	fs_numServerReferencedPaks = c;	
 }
 
 /*
-- 
cgit v1.2.3