------------------------------------------------------------------------
r288 | auerswald | 2013-08-11 23:28:33 +0200 (Ne, 11 srp 2013) | 21 lines
nasd: Use snprintf, strncpy, and strncat to guard against buffer overflows.
Hamid Zamani <me@hamidx9.ir> reported a list of vulnerabilities in nasd
to the NAS mailing list:
http://radscan.com/pipermail/nas/2013-August/001270.html
Most of these result from using sprintf, strcpy, and strcat with fixed
buffer sizes, but unknown input data. Ensure that the fixed buffers do
not overflow by using snprintf, strncpy, and strncat. Since strncpy
does not guarantee to terminate the copied string with '\0', set the
last position of the destination buffer to '\0' manually.
This commit assumes that snprintf, strncpy, and strncat are available
everywhere nasd is used.
One vulnerability from the list mentioned above is still open. This affects
MINIX only, and Hamid Zamani suggested a fix:
http://radscan.com/pipermail/nas/2013-August/001284.html
------------------------------------------------------------------------
Index: server/os/connection.c
===================================================================
--- server/os/connection.c (revision 287)
+++ server/os/connection.c (revision 288)
@@ -534,8 +534,10 @@
chmod(X_UNIX_DIR, 0777 | S_ISVTX);
#endif
- strcpy(unsock.sun_path, X_UNIX_PATH);
- strcat(unsock.sun_path, display);
+ strncpy(unsock.sun_path, X_UNIX_PATH, sizeof unsock.sun_path);
+ unsock.sun_path[sizeof unsock.sun_path - 1] = '\0';
+ strncat(unsock.sun_path, display,
+ sizeof unsock.sun_path - strlen(unsock.sun_path) - 1);
#ifdef BSD44SOCKETS
unsock.sun_len = strlen(unsock.sun_path);
#endif
@@ -550,12 +552,15 @@
static char oldLinkName[256];
uname(&systemName);
- strcpy(oldLinkName, OLD_UNIX_DIR);
+ strncpy(oldLinkName, OLD_UNIX_DIR, sizeof oldLinkName);
+ oldLinkName[sizeof oldLinkName - 1] = '\0';
if (!mkdir(oldLinkName, 0777))
chown(oldLinkName, 2, 3);
- strcat(oldLinkName, "/");
- strcat(oldLinkName, systemName.nodename);
- strcat(oldLinkName, display);
+ strncat(oldLinkName, "/", sizeof oldLinkName - strlen(oldLinkName) - 1);
+ strncat(oldLinkName, systemName.nodename,
+ sizeof oldLinkName - strlen(oldLinkName) - 1);
+ strncat(oldLinkName, display,
+ sizeof oldLinkName - strlen(oldLinkName) - 1);
unlink(oldLinkName);
symlink(unsock.sun_path, oldLinkName);
}
@@ -568,8 +573,8 @@
i = strlen(unsock.sun_path);
buffer = (char *) malloc(i + 80);
if (buffer) {
- sprintf(buffer, "Error creating unix socket: %s\n",
- unsock.sun_path);
+ snprintf(buffer, i+80, "Error creating unix socket: %s\n",
+ unsock.sun_path);
Error(buffer);
free(buffer);
} else
@@ -591,7 +596,7 @@
i = strlen(unsock.sun_path);
buffer = (char *) malloc(i + 80);
if (buffer) {
- sprintf(buffer, "Error binding unix socket: %s\n",
+ snprintf(buffer, i+80, "Error binding unix socket: %s\n",
unsock.sun_path);
Error(buffer);
free(buffer);
@@ -669,15 +674,15 @@
mkdir(AUDIO_ISC_DIR, 0777);
chmod(AUDIO_ISC_DIR, 0777);
- strcpy(path, AUDIO_ISC_PATH);
+ strncpy(path, AUDIO_ISC_PATH, sizeof path); path[sizeof path - 1] = '\0';
#else /* SVR4_ACP && UNIXCONN */
mkdir(X_UNIX_DIR, 0777);
chmod(X_UNIX_DIR, 0777);
- strcpy(path, X_UNIX_PATH);
+ strncpy(path, X_UNIX_PATH, sizeof path); path[sizeof path - 1] = '\0';
#endif /* SVR4_ACP && UNIXCONN */
- strcat(path, display);
+ strncat(path, display, sizeof path - strlen(path) - 1);
if (unlink(path) < 0 && errno != ENOENT) {
ErrorF("audio server: ISC listener pipe in use (%s)\n", path);
@@ -729,8 +734,8 @@
int fds = -1, fdr = -1;
char pathS[64], pathR[64];
- sprintf(pathS, "%s%sS", AUDIO_XSIGHT_PATH, display);
- sprintf(pathR, "%s%sR", AUDIO_XSIGHT_PATH, display);
+ snprintf(pathS, sizeof pathS, "%s%sS", AUDIO_XSIGHT_PATH, display);
+ snprintf(pathR, sizeof pathR, "%s%sR", AUDIO_XSIGHT_PATH, display);
if ((unlink(pathS) < 0 && errno != ENOENT) ||
(unlink(pathR) < 0 && errno != ENOENT)) {
@@ -802,8 +807,9 @@
mkdir(AUDIO_STREAMS_DIR, 0777);
chmod(AUDIO_STREAMS_DIR, 0777);
- strcpy(path, AUDIO_STREAMS_PATH);
- strcat(path, display);
+ strncpy(path, AUDIO_STREAMS_PATH, sizeof path);
+ path[sizeof path - 1] = '\0';
+ strncat(path, display, sizeof path - strlen(path) - 1);
if ((unlink(path) < 0 && errno != ENOENT)) {
ErrorF("audio server: USL listener pipe in use (%s)\n", path);
@@ -842,8 +848,9 @@
mkdir(AUDIO_STREAMS_DIR, 0777);
chmod(AUDIO_STREAMS_DIR, 0777);
- strcpy(path, AUDIO_NSTREAMS_PATH);
- strcat(path, display);
+ strncpy(path, AUDIO_NSTREAMS_PATH, sizeof path);
+ path[sizeof path - 1] = '\0';
+ strncat(path, display, sizeof path - strlen(path) - 1);
if ((unlink(path) < 0 && errno != ENOENT)) {
ErrorF("audio server: SVR4 named listener pipe in use (%s)\n",
@@ -1039,7 +1046,8 @@
}
bzero((char *) &dnsock, sizeof(dnsock));
dnsock.sdn_family = AF_DECnet;
- sprintf(dnsock.sdn_objname, "AUDIO$%d", atoi(display));
+ snprintf(dnsock.sdn_objname, sizeof(dnsock.sdn_objname), "AUDIO$%d",
+ atoi(display));
dnsock.sdn_objnamel = strlen(dnsock.sdn_objname);
if (bind(request, (struct sockaddr *) &dnsock, sizeof(dnsock))) {
Error("Binding DECnet socket");
@@ -1293,31 +1301,35 @@
{
char addr[128];
- if (!len)
- strcpy(addr, "local host");
+ if (!len) {
+ strncpy(addr, "local host", sizeof addr);
+ addr[sizeof addr - 1] = '\0';
+ }
else
switch (saddr->sa_family) {
case AF_UNSPEC:
#ifdef UNIXCONN
case AF_UNIX:
#endif
- strcpy(addr, "local host");
+ strncpy(addr, "local host", sizeof addr);
+ addr[sizeof addr - 1] = '\0';
break;
#ifdef TCPCONN
case AF_INET:
- sprintf(addr, "IP %s port %d",
+ snprintf(addr, sizeof addr, "IP %s port %d",
inet_ntoa(((struct sockaddr_in *) saddr)->sin_addr),
(int) ntohs(((struct sockaddr_in *) saddr)->sin_port));
break;
#endif
#ifdef DNETCONN
case AF_DECnet:
- sprintf(addr, "DN %s",
+ snprintf(addr, sizeof addr, "DN %s",
dnet_ntoa(&((struct sockaddr_dn *) saddr)->sdn_add));
break;
#endif
default:
- strcpy(addr, "unknown address");
+ strncpy(addr, "unknown address", sizeof addr);
+ addr[sizeof addr - 1] = '\0';
}
if (letin)
AuditF("client %d connected from %s\n", client, addr);
@@ -2115,7 +2127,7 @@
if (AuServerHostName == NULL)
FatalError
("AUDIOHOST not set, or server host name not given\n");
- sprintf(host, "%s/%s:%s", DEF_AUSVRDIR, AuServerHostName,
+ snprintf(host, sizeof host, "%s/%s:%s", DEF_AUSVRDIR, AuServerHostName,
0 /* port */ );
uniqport(&Au.cap_port);
@@ -2366,13 +2378,13 @@
buf[0] = '\0';
if (status == 0)
- sprintf(buf, "NONE");
+ snprintf(buf, sizeof buf, "NONE");
if (status & CONN_KILLED)
- sprintf(buf, "%s KILLED", buf);
+ strncat(buf, " KILLED", sizeof buf - strlen(buf) - 1);
if (status & REQ_PUSHBACK)
- sprintf(buf, "%s PUSHBACK", buf);
+ strncat(buf, " PUSHBACK", sizeof buf - strlen(buf) - 1);
if (status & IGNORE)
- sprintf(buf, "%s IGNORE", buf);
+ strncat(buf, " IGNORE", sizeof buf - strlen(buf) - 1);
return buf;
}
@@ -2466,7 +2478,8 @@
case STD_INFO:
rep.h_status = STD_OK;
- sprintf(repb, "audio server on %s", AuServerHostName);
+ snprintf(repb, REPLY_BUFSIZE, "audio server on %s",
+ AuServerHostName);
rep.h_size = strlen(repb);
putrep(&rep, repb, rep.h_size);
break;
@@ -2631,9 +2644,9 @@
int result;
int looping = 0;
- strncpy(name, AuTcpServerName, BUFSIZ);
+ strncpy(name, AuTcpServerName, BUFSIZ); name[BUFSIZ-1] = '\0';
if ((err = name_lookup(name, &svrcap)) != STD_OK) {
- sprintf(name, "%s/%s", TCP_SVR_NAME, AuTcpServerName);
+ snprintf(name, BUFSIZ, "%s/%s", TCP_SVR_NAME, AuTcpServerName);
if ((err = name_lookup(name, &svrcap)) != STD_OK)
FatalError("Lookup %s failed: %s\n", AuTcpServerName,
err_why(err));
Index: server/os/access.c
===================================================================
--- server/os/access.c (revision 287)
+++ server/os/access.c (revision 288)
@@ -478,9 +478,9 @@
validhosts = host->next;
FreeHost(host);
}
- strcpy(fname, "/etc/X");
- strcat(fname, display);
- strcat(fname, ".hosts");
+ strncpy(fname, "/etc/X", sizeof fname); fname[sizeof fname - 1] = '\0';
+ strncat(fname, display, sizeof fname - strlen(fname) - 1);
+ strncat(fname, ".hosts", sizeof fname - strlen(fname) - 1);
if (fd = fopen(fname, "r")) {
while (fgets(hostname, sizeof(hostname), fd)) {
if (ptr = index(hostname, '\n'))
Index: server/os/osinit.c
===================================================================
--- server/os/osinit.c (revision 287)
+++ server/os/osinit.c (revision 288)
@@ -101,7 +101,7 @@
/* hack test to decide where to log errors */
if (write(2, fname, 0)) {
FILE *err;
- sprintf(fname, ADMPATH, display);
+ snprintf(fname, sizeof fname, ADMPATH, display);
/*
* uses stdio to avoid os dependencies here,
* a real os would use
Index: server/os/aulog.c
===================================================================
--- server/os/aulog.c (revision 287)
+++ server/os/aulog.c (revision 288)
@@ -29,7 +29,7 @@
va_start(ap, fmt);
- (void) vsprintf(buf, fmt, ap);
+ (void) vsnprintf(buf, sizeof buf, fmt, ap);
va_end(ap);
Index: server/os/iopreader.c
===================================================================
--- server/os/iopreader.c (revision 287)
+++ server/os/iopreader.c (revision 288)
@@ -49,7 +49,8 @@
*/
if (AuServerHostName == NULL)
FatalError("No hostname, no screen\n");
- sprintf(host, "%s/%s/%s", HOST_DIR, AuServerHostName, DEF_IOPSVRNAME);
+ snprintf(host, sizeof host, "%s/%s/%s", HOST_DIR, AuServerHostName,
+ DEF_IOPSVRNAME);
if ((err = name_lookup(host, &iopcap)) != STD_OK)
FatalError("Cannot find IOP server %s: %s\n", host, err_why(err));
Index: server/dda/sun/ausuni.c
===================================================================
--- server/dda/sun/ausuni.c (revision 287)
+++ server/dda/sun/ausuni.c (revision 288)
@@ -1368,7 +1368,7 @@
/* pebl: We cannot just concat "ctl" on variable device, so
make a copy and concat "ctl". (free it again) */
if (!(devicectl = (char *) aualloc(strlen(device) +
- strlen("ctl"))))
+ strlen("ctl") + 1)))
return AuFalse;
sprintf(devicectl, "%sctl", device);
Index: server/dda/voxware/auvoxware.c
===================================================================
--- server/dda/voxware/auvoxware.c (revision 287)
+++ server/dda/voxware/auvoxware.c (revision 288)
@@ -1445,7 +1445,8 @@
{
char tempbuf[80];
- sprintf(tempbuf, "\nwriteMono buf: %d size: %d\n", buf, bufSize);
+ snprintf(tempbuf, sizeof tempbuf, "\nwriteMono buf: %d size: %d\n",
+ buf, bufSize);
write(dspout, tempbuf, strlen(tempbuf));
write(dspout, buf, bufSize);
}
@@ -1524,7 +1525,8 @@
{
char tempbuf[80];
- sprintf(tempbuf, "\nwriteStereo buf: %d size: %d\n", buf, bufSize);
+ snprintf(tempbuf, sizeof tempbuf, "\nwriteStereo buf: %d size: %d\n",
+ buf, bufSize);
write(dspout, tempbuf, strlen(tempbuf));
write(dspout, buf, bufSize);
}
@@ -1586,7 +1588,7 @@
#ifdef DEBUGDSPIN
{
char tempbuf[80];
- sprintf(tempbuf, "\nreadInputs buf: %d size: %d\n",
+ snprintf(tempbuf, sizeof tempbuf, "\nreadInputs buf: %d size: %d\n",
stereoInputDevice->minibuf,
stereoInputDevice->bytesPerSample * auMinibufSamples);
write(dspin, tempbuf, strlen(tempbuf));