Date: Sat, 1 Aug 2015 09:34:11 -0500 From: Eric van Gyzen <vangyzen@FreeBSD.org> To: "Pedro F. Giffuni" <pfg@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r286144 - head/usr.bin/wall Message-ID: <55BCD8E3.3070607@FreeBSD.org> In-Reply-To: <201508010129.t711TuAv036881@repo.freebsd.org> References: <201508010129.t711TuAv036881@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 7/31/15 8:29 PM, Pedro F. Giffuni wrote: > Author: pfg > Date: Sat Aug 1 01:29:55 2015 > New Revision: 286144 > URL: https://svnweb.freebsd.org/changeset/base/286144 > > Log: > Buffer overflow in wall(1). > > Revert r286102 and apply a cleaner fix. > Tested for overflows by FORTIFY_SOURCE GSoC (with clang). > > Suggested by: bde > Reviewed by: Oliver Pinter > Tested by: Oliver Pinter > MFC after: 3 days > > Modified: > head/usr.bin/wall/ttymsg.c > > Modified: head/usr.bin/wall/ttymsg.c > ============================================================================== > --- head/usr.bin/wall/ttymsg.c Fri Jul 31 23:40:18 2015 (r286143) > +++ head/usr.bin/wall/ttymsg.c Sat Aug 1 01:29:55 2015 (r286144) > @@ -62,7 +62,7 @@ ttymsg(struct iovec *iov, int iovcnt, co > struct iovec localiov[7]; > ssize_t left, wret; > int cnt, fd; > - char device[MAXNAMLEN] = _PATH_DEV; > + char device[MAXNAMLEN]; > static char errbuf[1024]; > char *p; > int forked; > @@ -71,8 +71,9 @@ ttymsg(struct iovec *iov, int iovcnt, co > if (iovcnt > (int)(sizeof(localiov) / sizeof(localiov[0]))) > return ("too many iov's (change code in wall/ttymsg.c)"); > > - strlcat(device, line, sizeof(device)); > + strlcpy(device, _PATH_DEV, sizeof(device)); > p = device + sizeof(_PATH_DEV) - 1; > + strlcpy(p, line, sizeof(device) - sizeof(_PATH_DEV)); You're probably already sick of this change, but I would encourage this instead: strlcat(device, line, sizeof(device)); Your current code works, and it's even more efficient than strlcat. However, doing arithmetic on either the first or third argument to strlcpy/strlcat is precisely what caused the overflow that you're fixing. In fact, the size passed by the current code is one byte too small. This is obviously not a real concern in this code, but it demonstrates how easy it is to get these calculations wrong. > if (strncmp(p, "pts/", 4) == 0) > p += 4; > if (strchr(p, '/') != NULL) { >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55BCD8E3.3070607>