tags 413296 + patch fixed-upstream tags 413299 + patch fixed-upstream thanks
Ah, I see... this has already been reported and fixed upstream. Attached is the latest upstream commit, fixing both reported security holes (and some more?). svn -r691:693 diff svn://radscan.com/conquest/trunk > 691-693.diff -- Regards, Andreas Henriksson
Index: nWelcome.c =================================================================== --- nWelcome.c (revision 691) +++ nWelcome.c (revision 693) @@ -104,12 +104,20 @@ switch (pkttype) { case SP_CLIENTSTAT: - scstat = (spClientStat_t *)buf; - - Context.unum = (int)ntohs(scstat->unum); - Context.snum = scstat->snum; - Ships[Context.snum].team = scstat->team; - done = TRUE; + if ((scstat = chkClientStat(buf))) + { + Context.unum = scstat->unum; + Context.snum = scstat->snum; + Ships[Context.snum].team = scstat->team; + done = TRUE; + } + else + { + clog("nWelcomeInit: invalid CLIENTSTAT"); + fatal = TRUE; + done = TRUE; + return; + } break; case SP_ACK: sack = *(spAck_t *)buf; Index: meta.c =================================================================== --- meta.c (revision 691) +++ meta.c (revision 693) @@ -352,12 +352,14 @@ /* contact a meta server, and return a pointer to a static array of metaSRec_t's coresponding to the server list. returns number of servers found, or ERR if error */ +#define SERVER_BUFSIZE 1024 + int metaGetServerList(char *remotehost, metaSRec_t **srvlist) { static metaSRec_t servers[META_MAXSERVERS]; struct sockaddr_in sa; struct hostent *hp; - char buf[1024]; /* server buffer */ + char buf[SERVER_BUFSIZE]; /* server buffer */ int off; int firsttime = TRUE; int s; /* socket */ @@ -405,7 +407,7 @@ off = 0; while (read(s, &c, 1) > 0) { - if (c != '\n') + if (c != '\n' && off < (SERVER_BUFSIZE - 1)) { buf[off++] = c; } @@ -414,11 +416,19 @@ buf[off] = 0; /* convert to a metaSRec_t */ - if (str2srec(&servers[nums], buf)) - nums++; + if (nums < META_MAXSERVERS) + { + if (str2srec(&servers[nums], buf)) + nums++; + else + clog("metaGetServerList: str2srec(%s) failed, skipping", buf); + } else - clog("metaGetServerList: str2srec(%s) failed, skipping", buf); - + { + clog("metaGetServerList: num servers exceeds %d, skipping", + META_MAXSERVERS); + } + off = 0; } } Index: nPlay.c =================================================================== --- nPlay.c (revision 691) +++ nPlay.c (revision 693) @@ -124,13 +124,25 @@ break; case SP_CLIENTSTAT: - scstat = *(spClientStat_t *)buf; /* make a copy... */ - /* first things first */ - Context.unum = (int)ntohs(scstat.unum); - Context.snum = scstat.snum; - Ships[Context.snum].team = scstat.team; - - return TRUE; + { + spClientStat_t *scstatp; + + if ((scstatp = chkClientStat(buf))) + { + scstat = *scstatp; /* make a copy */ + /* first things first */ + Context.unum = scstat.unum; + Context.snum = scstat.snum; + Ships[Context.snum].team = scstat.team; + + return TRUE; + } + else + { + clog("nPlay: _newship: invalid CLIENTSTAT"); + return FALSE; + } + } break; /* we might get other packets too */ Index: client.c =================================================================== --- client.c (revision 691) +++ client.c (revision 693) @@ -984,11 +984,13 @@ break; case SP_CLIENTSTAT: - scstat = (spClientStat_t *)buf; - Context.snum = scstat->snum; - Context.unum = (int)ntohs(scstat->unum); - Ships[Context.snum].team = scstat->team; - clientFlags = scstat->flags; + if ((scstat = chkClientStat(buf))) + { + Context.snum = scstat->snum; + Context.unum = scstat->unum; + Ships[Context.snum].team = scstat->team; + clientFlags = scstat->flags; + } break; case SP_MESSAGE: @@ -1066,3 +1068,45 @@ return; } + +/* this function accepts a character buffer representing a clientstat packet + and validates it. It return a pointer to a static spClientStat_t + packet if everything is in order, NULL otherwise. */ +spClientStat_t *chkClientStat(char *buf) +{ + static spClientStat_t scstat; + + if (!buf) + return NULL; + + scstat = *(spClientStat_t *)buf; + + scstat.unum = (Unsgn16)ntohs(scstat.unum); + + if (scstat.unum >= MAXUSERS) + { +#if defined(DEBUG_PKT) + clog("%s: unum not in valid range", __FUNCTION__); +#endif + return NULL; + } + + if (scstat.snum < 1 || scstat.snum > MAXSHIPS) + { +#if defined(DEBUG_PKT) + clog("%s: snum not in valid range", __FUNCTION__); +#endif + return NULL; + } + + if (scstat.team >= NUMALLTEAMS) + { +#if defined(DEBUG_PKT) + clog("%s: team not in valid range", __FUNCTION__); +#endif + return NULL; + } + + return &scstat; +} + Index: HISTORY =================================================================== --- HISTORY (revision 691) +++ HISTORY (revision 693) @@ -9,6 +9,14 @@ Conquest HISTORY +??? + +3/3/2006 + + - fixed some security (possble buffer overruns) in the client + (meta.c and CLIENTSTAT processing) reported by Luigi Auriemma. + + 8.2a (devel) 11/27/2006 - Added sound support using SDL and the SDL_mixer API based on Index: client.h =================================================================== --- client.h (revision 691) +++ client.h (revision 693) @@ -91,4 +91,6 @@ void sendUDPKeepAlive(Unsgn32 timebase); +spClientStat_t *chkClientStat(char *buf); + #endif /* CLIENT_H_INCLUDED */ Index: conquest.c =================================================================== --- conquest.c (revision 691) +++ conquest.c (revision 693) @@ -3132,7 +3132,7 @@ break; default: - clog("conquest:newship: unexpected ack code %d", + clog("newship: unexpected ack code %d", sack->code); break; } @@ -3141,22 +3141,28 @@ break; case SP_CLIENTSTAT: - scstat = (spClientStat_t *)buf; + if ((scstat = chkClientStat(buf))) + { + /* first things first */ + Context.unum = scstat->unum; + Context.snum = scstat->snum; + Ships[Context.snum].team = scstat->team; + + if (scstat->esystem == 0) /* we are done */ + return TRUE; + + /* otherwise, need to prompt for system to enter */ + if (selectentry(scstat->esystem)) + return TRUE; /* done */ + else + return FALSE; + } + else + { + clog("newship: invalid CLIENTSTAT\n"); + return FALSE; /* don't want to hang if a bad pkt */ + } - /* first things first */ - Context.unum = (int)ntohs(scstat->unum); - Context.snum = scstat->snum; - Ships[Context.snum].team = scstat->team; - - if (scstat->esystem == 0) /* we are done */ - return TRUE; - - /* otherwise, need to prompt for system to enter */ - if (selectentry(scstat->esystem)) - return TRUE; /* done */ - else - return FALSE; - break; /* we might get other packets too */ @@ -3322,7 +3328,7 @@ char * selected_str="You have been selected to command a"; char * starship_str=" starship."; char * prepare_str="Prepare to be beamed aboard..."; - spClientStat_t scstat = {}; + spClientStat_t *scstat; spAck_t *sack = NULL; int pkttype; Unsgn8 buf[PKT_MAXSIZE]; @@ -3350,12 +3356,18 @@ switch (pkttype) { case SP_CLIENTSTAT: - scstat = *(spClientStat_t *)buf; - - *unum = (int)ntohs(scstat.unum); - Context.snum = scstat.snum; - Ships[Context.snum].team = scstat.team; - done = TRUE; + if ((scstat = chkClientStat(buf))) + { + *unum = scstat->unum; + Context.snum = scstat->snum; + Ships[Context.snum].team = scstat->team; + done = TRUE; + } + else + { + clog("welcome: invalid CLIENTSTAT\n"); + return FALSE; + } break; case SP_ACK: @@ -3370,7 +3382,7 @@ } } - if ( pkttype == SP_CLIENTSTAT && (scstat.flags & SPCLNTSTAT_FLAG_NEW) ) + if ( pkttype == SP_CLIENTSTAT && (scstat->flags & SPCLNTSTAT_FLAG_NEW) ) { /* Must be a new player. */ cdclear(); @@ -3385,7 +3397,7 @@ c_sleep( 2.0 ); return ( FALSE ); } - team = scstat.team; + team = scstat->team; cbuf[0] = EOS; apptitle( team, cbuf ); appchr( ' ', cbuf );