From owner-svn-src-head@freebsd.org Sat Aug 1 15:00:54 2015 Return-Path: Delivered-To: svn-src-head@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 7B8329AF156 for ; Sat, 1 Aug 2015 15:00:54 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from nm29-vm1.bullet.mail.bf1.yahoo.com (nm29-vm1.bullet.mail.bf1.yahoo.com [98.139.213.144]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3D701106B for ; Sat, 1 Aug 2015 15:00:54 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1438441251; bh=OIjdZa74z9/XY5CcJQLoYfSchv+YN4LLsJdfSox228o=; h=Subject:To:References:From:Date:In-Reply-To:From:Subject; b=WL6ANTJzjrrLnazRbbrTqCHKR41+5bY8r0eUR3pcWA0OW/jW8tbEaJzt3bEY1FglWQk9f8UcTdSg1GW0fvVJl0hjOVwwzZsXFQgAqtAiG0w+gHHERZCmiy+sDRLDDAXc+OoqxVNOedIDP1gqNa4aOUlysh9u/0PjLUmHUIs+ZXaCj3lZnXsoF1UGUrVxnkrqAiUFaDQXwHx9lHpZos911gFIlOcahHgotGLGz4iaogS9SMqWKGtJVzq4h1xlPt85ypbEIr2L8EKoEDlbAgyHI0Q42Qj3+5/cjXySwJo+ZqMIvdGpHPdbZM7cjm+8M8Qr7Clvdahqow6FjImIlhOXdw== Received: from [66.196.81.172] by nm29.bullet.mail.bf1.yahoo.com with NNFMP; 01 Aug 2015 15:00:51 -0000 Received: from [98.139.211.161] by tm18.bullet.mail.bf1.yahoo.com with NNFMP; 01 Aug 2015 15:00:51 -0000 Received: from [127.0.0.1] by smtp218.mail.bf1.yahoo.com with NNFMP; 01 Aug 2015 15:00:51 -0000 X-Yahoo-Newman-Id: 603477.82493.bm@smtp218.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: FppnWUgVM1lErMSpuPMpx9PjLc0M7Y03pHG9b_roiSUVmEE QpDs5_tJCYcndMgLY2LTo5t1B0qddgUG9gbhYbgSHYdBb9zmyAPS6U7VYcP6 c3fIxjy1JGv1i91EWk0KZUbx9laPsVqTvqb3ucCWI_VlXzMc.bJw.Zf2bKru ggMKU3eBUlBeU4q_03OxG1dLpCL.VGIRgdxWaq7e1Ntih.SsB3DZEhZZAPfA 38pu1a9sVVnNK.cx8KWTSJ0SwTO0r6MzHqmLQ8wHDPw9PGTzIfZQEupHT4Ae fZzvoKq_qSBl99.pYMRfFcpzoQlR13boJLvqfjIpMC8M.JpDm1jISisVIfG5 C3pJEYm.sI_sMl7ERTMK0joCQGm_dSDjFvB5DaRwFxG3Gj6X.rpH.CvITGz2 BCSyxxe8ArQyf5qkvz3f7PeF86oRVffIm_6j8Eo.qfnXKPHK.cRu48QVtoD7 ZaFW5qK2kDjk55Icazm_vzvOrdPZlfDBiW217MMXIGvl38ODMR6pz78ExCmt 1m2LYd.KFpEnpV4oVpIwkGoNb._aUjJNB X-Yahoo-SMTP: xcjD0guswBAZaPPIbxpWwLcp9Unf Subject: Re: svn commit: r286144 - head/usr.bin/wall To: Eric van Gyzen , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201508010129.t711TuAv036881@repo.freebsd.org> <55BCD8E3.3070607@FreeBSD.org> From: Pedro Giffuni Message-ID: <55BCDF2D.8000700@FreeBSD.org> Date: Sat, 1 Aug 2015 10:01:01 -0500 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <55BCD8E3.3070607@FreeBSD.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Aug 2015 15:00:54 -0000 On 08/01/15 09:34, Eric van Gyzen wrote: > 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)); > It looks exactly like the code I just replaced ;). > Your current code works, and it's even more efficient than strlcat. Indeed, efficient is good and was the reason for the changes suggested by bde. We evaluated snprintf but costs even more. > 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. > Yeah, it's a benign off-by-one :(. I will fix it by using the return value of the original strlcpy so we it will be less confusing at the cost of adding a new variable. This said, the original bug is the first detected by FORTIFY_SOURCE that is not detected by Coverity, so it will be very exciting to bring FORTIFY_SOURCE to the tree. Pedro.