Game Development Community

Serious buffer overflow bug in dSprintf in TGE 1.3 and 1.4

by Orion Elenzil · in Torque Game Engine · 07/13/2006 (12:44 pm) · 20 replies

From winStrings.cc

S32 dSprintf(char *buffer, U32 bufferSize, const char *format, ...)
{
   va_list args;
   va_start(args, format);

#if defined(TORQUE_COMPILER_CODEWARRIOR)
   S32 len = vsnprintf(buffer, bufferSize, format, args);
#else
   bufferSize;
   S32 len = vsprintf(buffer, format, args);
#endif
   return (len);
}

Q: what's wrong with this code ?

A: we're ignoring the whole bufferSize parameter !

these two lines:

bufferSize;
   S32 len = vsprintf(buffer, format, args);

should read:

S32 len = vsnprintf(buffer, bufferSize, format, args);

which argues that the entire function should be refactored thus:

S32 dSprintf(char *buffer, U32 bufferSize, const char *format, ...)
{
   va_list args;
   va_start(args, format);

   S32 len = vsnprintf(buffer, bufferSize, format, args);
   return (len);
}

possibly vsnprintf didn't compile on windows in the past, but it compiles fine for me with VS8,
so i urge that everyone make this change asap. Note that the implementations look fine in Unix.

DITTO dVsprintf(). it's also dropping the buffer size. dVsprintf() should be:

S32 dVsprintf(char *buffer, U32 bufferSize, const char *format, void *arglist)
{
   S32 len = vsnprintf(buffer, bufferSize, format, (char*)arglist);
   return (len);
}

more on this, if the problem isn't painfully obvious already.
by the time execution hits breakpointline2 in the following code snippet, the string "bar" has become "zzzzZZZZ" !
Think about the memory being trashed if foo & bar weren't there.

ConsoleFunction(testdSprintf, void, 1, 1, "" )
{
   char bar[41]    = "4444444444333333333322222222221111111111";
   char buf[16]    = "===============";
   char foo[41]    = "1111111111222222222233333333334444444444";

   int breakpointline1 = 9;

   dSprintf(buf, 15, "wwwwWWWWxxxxXXXXyyyyYYYYzzzzZZZZ");

   int breakpointline2 = 9;
}

.. and now, i rant. no real point in reading further.

obviously at some point in the past there was a problem compiling vsnprintf on windows.
.. bummer. and also a little hard to believe, but i'll try to believe it.

* it seems like in that case the best thing to do would be to frigging EMULATE vsnprintf !
it's not rocket science!

* the second best thing would be to responsibly bubble the lack of support for buffer-overrun-protected sprintf up to the programmer, so at least the programmer would have a clue that this method he or she was counting on to protect their buffers was not in fact protecting them at all.

* the worst thing to do is what was done in the codebase at hand, which is to give the appearance that a critical function is being performed when it isn't.

this is the kind of stuff that makes servers crash when released into the real world even tho they worked fine in the lab. for example maybe a user signs on with a very long user name. there's thousand other examples.

okay.

Associate Orion Elenzil
Real Life Plus

About the author

Recent Threads

ot: new app submitted

request - include the 'summary' in the main page of a resource.

1.0 crash-on-exit due to GFX being NULL during GFXPCD3D9TextureTarget::zombify().

stepping into tVector.h hangs visual studio for minutes - LOGGED

iPhone dead reckoning - i reckon not

NextNext Thread

PrevPrevious Thread

More from Torque Game Engine

Associate Orion Elenzil
Real Life Plus

07/13/2006 (1:46 pm)

One more note on this.

in the case where you actually *do* overrun the buffer,
the standard stdio implementation of vsnprintf() does not terminate the string !
which means that unless you're checking the return value (unlikely), you could end up using a string which wanders off into random memory.

i propose the following implementations.

S32 dSprintf(char *buffer, U32 bufferSize, const char *format, ...)
{
   va_list args;
   va_start(args, format);

   S32 len = vsnprintf(buffer, bufferSize, format, args);
   if (bufferSize > 0)
      buffer[bufferSize-1] = '[[6280e7a146b01]]';
   AssertWarn(len >= 0 && len < bufferSize, "String truncated"); // note recursive, but should terminate with next entry.
   return (len);
}   


S32 dVsprintf(char *buffer, U32 bufferSize, const char *format, void *arglist)
{
   S32 len = vsnprintf(buffer, bufferSize, format, (char*)arglist);
   if (bufferSize > 0)
      buffer[bufferSize-1] = '[[6280e7a146b01]]';
   AssertWarn(len >= 0 && len < bufferSize, "String truncated"); // note recursive, but should terminate with next entry.
   return (len);
}

