Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!

Bug 102631

Summary: games-simulation/openttd: format string vulnerabilities
Product: Gentoo Security Reporter: Alexey Dobriyan <adobriyan>
Component: VulnerabilitiesAssignee: Gentoo Security <security>
Status: RESOLVED FIXED    
Severity: major CC: dholm
Priority: High    
Version: unspecified   
Hardware: All   
OS: Linux   
Whiteboard: B1 [glsa]
Package list:
Runtime testing required: ---
Attachments:
Description Flags
patch to fix possible buffer overflows + format string bugs none

Description Alexey Dobriyan 2005-08-15 10:35:12 UTC
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);
 }
Comment 1 Sune Kloppenborg Jeppesen (RETIRED) gentoo-dev 2005-08-16 00:09:41 UTC
Auditors please advise. 
Comment 2 Tavis Ormandy (RETIRED) gentoo-dev 2005-08-16 02:11:33 UTC
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.
Comment 3 Tavis Ormandy (RETIRED) gentoo-dev 2005-08-16 06:54:47 UTC
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.
Comment 4 Alexey Dobriyan 2005-08-16 10:39:48 UTC
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?
Comment 5 Tavis Ormandy (RETIRED) gentoo-dev 2005-08-16 10:47:13 UTC
Ah, okay, that does look like an issue. Reassigning back to security.
Comment 6 Tavis Ormandy (RETIRED) gentoo-dev 2005-08-16 10:51:48 UTC
Should be an easy fix,

- NetworkTextMessage(NETWORK_ACTION_LEAVE, 1, false, client_name, str);
+ NetworkTextMessage(NETWORK_ACTION_LEAVE, 1, false, client_name, "%s", str);
Comment 7 Alexey Dobriyan 2005-08-16 14:44:24 UTC
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...
Comment 8 Alexey Dobriyan 2005-08-16 14:57:14 UTC
Fix for client not crashing goes to network_client.c, line 347.



Comment 9 Alexey Dobriyan 2005-08-16 15:40:39 UTC
Created attachment 66105 [details, diff]
patch to fix possible buffer overflows + format 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?
Comment 10 Thierry Carrez (RETIRED) gentoo-dev 2005-08-18 10:04:26 UTC
games team please advise.
Comment 11 Tavis Ormandy (RETIRED) gentoo-dev 2005-08-22 00:49:06 UTC
adding ppc, as they're the only affected security supported arch.
Comment 12 Michael Hanselmann (hansmi) (RETIRED) gentoo-dev 2005-08-22 13:28:53 UTC
Stable on ppc.
Comment 13 Thierry Carrez (RETIRED) gentoo-dev 2005-08-23 01:02:09 UTC
hansmi: in fact the version in Portage is not fixed, apparently you were called
to give inputs rather than to mark stable.
Comment 14 Michael Hanselmann (hansmi) (RETIRED) gentoo-dev 2005-08-23 11:15:20 UTC
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@.
Comment 15 Mr. Bones. (RETIRED) gentoo-dev 2005-08-23 11:24:42 UTC
Security team, please remember to cc the maintainer.
Comment 16 Michael Hanselmann (hansmi) (RETIRED) gentoo-dev 2005-08-23 11:27:11 UTC
dholm is currently moving and might not return in the next days, see /devaway/
Comment 17 Thierry Carrez (RETIRED) gentoo-dev 2005-08-25 11:41:14 UTC
Can someone in games herd commit the fix while the dedicated maintainer is away ?
Alexey: did you send your patch upstream ?
Comment 18 Alexey Dobriyan 2005-08-25 12:40:21 UTC
Yes.
Comment 19 Mr. Bones. (RETIRED) gentoo-dev 2005-08-25 13:10:38 UTC
masked until dholm can deal with it.
Comment 20 Thierry Carrez (RETIRED) gentoo-dev 2005-08-29 07:44:07 UTC
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 ?
Comment 21 Thierry Carrez (RETIRED) gentoo-dev 2005-08-31 08:53:20 UTC
vapier should handle the patching tonight.
Comment 22 Alexey Dobriyan 2005-08-31 10:22:41 UTC
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
Comment 23 SpanKY gentoo-dev 2005-08-31 15:24:53 UTC
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
Comment 24 Sune Kloppenborg Jeppesen (RETIRED) gentoo-dev 2005-08-31 22:04:14 UTC
This one is ready for GLSA. Anyone wants to contact upstream? 
Comment 25 SpanKY gentoo-dev 2005-08-31 22:13:32 UTC
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
Comment 26 Thierry Carrez (RETIRED) gentoo-dev 2005-09-01 00:46:14 UTC
I asked MITRE for CAN numbers.
Comment 27 Alexey Dobriyan 2005-09-01 08:29:53 UTC
So far nobody said "I will" or "I won't" at #openttd.
Comment 28 Thierry Carrez (RETIRED) gentoo-dev 2005-09-01 13:35:52 UTC
Use :
CAN-2005-2763 for the format strings
CAN-2005-2764 for the buffer overflows
Comment 29 Thierry Carrez (RETIRED) gentoo-dev 2005-09-02 03:06:22 UTC
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.
Comment 30 Stefan Cornelius (RETIRED) gentoo-dev 2005-09-05 09:30:17 UTC
GLSA 200509-03
Thanks to everybody involved.