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 );