Associate Orion Elenzil
Real Life Plus

07/13/2006 (1:49 pm)

Sorry, yet more on this.
expanding on fix proposed just above (ie, forcing string termination),
if you don't do that then the following code will end up with "buf" being something like "wwwwWWWWxxxxXXXXand now, totally random memory! who knows when it'll stop!"

ConsoleFunction(testdSprintf, void, 1, 1, "" )
{
   char buf[16];

   dSprintf(buf, 16, "wwwwWWWWxxxxXXXXyyyyYYYYzzzzZZZZ");

   int breakpointline = 9;
}

with the fix immediately above,
buf will be the more reasonable "wwwwWWWWxxxxXXX".

Torque Owner Cliff

07/13/2006 (2:17 pm)

I haven't had the opportunity to check, is this also in the TSE? If so, I'll have to patch up our version tonight.

Torque Owner Thomas \"Man of Ice\" Lund

07/13/2006 (2:47 pm)

Also in TSE

Torque Owner Cliff

07/13/2006 (3:23 pm)

Figured it would be... roight, guess that's the first bit of code to write tonight.

Associate Orion Elenzil
Real Life Plus

07/13/2006 (6:21 pm)

In thinking and talking about this a bit more, we've decided to actually implement those proposed versions of dSprintf() and dVsprintf() in both the winStrings.cc and x86UNIXStrings.cc,
but actually with the following slight modification, which replaces the AssertWarn() with a Con::errorf().

the motivation is so that you'll get some warning in the log even with release builds.

winStrings.cc and x86UNIXStrings.cc

#include "console/console.h"

...

S32 dSprintf(char *buffer, U32 bufferSize, const char *format, ...)
{
   va_list args;
   va_start(args, format);

   S32 len = vsnprintf(buffer, bufferSize, format, args);
   if (bufferSize > 0)
      buffer[bufferSize-1] = '[[6280e7a194c08]]';
   if (len < 0 || len >= bufferSize)
      Con::errorf("in dSprintf, truncated a string to \"%s\".", buffer);  // note recursive, but should terminate with next entry.
   return (len);
}   


S32 dVsprintf(char *buffer, U32 bufferSize, const char *format, void *arglist)
{
   S32 len = vsnprintf(buffer, bufferSize, format, (char*)arglist);
   if (bufferSize > 0)
      buffer[bufferSize-1] = '[[6280e7a194c08]]';
   if (len < 0 || len >= bufferSize)
      Con::errorf("in dVsprintf, truncated a string to \"%s\".", buffer);  // note recursive, but should terminate with next entry.
   return (len);
}

Associate Orion Elenzil
Real Life Plus

07/28/2006 (11:29 am)

Yet more on this,
a slight improvement to the above versions of dSprintf and dVsprintf.

those functions both do the following:

if (bufferSize > 0)
      buffer[bufferSize-1] = '[[6280e7a1993ef]]';

which is well and good, unless the caller is *lying* about the bufferSize !
.. which turns out to be the case in CodeBlock::exec() ... case OP_TAG_TO_STR:.
basically it says it has a buffer of size N when it's in fact got a buffer of size M, M < N.

in that situation the above line buffer[bufferSize-1] = 0 will write into random memory,
even if the string being written to buffer does not overflow the buffer.

two fixes:

one, a fix to the core offending routine, which Clint Brewer amazingly tracked down and nailed. I'll let clint post that guy up if he wants.

two, another band-aid to dSprintf which only null-trerminated the string in the case where the buffer has in fact overflowed. This is only flimsy protection against incorrect bufferSizes, but it's better than the previous state. that code is here:

winStrings.cc and x86UNIXStrings.cc

S32 dSprintf(char *buffer, U32 bufferSize, const char *format, ...)
{
   va_list args;
   va_start(args, format);

   S32 len = vsnprintf(buffer, bufferSize, format, args);
   if (len < 0 || len >= bufferSize)
   {
      if (bufferSize > 0)
        buffer[bufferSize-1] = '[[6280e7a1994d3]]';
      // AWOOOOGA AWOOOOGA
      AssertFatal(0, "Buffer Overflow in dSprintf");            // note recurses one step
      Con::errorf("Buffer Overflow in dSprintf (%s)", buffer);  // note recurses one step
   }
   return (len);
}   


