Home | Docs | Forums | Lists | Bugs | Planet | Store | GMN | Get Gentoo!

Full Text Bug Listing

ttd.c:
        char * CDECL str_fmt(const char *str, ...)
        {
        ===>    char buf[4096];    <===
                va_list va;
                int len;
                char *p;

                va_start(va, str);
        ===>    len = vsprintf(buf, str, va);   <===
                va_end(va);
                p = malloc(len + 1);
                if (p)
                        memcpy(p, buf, len + 1);
                return p;
        }
-------------------------------------------------------------------------------
str_fmt() is used everywhere, including the following snippet in unix.c:

===>    const char *homedir = getenv("HOME");   <===

        if (homedir == NULL) {
                const struct passwd *pw = getpwuid(getuid());
                if (pw != NULL) homedir = pw->pw_dir;
        }

        _path.personal_dir = str_fmt("%s" PATHSEP "%s", homedir, PERSONAL_DIR);
                             ^^^^^^^ ^^^                ^^^^^^^
-------------------------------------------------------------------------------
Notice the first chunk mentions "Network".

diff -uprN openttd-0.4.0.1/network.c openttd-0.4.0.1-vsprintf/network.c
--- openttd-0.4.0.1/network.c	2005-05-17 20:01:19.000000000 +0400
+++ openttd-0.4.0.1-vsprintf/network.c	2005-08-15 20:34:45.000000000 +0400
@@ -96,7 +96,7 @@ void CDECL NetworkTextMessage(NetworkAct
 	StringID TempStr = STR_NULL;
 
 	va_start(va, str);
-	vsprintf(buf, str, va);
+	vsnprintf(buf, sizeof(buf), str, va);
 	va_end(va);
 
 	switch (action) {
diff -uprN openttd-0.4.0.1/os2.c openttd-0.4.0.1-vsprintf/os2.c
--- openttd-0.4.0.1/os2.c	2005-05-15 18:01:35.000000000 +0400
+++ openttd-0.4.0.1-vsprintf/os2.c	2005-08-15 20:31:26.000000000 +0400
@@ -642,7 +642,7 @@ static long CDECL MidiSendCommand(const 
 	va_list va;
 	char buf[512];
 	va_start(va, cmd);
-	vsprintf(buf, cmd, va);
+	vsnprintf(buf, sizeof(buf), cmd, va);
 	va_end(va);
 	return mciSendString(buf, NULL, 0, NULL, 0);
 }
diff -uprN openttd-0.4.0.1/strgen/strgen.c openttd-0.4.0.1-vsprintf/strgen/strgen.c
--- openttd-0.4.0.1/strgen/strgen.c	2005-04-24 19:41:01.000000000 +0400
+++ openttd-0.4.0.1-vsprintf/strgen/strgen.c	2005-08-15 20:37:12.000000000 +0400
@@ -84,7 +84,7 @@ void warning(const char *s, ...) {
 	char buf[1024];
 	va_list va;
 	va_start(va, s);
-	vsprintf(buf, s, va);
+	vsnprintf(buf, sizeof(buf), s, va);
 	va_end(va);
 	fprintf(stderr, "%d: ERROR: %s\n", _cur_line, buf);
 	_warnings = true;
@@ -94,7 +94,7 @@ void NORETURN error(const char *s, ...) 
 	char buf[1024];
 	va_list va;
 	va_start(va, s);
-	vsprintf(buf, s, va);
+	vsnprintf(buf, sizeof(buf), s, va);
 	va_end(va);
 	fprintf(stderr, "%d: FATAL: %s\n", _cur_line, buf);
 	exit(1);
diff -uprN openttd-0.4.0.1/texteff.c openttd-0.4.0.1-vsprintf/texteff.c
--- openttd-0.4.0.1/texteff.c	2005-03-28 16:38:02.000000000 +0400
+++ openttd-0.4.0.1-vsprintf/texteff.c	2005-08-15 20:36:22.000000000 +0400
@@ -57,7 +57,7 @@ void CDECL AddTextMessage(uint16 color, 
 	int length;
 
 	va_start(va, message);
-	vsprintf(buf, message, va);
+	vsnprintf(buf, sizeof(buf), message, va);
 	va_end(va);
 
 	/* Special color magic */
diff -uprN openttd-0.4.0.1/ttd.c openttd-0.4.0.1-vsprintf/ttd.c
--- openttd-0.4.0.1/ttd.c	2005-05-16 20:19:32.000000000 +0400
+++ openttd-0.4.0.1-vsprintf/ttd.c	2005-08-15 20:33:33.000000000 +0400
@@ -70,7 +70,7 @@ void CDECL error(const char *s, ...) {
 	va_list va;
 	char buf[512];
 	va_start(va, s);
-	vsprintf(buf, s, va);
+	vsnprintf(buf, sizeof(buf), s, va);
 	va_end(va);
 
 	ShowOSErrorBox(buf);
@@ -86,7 +86,7 @@ void CDECL ShowInfoF(const char *str, ..
 	va_list va;
 	char buf[1024];
 	va_start(va, str);
-	vsprintf(buf, str, va);
+	vsnprintf(buf, sizeof(buf), str, va);
 	va_end(va);
 	ShowInfo(buf);
 }
@@ -99,7 +99,7 @@ char * CDECL str_fmt(const char *str, ..
 	char *p;
 
 	va_start(va, str);
-	len = vsprintf(buf, str, va);
+	len = vsnprintf(buf, sizeof(buf), str, va);
 	va_end(va);
 	p = malloc(len + 1);
 	if (p)
diff -uprN openttd-0.4.0.1/win32.c openttd-0.4.0.1-vsprintf/win32.c
--- openttd-0.4.0.1/win32.c	2005-05-16 20:19:32.000000000 +0400
+++ openttd-0.4.0.1-vsprintf/win32.c	2005-08-15 20:37:31.000000000 +0400
@@ -841,7 +841,7 @@ static long CDECL MidiSendCommand(const 
 	char buf[512];
 
 	va_start(va, cmd);
-	vsprintf(buf, cmd, va);
+	vsnprintf(buf, sizeof(buf), cmd, va);
 	va_end(va);
 	return mciSendStringA(buf, NULL, 0, 0);
 }

Auditors please advise. 

openttd is not installed suid or sgid, therefore there is no security impact 
from the expansion of environment variables.

The win32.c looks like it's conditionally compiled on windows systems only, and 
therefore is not an issue for Gentoo Linux users, however upstream might like to 
be informed so they can investigate.

the NetworkTextMessage() does not look like it can be passed enough information 
to overflow that buffer, Reporter: have you have tested this issue? There might 
be a format string issue via player names, but I dont have any TTD files to 
verify this.

The same for AddTextMessage() which is only called via NetworkTextMessage(), and 
does not appear to be an issue.

os2.c does not appear to be compiled on non-os2 platforms, and therefore is not 
an issue for Gentoo Linux users.

strgen.c appears to be a component of a tool used during build, and is not 
installed as part of the final package. Therefore there is no potential for 
security issues.

I checked every call to error(), it's mostly called with literal arguments, 
except in a few places that include filenames and so on. As loading data files 
should be considered a trusted operation, this doesn not appear to be an issue, 
however upstream might be interested in your patch as a proactive step against 
future typos.

The same for ShowInfoF(), the only attack vector seems to be via reading 
malformed ini files, which should be a trusted operation.

str_fmt() also appears to only deal with opening language packs and so on.

The 3.6-r1 version is the only version marked stable on a security supported 
architecture (ppc).

Reporter: if you have any testcases that demonstrate these bugs, please include 
them in your bug report to help speed up the auditing process.

I found a copy of the game online and attempted to trigger these issues,
however 
I could not, handing this over to the games team to see how they want to 
proceed.

Note about format bugs was damn right. Let's cook a malicious client.

network_client.c:
   228	// Send an quit-packet over the network
   229	DEF_CLIENT_SEND_COMMAND_PARAM(PACKET_CLIENT_QUIT)(const char *leavemsg)
   230	{
   231		//
   232		// Packet: CLIENT_QUIT
   233		// Function: The client is quiting the game
   234		// Data:
   235		//    String: leave-message
   236		//
   237		Packet *p = NetworkSend_Init(PACKET_CLIENT_QUIT);
   238	
   239	===>	NetworkSend_string(p, leavemsg);   <===
   240		NetworkSend_Packet(p, MY_CLIENT);
   241	}

Usually leavemsg = "leave", but we'll make it "%n".

On server side:

network_server.c:
   921	DEF_SERVER_RECEIVE_COMMAND(PACKET_CLIENT_QUIT)
   922	{
   923		// The client wants to leave. Display this and report it to the other
   924		//  clients.
   925		NetworkClientState *new_cs;
   926		char str[100];
   927		char client_name[NETWORK_CLIENT_NAME_LENGTH];
   928	
   929		// The client was never joined.. thank the client for the packet, but
ignore it
   930		if (cs->status < STATUS_DONE_MAP || cs->quited) {
   931			cs->quited = true;
   932			return;
   933		}
   934	
   935	===>	NetworkRecv_string(cs, p, str, 100);   <===
   936	
   937		NetworkGetClientName(client_name, sizeof(client_name), cs);
   938	
   939	===>	NetworkTextMessage(NETWORK_ACTION_LEAVE, 1, false, client_name,
str);   <===

Server usually gets "leave", but now it's "%n" which is passed straight to
vsprintf() in NetworkTextMessage().

/me recompiles, starts server, starts "client", joins, quits.
Yup, "Segmentation fault" on server side.

For now, all places with
	NetworkRecv_string(,,str,);
	NetworkTextMessage(,,,,str);
are invitations for DoS. Tavis?

Ah, okay, that does look like an issue. Reassigning back to security.

Should be an easy fix,

- NetworkTextMessage(NETWORK_ACTION_LEAVE, 1, false, client_name, str);
+ NetworkTextMessage(NETWORK_ACTION_LEAVE, 1, false, client_name, "%s", str);

How nice...

start server, client, join. In client type:
` (backtick, quake console appears)
name %n

Bye-bye server.

After an obvious fix to
network_server.c::DEF_SERVER_RECEIVE_COMMAND(PACKET_CLIENT_SET_NAME), server
survives, client dies. Investigating further...

Fix for client not crashing goes to network_client.c, line 347.

Created an attachment (id=66105) [details]
patch to fix possible buffer overflows + 3 + 1 fornat string bugs

Chunk to console_cmds.c isn't tested. It's late here and I can't find
codepath[s] in question. Other NetworkTextMessage()-related changes are tested.


P.S.: silly half-sleeping question: does vsnprintf() exist on OS/2 at all?

games team please advise.

adding ppc, as they're the only affected security supported arch.

Stable on ppc.

hansmi: in fact the version in Portage is not fixed, apparently you were called
to give inputs rather than to mark stable.

Sorry, I was half-sleeping yesterday. The patch looks good and doesn't contain
any platform-specific things. Also, it compiled here. But I'm not games@.

Security team, please remember to cc the maintainer.

dholm is currently moving and might not return in the next days, see /devaway/

Can someone in games herd commit the fix while the dedicated maintainer is away
?
Alexey: did you send your patch upstream ?

Yes.

masked until dholm can deal with it.

Hm. Theorically if it stays that way we need to issue a masking GLSA for this
one.  I guess we should rather find someone to commit the patch, since hansmi
tested it OK and it's rather straightforward.

vapier: want to have a shot at it ?

vapier should handle the patching tonight.

Slightly different patch was commited to openttd SVN:

http://svn.openttd.org/cgi-bin/viewcvs.cgi?rev=2899&view=rev

M trunk/console_cmds.c
M trunk/network.c
M trunk/network_client.c
M trunk/network_server.c
M trunk/texteff.c

i merged your fixes with the upstream changes ... think you could convince them
to merge the rest of your vsprintf -> vnsprintf changes please ?

0.4.0.1-r1 now in portage stable for arches

This one is ready for GLSA. Anyone wants to contact upstream? 

upstream has already merged most of the fixes into their svn ... i'm hoping
Alexey can convince them to merge the remaining vsprintf->vsnprintf changes

I asked MITRE for CAN numbers.

So far nobody said "I will" or "I won't" at #openttd.

Use :
CAN-2005-2763 for the format strings
CAN-2005-2764 for the buffer overflows

Alexey, please forward the CAN numbers to your OpenTTD contacts. We shall
release a GLSA very soon, crediting you as the discoverer. Feel free to come up
with your own announcement if you want.

GLSA 200509-03
Thanks to everybody involved.