Nmap Development mailing list archives
[patch] fix strncpy() management
From: Vasily Kulikov <segoon () openwall com>
Date: Tue, 13 Aug 2013 12:04:36 +0400
The patch fixes several strncpy() calls to properly set '\0' in the end of the string. The bugs were found with the help of the following coccinelle script: @@ expression d, s, n; statement S; @@ ( strncpy(d, s, n); d[n-1] = \( 0 \| '\0' \) ; | strncpy((char*)d, s, n); d[n-1] = \( 0 \| '\0' \) ; | strncpy(d, s, sizeof(d)-1); d[sizeof(d)-1] = \( 0 \| '\0' \) ; | strncpy(d, s, n); - d[n] + d[n-1] = \( 0 \| '\0' \) ; | strncpy(d, s, n); + d[n-1] = 0; ) All found cases were manually checked, some of them were false positives. Some code doesn't need = '\0', but needs changes to the size argument. strncpy() from libpcap/ need additional review as it looks like several strncats deliberately don't add zero at the end of the string to satisfy setsockopt/getsockopt API. -- FPEngine.cc | 1 + idle_scan.cc | 1 + libnetutil/netutil.cc | 2 ++ libpcre/pcreposix.c | 2 +- ncat/ncat_connect.c | 1 + ncat/ncat_main.c | 2 ++ nping/NpingOps.cc | 1 + nping/NpingTargets.cc | 4 +++- nping/ProbeMode.cc | 1 + nping/utils_net.cc | 6 ++++-- nse_pcrelib.cc | 1 + output.cc | 4 ++++ tcpip.cc | 2 ++ 13 files changed, 24 insertions(+), 4 deletions(-) -- Index: output.cc =================================================================== --- output.cc (revision 31756) +++ output.cc (working copy) @@ -1973,6 +1973,7 @@ time_t lastboot; lastboot = (time_t) currenths->seq.lastboot; strncpy(tmbuf, ctime(&lastboot), sizeof(tmbuf)); + tmbuf[sizeof(tmbuf)-1] = 0; chomp(tmbuf); gettimeofday(&tv, NULL); if (o.verbose) @@ -2102,6 +2103,7 @@ if (!hostname_tbl[i][0]) { numhostnames++; strncpy(&hostname_tbl[i][0], sd.hostname, sizeof(hostname_tbl[i])); + hostname_tbl[i][sizeof(hostname_tbl[i])-1] = 0; break; } } @@ -2115,6 +2117,7 @@ if (!ostype_tbl[i][0]) { numostypes++; strncpy(&ostype_tbl[i][0], sd.ostype, sizeof(ostype_tbl[i])); + ostype_tbl[i][sizeof(ostype_tbl[i])-1] = 0; break; } } @@ -2128,6 +2131,7 @@ if (!devicetype_tbl[i][0]) { numdevicetypes++; strncpy(&devicetype_tbl[i][0], sd.devicetype, sizeof(devicetype_tbl[i])); + devicetype_tbl[i][sizeof(devicetype_tbl[i])-1] = 0; break; } } Index: tcpip.cc =================================================================== --- tcpip.cc (revision 31756) +++ tcpip.cc (working copy) @@ -1253,6 +1253,7 @@ realfrag = htons(ntohs(ip->ip_off) & IP_OFFMASK); tot_len = htons(ip->ip_len); strncpy(sourcehost, inet_ntoa(bullshit), 16); + sourcehost[15] = 0; i = 4 * (ntohs(ip->ip_hl) + ntohs(tcp->th_off)); if (ip->ip_p == IPPROTO_TCP) { if (realfrag) @@ -1329,6 +1330,7 @@ realfrag = htons(ntohs(ip->ip_off) & IP_OFFMASK); tot_len = htons(ip->ip_len); strncpy(sourcehost, inet_ntoa(bullshit), 16); + sourcehost[15] = 0; i = 4 * (ntohs(ip->ip_hl)) + 8; if (ip->ip_p == IPPROTO_UDP) { if (realfrag) Index: nse_pcrelib.cc =================================================================== --- nse_pcrelib.cc (revision 31756) +++ nse_pcrelib.cc (working copy) @@ -92,6 +92,7 @@ luaL_error(L, "cannot set locale"); strncpy(old_locale, setlocale(LC_CTYPE, NULL), 255); /* store the locale */ + old_locale[255] = 0; if(setlocale(LC_CTYPE, locale) == NULL) /* set new locale */ luaL_error(L, "cannot set locale"); Index: ncat/ncat_connect.c =================================================================== --- ncat/ncat_connect.c (revision 31756) +++ ncat/ncat_connect.c (working copy) @@ -565,6 +565,7 @@ srcaddr.un.sun_family = AF_UNIX; strncpy(srcaddr.un.sun_path, tmp_name, sizeof(srcaddr.un.sun_path)); + srcaddr.un.sun_path[sizeof(srcaddr.un.sun_path) - 1] = 0; free (tmp_name); } nsi_set_localaddr(cs.sock_nsi, &srcaddr.storage, SUN_LEN((struct sockaddr_un *)&srcaddr.storage)); Index: ncat/ncat_main.c =================================================================== --- ncat/ncat_main.c (revision 31756) +++ ncat/ncat_main.c (working copy) @@ -705,6 +705,7 @@ if (o.proto == IPPROTO_UDP) { srcaddr.un.sun_family = AF_UNIX; strncpy(srcaddr.un.sun_path, source, sizeof(srcaddr.un.sun_path)); + srcaddr.un.sun_path[sizeof(srcaddr.un.sun_path) - 1] = 0; srcaddrlen = SUN_LEN(&srcaddr.un); } else @@ -740,6 +741,7 @@ memset(&targetss.storage, 0, sizeof(struct sockaddr_un)); targetss.un.sun_family = AF_UNIX; strncpy(targetss.un.sun_path, argv[optind], sizeof(targetss.un.sun_path)); + targetss.un.sun_path[sizeof(targetss.un.sun_path) - 1] = 0; targetsslen = SUN_LEN(&targetss.un); o.target = argv[optind]; optind++; Index: libnetutil/netutil.cc =================================================================== --- libnetutil/netutil.cc (revision 31756) +++ libnetutil/netutil.cc (working copy) @@ -1626,7 +1626,9 @@ char gwbuf[INET6_ADDRSTRLEN]; strncpy(destbuf, inet_ntop_ez(&dcrn->routes[i].dest, sizeof(dcrn->routes[i].dest)), sizeof(destbuf)); + destbuf[sizeof(destbuf)-1] = 0; strncpy(gwbuf, inet_ntop_ez(&dcrn->routes[i].gw, sizeof(dcrn->routes[i].gw)), sizeof(gwbuf)); + gwbuf[sizeof(gwbuf)-1] = 0; /* netutil_error("WARNING: Unable to find appropriate interface for system route to %s/%u gw %s", destbuf, dcrn->routes[i].netmask_bits, gwbuf); Index: libpcre/pcreposix.c =================================================================== --- libpcre/pcreposix.c (revision 31756) +++ libpcre/pcreposix.c (working copy) @@ -173,7 +173,7 @@ addmessage = " at offset "; addlength = (preg != NULL && (int)preg->re_erroffset != -1)? - strlen(addmessage) + 6 : 0; + strlen(addmessage) + 7 : 0; if (errbuf_size > 0) { Index: FPEngine.cc =================================================================== --- FPEngine.cc (revision 31756) +++ FPEngine.cc (working copy) @@ -2463,6 +2463,7 @@ this->link_eth = true; if (devname != NULL) { strncpy(this->eth_hdr.devname, devname, sizeof(this->eth_hdr.devname)-1); + this->eth_hdr.devname[sizeof(this->eth_hdr.devname)-1] = 0; if ((this->eth_hdr.ethsd = eth_open_cached(devname)) == NULL) fatal("%s: Failed to open ethernet device (%s)", __func__, devname); } else { Index: nping/NpingTargets.cc =================================================================== --- nping/NpingTargets.cc (revision 31756) +++ nping/NpingTargets.cc (working copy) @@ -222,8 +222,10 @@ memcpy( t, &next, sizeof( struct sockaddr_storage ) ); /* If current spec is a named host (not a range), store name in supplied buff */ if(current_group.get_namedhost()){ - if( hname!=NULL && hlen>0 ) + if( hname!=NULL && hlen>0 ) { strncpy(hname, specs[ current_spec ], hlen); + hname[hlen-1] = 0; + } }else{ /* If current spec is not a named host, insert NULL in the first position */ if( hname!=NULL && hlen>0 ) hname[0]='\0'; Index: nping/utils_net.cc =================================================================== --- nping/utils_net.cc (revision 31756) +++ nping/utils_net.cc (working copy) @@ -620,7 +620,7 @@ /* Store info */ memset(&(archive[current_index]), 0, sizeof(cached_host_t) ); - strncpy(archive[current_index].hostname, host, MAX_CACHED_HOSTNAME_LEN); + strncpy(archive[current_index].hostname, host, MAX_CACHED_HOSTNAME_LEN-1); archive[current_index].sslen = *sslen; memcpy(&(archive[current_index].ss), ss, *sslen); @@ -712,7 +712,7 @@ /* Store the hostent entry in the cache */ memset(&(archive[current_index]), 0, sizeof(gethostbynamecached_t) ); - strncpy(archive[current_index].hostname, host, MAX_CACHED_HOSTNAME_LEN); + strncpy(archive[current_index].hostname, host, MAX_CACHED_HOSTNAME_LEN-1); archive[current_index].h = hostentcpy( result ); /* Return the entry that we've just added */ @@ -1057,6 +1057,7 @@ } strncpy((char*)dstbuff, protoinfo, dstlen); + dstbuff[dstlen-1] = 0; return OP_SUCCESS; @@ -1129,6 +1130,7 @@ } strncpy((char*)dstbuff, protoinfo, dstlen); + dstbuff[dstlen-1] = 0; return OP_SUCCESS; Index: nping/ProbeMode.cc =================================================================== --- nping/ProbeMode.cc (revision 31756) +++ nping/ProbeMode.cc (working copy) @@ -1336,6 +1336,7 @@ strncpy(filterstring, buffer, sizeof(filterstring)-1); else strncpy(filterstring, "", 2); + filterstring[sizeof(filterstring)-1] = 0; nping_print(DBG_1, "BPF-filter: %s", filterstring); return filterstring; } Index: nping/NpingOps.cc =================================================================== --- nping/NpingOps.cc (revision 31756) +++ nping/NpingOps.cc (working copy) @@ -2248,6 +2248,7 @@ int NpingOps::setEchoPassphrase(const char *str){ strncpy(this->echo_passphrase, str, sizeof(echo_passphrase)-1); + this->echo_passphrase[sizeof(echo_passphrase) - 1] = 0; this->echo_passphrase_set=true; return OP_SUCCESS; } /* End of setEchoPassphrase() */ Index: idle_scan.cc =================================================================== --- idle_scan.cc (revision 31756) +++ idle_scan.cc (working copy) @@ -1046,6 +1046,7 @@ if (!*lastproxy) { initialize_idleproxy(&proxy, proxyName, target->v4hostip(), ports); strncpy(lastproxy, proxyName, sizeof(lastproxy)); + lastproxy[sizeof(lastproxy)-1] = 0; } /* If we don't have timing infoz for the new target, we'll use values -- Vasily Kulikov http://www.openwall.com - bringing security into open computing environments _______________________________________________ Sent through the dev mailing list http://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/
Current thread:
- [patch] fix strncpy() management Vasily Kulikov (Aug 13)
- Re: [patch] fix strncpy() management David Fifield (Aug 16)
- Re: [patch] fix strncpy() management Vasily Kulikov (Aug 25)
- Re: [patch] fix strncpy() management David Fifield (Sep 04)
- Re: [patch] fix strncpy() management Vasily Kulikov (Aug 25)
- Re: [patch] fix strncpy() management David Fifield (Aug 16)