S32 dVsprintf(char *buffer, U32 bufferSize, const char *format, void *arglist)
{
   S32 len = vsnprintf(buffer, bufferSize, format, (char*)arglist);
   if (len < 0 || len >= bufferSize)
   {
      if (bufferSize > 0)
        buffer[bufferSize-1] = '[[6280e7a1994d3]]';
      // AWOOOOGA AWOOOOGA
      AssertFatal(0, "Buffer Overflow in dVsprintf");            // note recurses one step
      Con::errorf("Buffer Overflow in dVsprintf (%s)", buffer);  // note recurses one step
   }
   return (len);
}

Torque Owner Clint S. Brewer

07/28/2006 (12:06 pm)

Here's a fix to the offending call, I didn't track down fully the reasons for the way it was setup before though, why the hardcoded 7 etc, so if anyone else looks into it deeper, do tell!
:

in compiler.h in CodeBlock,
right after:

char *globalStrings;
   char *functionStrings;

add

U32 functionStringsMaxLen;
   U32 globalStringsMaxLen;

in compiler.cc in CodeBlock() constructor

after

globalStrings = NULL;
   functionStrings = NULL;

add

functionStringsMaxLen = 0;
   globalStringsMaxLen = 0;

in ~CodeBock() destructor,
add

functionStringsMaxLen = 0;
   globalStringsMaxLen = 0;

in
bool CodeBlock::read(StringTableEntry fileName, Stream &st)

after

globalStrings = new char[size];

add

globalStringsMaxLen = size;

after

functionStrings = new char[size];

add

functionStringsMaxLen = size;

still in Compiler.cc in compileExec

after

globalStrings = gGlobalStringTable.build();

add

globalStringsMaxLen = gGlobalStringTable.totalLen;

and after

functionStrings = gFunctionStringTable.build();

add

functionStringsMaxLen = gFunctionStringTable.totalLen;

almost done, now in CompiledEval.cc

at the top of CodeBlock::exec

after

char *curStringTable;

add

S32 curStringTableLen = 0; //clint to ensure we dont overwrite it

after

curStringTable = functionStrings;

add

curStringTableLen = functionStringsMaxLen;

after

curStringTable = globalStrings;

add

curStringTableLen = globalStringsMaxLen;

replace your op tag to string block with mine, you can see the changes
NOTE this is a TGE 1.3 build, the changes might be different for TGE 1.4, but specifically look at maxBufferLen's calculation and send that into sprintf, that's the key.

case OP_TAG_TO_STR:
            code[ip-1] = OP_LOADIMMED_STR;
            // it's possible the string has already been converted
            if(U8(curStringTable[code[ip]]) != StringTagPrefixByte)
            {
               U32 id = GameAddTaggedString(curStringTable + code[ip]);
               U32 offset = code[ip] + 1;
               U32 maxBufferLen = curStringTableLen - offset;
               AssertFatal( (S32)curStringTableLen - (S32)offset > 0, "maxlen is negative");

               dSprintf(curStringTable + offset,  maxBufferLen , "%d", id);
               *(curStringTable + code[ip]) = StringTagPrefixByte;
            }

here is our old one for comparison:

case OP_TAG_TO_STR:
            code[ip-1] = OP_LOADIMMED_STR;
            // it's possible the string has already been converted
            if(U8(curStringTable[code[ip]]) != StringTagPrefixByte)
            {
               U32 id = GameAddTaggedString(curStringTable + code[ip]);
               dSprintf(curStringTable + code[ip] + 1, 7, "%d", id);


               *(curStringTable + code[ip]) = StringTagPrefixByte;
            }

and finally towards the bottom of the func

after

delete[] const_cast<char*>(globalStrings);

add

globalStringsMaxLen = 0;

Torque Owner Cliff

09/30/2006 (3:45 pm)

Just wanted to bump this because it looks like this is one of the few community related bug fixes that didn't make it into TSE MS4.

Torque Owner David Stewart

#10

09/30/2006 (4:13 pm)

Hmm, has this been updated in TGE 1.4.2? Will have to check.

update: an assertion has been put in, nothing else.

Associate Kyle Carter

#11

10/02/2006 (8:14 pm)

Issue #2239 for TSE. Thanks for the thorough write up!

Torque 3D Owner Jeff Loveless

#12

10/14/2006 (9:19 am)

Ben, if you know, will this fix make it into the next TGE version?

Associate Orion Elenzil
Real Life Plus

#13

11/09/2006 (1:50 pm)

