Fix potential buffer overflow in COM_Parse

e.g. `wad` field longer than 1023 characters
andrei-drexler · Sep 24, 2023 · 764ef9d · 764ef9d · andrei-drexler · Sep 24, 2023
1 parent 9497d57
commit 764ef9d
Show file tree

Hide file tree

Showing 7 changed files with 49 additions and 14 deletions.
diff --git a/Quake/common.c b/Quake/common.c
@@ -1253,12 +1253,16 @@ char *COM_TintString (const char *in, char *out, size_t outsize)
 
 /*
 ==============
-COM_Parse
+COM_ParseEx
 
 Parse a token out of a string
+
+The mode argument controls how overflow is handled:
+- CPE_NOTRUNC:		return NULL (abort parsing)
+- CPE_ALLOWTRUNC:	truncate com_token (ignore the extra characters in this token)
 ==============
 */
-const char *COM_Parse (const char *data)
+const char *COM_ParseEx (const char *data, cpe_mode mode)
 {
 	int		c;
 	int		len;
@@ -1310,26 +1314,32 @@ const char *COM_Parse (const char *data)
 				com_token[len] = 0;
 				return data;
 			}
-			com_token[len] = c;
-			len++;
+			if (len < countof (com_token) - 1)
+				com_token[len++] = c;
+			else if (mode == CPE_NOTRUNC)
+				return NULL;
 		}
 	}
 
 // parse single characters
 	if (c == '{' || c == '}'|| c == '('|| c == ')' || c == '\'' || c == ':')
 	{
-		com_token[len] = c;
-		len++;
+		if (len < countof (com_token) - 1)
+			com_token[len++] = c;
+		else if (mode == CPE_NOTRUNC)
+			return NULL;
 		com_token[len] = 0;
 		return data+1;
 	}
 
 // parse a regular word
 	do
 	{
-		com_token[len] = c;
+		if (len < countof (com_token) - 1)
+			com_token[len++] = c;
+		else if (mode == CPE_NOTRUNC)
+			return NULL;
 		data++;
-		len++;
 		c = *data;
 		/* commented out the check for ':' so that ip:port works */
 		if (c == '{' || c == '}'|| c == '('|| c == ')' || c == '\''/* || c == ':' */)
@@ -1341,6 +1351,21 @@ const char *COM_Parse (const char *data)
 }
 
 
+/*
+==============
+COM_Parse
+
+Parse a token out of a string
+
+Return NULL in case of overflow
+==============
+*/
+const char *COM_Parse (const char *data)
+{
+	return COM_ParseEx (data, CPE_NOTRUNC);
+}
+
+
 /*
 ================
 COM_CheckParm
@@ -2082,7 +2107,7 @@ const char *COM_ParseFloatNewline(const char *buffer, float *value)
 const char *COM_ParseStringNewline(const char *buffer)
 {
 	int i;
-	for (i = 0; i < 1023; i++)
+	for (i = 0; i < countof (com_token) - 1; i++)
 		if (!buffer[i] || q_isspace (buffer[i]))
 			break;
 	memcpy (com_token, buffer, i);

diff --git a/Quake/common.h b/Quake/common.h
@@ -270,7 +270,14 @@ extern int q_vsnprintf(char *str, size_t size, const char *format, va_list args)
 extern	THREAD_LOCAL char		com_token[1024];
 extern	qboolean	com_eof;
 
+typedef enum
+{
+	CPE_NOTRUNC,					// return parse error in case of overflow
+	CPE_ALLOWTRUNC,					// truncate com_token in case of overflow
+} cpe_mode;
+
 const char *COM_Parse (const char *data);
+const char *COM_ParseEx (const char *data, cpe_mode mode);
 
 
 extern	int		com_argc;

diff --git a/Quake/gl_fog.c b/Quake/gl_fog.c
@@ -224,7 +224,7 @@ void Fog_ParseWorldspawn (void)
 			q_strlcpy(key, com_token, sizeof(key));
 		while (key[0] && key[strlen(key)-1] == ' ') // remove trailing spaces
 			key[strlen(key)-1] = 0;
-		data = COM_Parse(data);
+		data = COM_ParseEx(data, CPE_ALLOWTRUNC);
 		if (!data)
 			return; // error
 		q_strlcpy(value, com_token, sizeof(value));

diff --git a/Quake/gl_model.c b/Quake/gl_model.c
@@ -2539,7 +2539,7 @@ qboolean Mod_LoadMapDescription (char *desc, size_t maxchars, const char *map)
 			is_classname = i != 0 && !strcmp (com_token, "classname");
 
 			// parse value
-			data = COM_Parse (data);
+			data = COM_ParseEx (data, CPE_ALLOWTRUNC);
 			if (!data)
 				return ret;
 

diff --git a/Quake/gl_rmisc.c b/Quake/gl_rmisc.c
@@ -407,7 +407,7 @@ static void R_ParseWorldspawn (void)
 			q_strlcpy(key, com_token, sizeof(key));
 		while (key[0] && key[strlen(key)-1] == ' ') // remove trailing spaces
 			key[strlen(key)-1] = 0;
-		data = COM_Parse(data);
+		data = COM_ParseEx(data, CPE_ALLOWTRUNC);
 		if (!data)
 			return; // error
 		q_strlcpy(value, com_token, sizeof(value));

diff --git a/Quake/gl_sky.c b/Quake/gl_sky.c
@@ -559,7 +559,7 @@ void Sky_NewMap (void)
 			q_strlcpy(key, com_token, sizeof(key));
 		while (key[0] && key[strlen(key)-1] == ' ') // remove trailing spaces
 			key[strlen(key)-1] = 0;
-		data = COM_Parse(data);
+		data = COM_ParseEx(data, CPE_ALLOWTRUNC);
 		if (!data)
 			return; // error
 		q_strlcpy(value, com_token, sizeof(value));

diff --git a/Quake/pr_edict.c b/Quake/pr_edict.c
@@ -1144,7 +1144,10 @@ const char *ED_ParseEdict (const char *data, edict_t *ent)
 		}
 
 		// parse value
-		data = COM_Parse (data);
+		// HACK: we allow truncation when reading the wad field,
+		// otherwise maps using lots of wads with absolute paths
+		// could cause a parse error
+		data = COM_ParseEx (data, !strcmp (keyname, "wad") ? CPE_ALLOWTRUNC : CPE_NOTRUNC);
 		if (!data)
 			Host_Error ("ED_ParseEntity: EOF without closing brace");