From owner-svn-src-all@freebsd.org Sat Aug 1 14:34:27 2015 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1389B9B0C0F; Sat, 1 Aug 2015 14:34:27 +0000 (UTC) (envelope-from vangyzen@FreeBSD.org) Received: from smtp.vangyzen.net (hotblack.vangyzen.net [199.48.133.146]) by mx1.freebsd.org (Postfix) with ESMTP id E2B89967; Sat, 1 Aug 2015 14:34:25 +0000 (UTC) (envelope-from vangyzen@FreeBSD.org) Received: from coconut.local (173-19-243-56.client.mchsi.com [173.19.243.56]) by smtp.vangyzen.net (Postfix) with ESMTPSA id DB3F556487; Sat, 1 Aug 2015 09:34:18 -0500 (CDT) Subject: Re: svn commit: r286144 - head/usr.bin/wall To: "Pedro F. Giffuni" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201508010129.t711TuAv036881@repo.freebsd.org> From: Eric van Gyzen Message-ID: <55BCD8E3.3070607@FreeBSD.org> Date: Sat, 1 Aug 2015 09:34:11 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <201508010129.t711TuAv036881@repo.freebsd.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Aug 2015 14:34:27 -0000 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) { >