Just discovered another funky aspect of this -
with my above suggested fixes to dSprintf() and dVsprintf(),
you can end up in an infinite recursion if the buffer you are overflowing happens to be the static char printBuffer from _printf(). this is because the errorf() and/or the assertWarn both end up calling _printf.

in english,
if you say echo() a line larger than PRINTF_BUFFER_SIZE, you will go into an infinite recursion and crash.

not sure how to get around this.

one temptation is rework _printf so that printBuffer is allocated on the stack. it's currently allocated thus:

static char printfBuffer[PRINTF_BUFFER_SIZE];

anyone have any insight here ?

Associate Orion Elenzil
Real Life Plus

#14

11/09/2006 (1:56 pm)

Well, another obvious fix is to simply return and neither assert() nor errorf(), (which i've tried and yes, it works)
but i feel this really is a critical event which needs to be announced when it happens.

Associate Orion Elenzil
Real Life Plus

#15

01/19/2007 (10:59 am)

Two more cases of unchecked & un-null-terminated uses of vsnprintf were found in this thread:
WinConsole::printf() and StdConsole::printf().

replacements:

x86UNIXConsole.cc

void StdConsole::printf(const char *s, ...)
{
   // Get the line into a buffer.
   static const int BufSize = 4096;
   static char buffer[BufSize];
   va_list args;
   va_start(args, s);

   S32 len = vsnprintf(buffer, BufSize, s, args);
   
   // null-terminate if overflow.
   if (len < 0 || len >= BufSize)
   {
      if (BufSize > 0)
         buffer[BufSize-1] = '[[6280e7a2120b4]]';
   }
   
   // Replace tabs with carats, like the "real" console does.
   char *pos = buffer;
   while (*pos) {
      if (*pos == '\t') {
         *pos = '^';
      }
      pos++;
   }
   // Axe the color characters.
   Con::stripColorChars(buffer);
   // Print it.
   write(stdOut, buffer, strlen(buffer));
   fflush(stdout);
}

winConsole.cc

void WinConsole::printf(const char *s, ...)
{
   // Get the line into a buffer.
   static const int BufSize = 4096;
   static char buffer[4096];
   DWORD bytes;
   va_list args;
   va_start(args, s);

   S32 len = vsnprintf(buffer, BufSize, s, args);
   
   // null-terminate if overflow.
   if (len < 0 || len >= BufSize)
   {
      if (BufSize > 0)
         buffer[BufSize-1] = '[[6280e7a212804]]';
   }
   
   // Replace tabs with carats, like the "real" console does.
   char *pos = buffer;
   while (*pos) {
      if (*pos == '\t') {
         *pos = '^';
      }
      pos++;
   }
   // Axe the color characters.
   Con::stripColorChars(buffer);
   // Print it.
   OVERLAPPED overlap;
   WriteFile(stdOut, buffer, strlen(buffer), &bytes, &overlap);
   FlushFileBuffers( stdOut );
}

Torque 3D Owner Pat Wilson

#16

01/19/2007 (3:11 pm)

Fixed in TSE.

The possible recursion was 'fixed' by not actually calling the assert, but by notifying and debug breaking manually:

...
#ifdef TORQUE_DEBUG
      Platform::AlertOK( "Overflow", "Buffer Overflow in dVsprintf" );
      Platform::debugBreak();
#endif
...

Associate Orion Elenzil
Real Life Plus

#17

01/19/2007 (3:57 pm)

Cool, thanks Pat.

since we may be running dedicated servers in debug and really don't want the process to crash,
we've opted to go with this:

...
      AssertWarn(0, "Buffer Overflow in StdConsole::printf()");  // note recurses one step
...

my earlier worries about possible infinite recursion were really due to my own foolishness
in trying to print out the string which just overflowed the buffer.

Associate Orion Elenzil
Real Life Plus

#18

01/19/2007 (3:59 pm)

Just to double-check,
i count 6 places in the TGE 1.3.5 codebase which need this check added:

one in winConsole.cc
two in winStrings.cc
one in x86UNIXConsole.cc
two in x86UNIXConsole.cc

Torque 3D Owner Pat Wilson

#19

01/19/2007 (4:42 pm)

Correct, Orion. I was putting this fix into TSE, so the win*.cc fixes and the fix from Clint has been integrated, and tested using a console function like the one in your first post.

Torque 3D Owner Thomas -elfprince13- Dickerson

#20

01/28/2010 (9:39 pm)

Did this happen to make it into TGE 1.5, does anyone know?

Post a blog
Add a resource
Start a Thread