| Summary: | games-simulation/openttd: format string vulnerabilities | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Gentoo Security | Reporter: | Alexey Dobriyan <adobriyan> | ||||
| Component: | Vulnerabilities | Assignee: | 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: |
|
||||||
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 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?
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. |
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); }