Re: Format string in Doomsday 1.8.6
On Mon, Apr 03, 2006 at 11:20:34PM +0200, Luigi Auriemma wrote:
> Application: Doomsday engine
> The Doomsday engine contains many functions used for the visualization
> of the messages in the console.
> Both Con_Message and conPrintf are vulnerable to a format string
> vulnerability which could allow an attacker to execute malicious code
> versus the server or the clients.
> The first function calls a "Con_Printf(buffer)" while the second one
> calls a "SW_Printf(prbuff)" if SW_IsActive is enabled (which means
> ever).
>
> >From Src/con_main.c:
>
> void Con_Message(const char *message, ...)
> {
> va_list argptr;
> char *buffer;
>
> if(message[0])
> {
> buffer = malloc(0x10000);
>
> va_start(argptr, message);
> vsprintf(buffer, message, argptr);
Duh!
> va_end(argptr);
>
> #ifdef UNIX
> if(!isDedicated)
> {
> // These messages are supposed to be visible in the
> real console.
> fprintf(stderr, "%s", buffer);
> }
> #endif
>
> // These messages are always dumped. If consoleDump is set,
> // Con_Printf() will dump the message for us.
> if(!consoleDump)
> printf("%s", buffer);
>
> // Also print in the console.
> Con_Printf(buffer);
>
> free(buffer);
> }
> Con_DrawStartupScreen(true);
> }
>
> ...
>
> void conPrintf(int flags, const char *format, va_list args)
> {
> unsigned int i;
> int lbc; // line buffer cursor
> char *prbuff, *lbuf = malloc(maxLineLen + 1);
> cbline_t *line;
>
> if(flags & CBLF_RULER)
> {
> Con_AddRuler();
> flags &= ~CBLF_RULER;
> }
>
> // Allocate a print buffer that will surely be enough (64Kb).
> // FIXME: No need to allocate on EVERY printf call!
> prbuff = malloc(65536);
>
> // Format the message to prbuff.
> vsprintf(prbuff, format, args);
>
> if(consoleDump)
> fprintf(outFile, "%s", prbuff);
> if(SW_IsActive())
> SW_Printf(prbuff);
> 4) Fix
> ======
>
>
> No fix.
C'mon! Just how hard it to do something like this:
diff -uprN Src.orig/con_main.c Src/con_main.c
--- Src.orig/con_main.c 2005-01-08 19:11:54.000000000 +0300
+++ Src/con_main.c 2006-04-07 18:22:55.000000000 +0400
@@ -988,7 +988,7 @@ static void printcvar(cvar_t *var, char
if(var->flags & CVF_PROTECTED)
equals = ':';
- Con_Printf(prefix);
+ Con_Printf("%s", prefix);
switch (var->type)
{
case CVT_NULL:
@@ -2782,7 +2782,7 @@ void Con_Message(const char *message, ..
printf("%s", buffer);
// Also print in the console.
- Con_Printf(buffer);
+ Con_Printf("%s", buffer);
free(buffer);
}
diff -uprN Src.orig/sys_master.c Src/sys_master.c
--- Src.orig/sys_master.c 2004-06-13 16:46:30.000000000 +0400
+++ Src/sys_master.c 2006-04-07 18:23:56.000000000 +0400
@@ -171,7 +171,7 @@ int N_MasterSendAnnouncement(void *parm)
memset(buf, 0, sizeof(buf));
while(recv(s, buf, sizeof(buf) - 1, 0) > 0)
{
- Con_Printf(buf);
+ Con_Printf("%s", buf);
memset(buf, 0, sizeof(buf));
}
*/
Or even better:
diff -uprN Src.orig/Common/m_multi.c Src/Common/m_multi.c
--- Src.orig/Common/m_multi.c 2004-10-23 16:42:46.000000000 +0400
+++ Src/Common/m_multi.c 2006-04-07 18:09:21.000000000 +0400
@@ -312,7 +312,7 @@ int Executef(int silent, char *format, .
char buffer[512];
va_start(argptr, format);
- vsprintf(buffer, format, argptr);
+ vsnprintf(buffer, sizeof(buffer), format, argptr);
va_end(argptr);
return Con_Execute(buffer, silent);
}
diff -uprN Src.orig/Common/p_xgline.c Src/Common/p_xgline.c
--- Src.orig/Common/p_xgline.c 2004-12-23 17:48:56.000000000 +0300
+++ Src/Common/p_xgline.c 2006-04-07 18:09:15.000000000 +0400
@@ -81,7 +81,7 @@ void XG_Dev(const char *format, ...)
if(!xgDev)
return;
va_start(args, format);
- vsprintf(buffer, format, args);
+ vsnprintf(buffer, sizeof(buffer), format, args);
strcat(buffer, "\n");
Con_Message(buffer);
va_end(args);
diff -uprN Src.orig/con_main.c Src/con_main.c
--- Src.orig/con_main.c 2005-01-08 19:11:54.000000000 +0300
+++ Src/con_main.c 2006-04-07 18:22:55.000000000 +0400
@@ -988,7 +988,7 @@ static void printcvar(cvar_t *var, char
if(var->flags & CVF_PROTECTED)
equals = ':';
- Con_Printf(prefix);
+ Con_Printf("%s", prefix);
switch (var->type)
{
case CVT_NULL:
@@ -1304,7 +1304,7 @@ int Con_Executef(int silent, const char
char buffer[4096];
va_start(argptr, command);
- vsprintf(buffer, command, argptr);
+ vsnprintf(buffer, sizeof(buffer), command, argptr);
va_end(argptr);
return Con_Execute(buffer, silent);
}
@@ -1966,7 +1966,7 @@ void conPrintf(int flags, const char *fo
prbuff = malloc(65536);
// Format the message to prbuff.
- vsprintf(prbuff, format, args);
+ vsnprintf(prbuff, sizeof(prbuff), format, args);
if(consoleDump)
fprintf(outFile, "%s", prbuff);
@@ -2765,7 +2765,7 @@ void Con_Message(const char *message, ..
buffer = malloc(0x10000);
va_start(argptr, message);
- vsprintf(buffer, message, argptr);
+ vsnprintf(buffer, sizeof(buffer), message, argptr);
va_end(argptr);
#ifdef UNIX
@@ -2782,7 +2782,7 @@ void Con_Message(const char *message, ..
printf("%s", buffer);
// Also print in the console.
- Con_Printf(buffer);
+ Con_Printf("%s", buffer);
free(buffer);
}
@@ -2806,7 +2806,7 @@ void Con_Error(const char *error, ...)
fprintf(outFile, "Con_Error: Stack overflow imminent,
aborting...\n");
va_start(argptr, error);
- vsprintf(buff, error, argptr);
+ vsnprintf(buff, sizeof(buff), error, argptr);
va_end(argptr);
Sys_MessageBox(buff, true);
@@ -2821,7 +2821,7 @@ void Con_Error(const char *error, ...)
Dir_ChDir(&ddRuntimeDir);
va_start(argptr, error);
- vsprintf(err, error, argptr);
+ vsnprintf(err, sizeof(err), error, argptr);
va_end(argptr);
fprintf(outFile, "%s\n", err);
diff -uprN Src.orig/dd_pinit.c Src/dd_pinit.c
--- Src.orig/dd_pinit.c 2004-06-01 20:11:38.000000000 +0400
+++ Src/dd_pinit.c 2006-04-07 18:17:27.000000000 +0400
@@ -82,7 +82,7 @@ void DD_ErrorBox(boolean error, char *fo
va_list args;
va_start(args, format);
- vsprintf(buff, format, args);
+ vsnprintf(buff, sizeof(buff), format, args);
va_end(args);
#ifdef WIN32
MessageBox(NULL, buff, "Doomsday " DOOMSDAY_VERSION_TEXT,
diff -uprN Src.orig/dpMapLoad/glBSP/system.c Src/dpMapLoad/glBSP/system.c
--- Src.orig/dpMapLoad/glBSP/system.c 2004-07-25 00:05:32.000000000 +0400
+++ Src/dpMapLoad/glBSP/system.c 2006-04-07 18:16:37.000000000 +0400
@@ -54,7 +54,7 @@ void FatalError(const char *str, ...)
va_list args;
va_start(args, str);
- vsprintf(message_buf, str, args);
+ vsnprintf(message_buf, sizeof(message_buf), str, args);
va_end(args);
(* cur_funcs->fatal_error)("\nError: *** %s ***\n\n", message_buf);
@@ -68,7 +68,7 @@ void InternalError(const char *str, ...)
va_list args;
va_start(args, str);
- vsprintf(message_buf, str, args);
+ vsnprintf(message_buf, sizeof(message_buf), str, args);
va_end(args);
(* cur_funcs->fatal_error)("\nINTERNAL ERROR: *** %s ***\n\n", message_buf);
@@ -82,7 +82,7 @@ void PrintMsg(const char *str, ...)
va_list args;
va_start(args, str);
- vsprintf(message_buf, str, args);
+ vsnprintf(message_buf, sizeof(message_buf), str, args);
va_end(args);
(* cur_funcs->print_msg)("%s", message_buf);
@@ -100,7 +100,7 @@ void PrintWarn(const char *str, ...)
va_list args;
va_start(args, str);
- vsprintf(message_buf, str, args);
+ vsnprintf(message_buf, sizeof(message_buf), str, args);
va_end(args);
(* cur_funcs->print_msg)("Warning: %s", message_buf);
@@ -122,7 +122,7 @@ void PrintMiniWarn(const char *str, ...)
if (cur_info->mini_warnings)
{
va_start(args, str);
- vsprintf(message_buf, str, args);
+ vsnprintf(message_buf, sizeof(message_buf), str, args);
va_end(args);
(* cur_funcs->print_msg)("Warning: %s", message_buf);
@@ -132,7 +132,7 @@ void PrintMiniWarn(const char *str, ...)
#if DEBUG_ENABLED
va_start(args, str);
- vsprintf(message_buf, str, args);
+ vsnprintf(message_buf, sizeof(message_buf), str, args);
va_end(args);
PrintDebug("MiniWarn: %s", message_buf);
diff -uprN Src.orig/dpMapLoad/maploader.c Src/dpMapLoad/maploader.c
--- Src.orig/dpMapLoad/maploader.c 2004-10-23 16:42:48.000000000 +0400
+++ Src/dpMapLoad/maploader.c 2006-04-07 18:17:05.000000000 +0400
@@ -202,7 +202,7 @@ static void fatal_error(const char *str,
va_list args;
va_start(args, str);
- vsprintf(buffer, str, args);
+ vsnprintf(buffer, sizeof(buffer), str, args);
va_end(args);
Con_Error(buffer);
@@ -218,7 +218,7 @@ static void print_msg(const char *str, .
va_list args;
va_start(args, str);
- vsprintf(buffer, str, args);
+ vsnprintf(buffer, sizeof(buffer), str, args);
va_end(args);
Con_Message(buffer);
diff -uprN Src.orig/drD3D/main.cpp Src/drD3D/main.cpp
--- Src.orig/drD3D/main.cpp 2004-12-23 18:52:48.000000000 +0300
+++ Src/drD3D/main.cpp 2006-04-07 18:09:43.000000000 +0400
@@ -75,7 +75,7 @@ void DP(const char *format, ...)
va_list args;
char buf[2048];
va_start(args, format);
- vsprintf(buf, format, args);
+ vsnprintf(buf, sizeof(buf), format, args);
va_end(args);
Con_Message("%s\n", buf);
}
diff -uprN Src.orig/m_string.c Src/m_string.c
--- Src.orig/m_string.c 2004-06-01 20:11:40.000000000 +0400
+++ Src/m_string.c 2006-04-07 18:17:58.000000000 +0400
@@ -185,7 +185,7 @@ void Str_Appendf(ddstring_t * ds, const
// Print the message into the buffer.
va_start(args, format);
- vsprintf(buf, format, args);
+ vsnprintf(buf, sizeof(buf), format, args);
va_end(args);
Str_Append(ds, buf);
}
diff -uprN Src.orig/sys_master.c Src/sys_master.c
--- Src.orig/sys_master.c 2004-06-13 16:46:30.000000000 +0400
+++ Src/sys_master.c 2006-04-07 18:23:56.000000000 +0400
@@ -171,7 +171,7 @@ int N_MasterSendAnnouncement(void *parm)
memset(buf, 0, sizeof(buf));
while(recv(s, buf, sizeof(buf) - 1, 0) > 0)
{
- Con_Printf(buf);
+ Con_Printf("%s", buf);
memset(buf, 0, sizeof(buf));
}
*/
diff -uprN Src.orig/sys_musd_win.c Src/sys_musd_win.c
--- Src.orig/sys_musd_win.c 2004-06-01 20:11:50.000000000 +0400
+++ Src/sys_musd_win.c 2006-04-07 18:17:44.000000000 +0400
@@ -779,7 +779,7 @@ int DM_WinCDCommand(char *returnInfo, in
MCIERROR error;
va_start(args, format);
- vsprintf(buf, format, args);
+ vsnprintf(buf, sizeof(buf), format, args);
va_end(args);
if((error = mciSendString(buf, returnInfo, returnLength, NULL)))
diff -uprN Src.orig/sys_sock.c Src/sys_sock.c
--- Src.orig/sys_sock.c 2004-06-01 20:11:50.000000000 +0400
+++ Src/sys_sock.c 2006-04-07 18:15:13.000000000 +0400
@@ -95,7 +95,7 @@ void N_SockPrintf(socket_t s, const char
// Print the message into the buffer.
va_start(args, format);
- length = vsprintf(buf, format, args);
+ length = vsnprintf(buf, sizeof(buf), format, args);
va_end(args);
if(length > sizeof(buf))
diff -uprN Src.orig/sys_stwin.c Src/sys_stwin.c
--- Src.orig/sys_stwin.c 2005-01-03 16:08:50.000000000 +0300
+++ Src/sys_stwin.c 2006-04-07 18:13:25.000000000 +0400
@@ -137,7 +137,7 @@ void SW_Printf(const char *format, ...)
return;
va_start(args, format);
- printedChars += vsprintf(buf, format, args);
+ printedChars += vsnprintf(buf, sizeof(buf), format, args);
va_end(args);
if(printedChars > 32768)
Even more better is to apply rm(1).