From owner-svn-src-all@freebsd.org Tue Aug 4 17:47:32 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 DC3BC9B2154 for ; Tue, 4 Aug 2015 17:47:32 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from nm13-vm0.bullet.mail.bf1.yahoo.com (nm13-vm0.bullet.mail.bf1.yahoo.com [98.139.213.79]) (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 94FE4D25 for ; Tue, 4 Aug 2015 17:47:32 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1438710445; bh=tstdlUa2CMgtpC/Ne0+nSWJOgFNCBkcZopm1gb4AhFo=; h=Subject:To:References:Cc:From:Date:In-Reply-To:From:Subject; b=Ck///fepolBufMATMeS3Bx3jDXOyCig7kC9JbskZQ+aHnCS3IPMKRB88HM92nk/XWPzq8GhRuqBDPSM/FiS0uSrcpwg648K9NzWL3G4arzOOKf8snlqQaD1t3WWrSo3vCiNi8neYg0ElFXT8cv4dIfq0oM6rjS+64qAPVPjxheWJ2X/Iiz8tji3AaWCrmnijMddN9xntfzNHCe6wFfiUrhnBhlzIPZC9d6yQggkuEY5zopGbLDO+KCosnFSb+WnX3jzIyg0GN42zFR0ga83f4iQ7GNeBw4oFPY8ZFC76ukgvxRrx61rtcKtpCNzhZbRiNTJvfhl5Q7CASACHdIjLGw== Received: from [98.139.170.178] by nm13.bullet.mail.bf1.yahoo.com with NNFMP; 04 Aug 2015 17:47:25 -0000 Received: from [98.139.211.202] by tm21.bullet.mail.bf1.yahoo.com with NNFMP; 04 Aug 2015 17:47:25 -0000 Received: from [127.0.0.1] by smtp211.mail.bf1.yahoo.com with NNFMP; 04 Aug 2015 17:47:25 -0000 X-Yahoo-Newman-Id: 345328.92453.bm@smtp211.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: QoqYy3YVM1mvxpCcBo8qV8Xq4O18w8uFj9TJtVOn79UKx9p 2kqIIlbFmwONntF_UOt1IT0UbIvwJPDS3sj8genWfynWaAh835Y90YT.Sc4p gTw2qljL3_vuLn7UcDi1FGWNGI1QT86vXPoq6_KkKE6hjvYEO1FhL4w9hxRs Rx4Ux9YCkLscdv6XqE6JDy1W5k0e4n0hQt4rVfCvcCqR6XPQZoA.4y5JbuEV Hle_II2WQfTtoSE7W8SZl1rdtgUqwRyWEBRBnfuQUtkKWO6SEk9TnPgmAW6L A7l1Svp.zoCwHdGkxKi_BhLNquQb9UINPLJBpcYViCmJtDLagr6ZXd.qy5Xk JLLL2pc6LLAUnObpBod1CtdOAmtL.5DaPbu07QwriFU2rd83T5JB3rkoBULg kO9zfj0i7PdaP2FdJW3VWXhLw6e72g5L6CRjWAoUPEdpKZSSkf5i6Bd7lLkS T3iKlDCcZoiCQCqaSf1ewX_Iu84IJ3NM4Mqx7uo64dChzHjruq2cZvSjbEfQ h1AOdWVkNPw7b0tdIJDNFBhJ59WC_OHcz X-Yahoo-SMTP: xcjD0guswBAZaPPIbxpWwLcp9Unf Subject: Re: svn commit: r286268 - head/usr.bin/wall To: Bruce Evans References: <201508040256.t742uWfQ053686@repo.freebsd.org> <20150804155237.L1126@besplex.bde.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org From: Pedro Giffuni Message-ID: <55C0FAAF.4010704@FreeBSD.org> Date: Tue, 4 Aug 2015 12:47:27 -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: <20150804155237.L1126@besplex.bde.org> Content-Type: text/plain; charset=windows-1252; 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: Tue, 04 Aug 2015 17:47:33 -0000 On 08/04/15 01:12, Bruce Evans wrote: > On Tue, 4 Aug 2015, Pedro F. Giffuni wrote: > >> Log: >> Revert r286144 leaving the original fix to the buffer overflow. >> >> Some developers consider the new code unnecessarily obfuscated. >> There was also a benign off-by-one. >> >> Discussed with: bde, vangyzen, jmallett > > It is this version that is unnecessarily obfuscated. > I preferred the previous solution, with some adjustment for the off-by-one, but of course I am biased. I am aware you wanted a new fix with strlcat(): the original fix used strlcat so it should be nearer to your expected solution. At this time I really don't want to start a re-write of wall(1) beyond avoiding the buffer overflow. >> Modified: head/usr.bin/wall/ttymsg.c >> ============================================================================== >> >> --- head/usr.bin/wall/ttymsg.c Tue Aug 4 02:41:14 2015 (r286267) >> +++ head/usr.bin/wall/ttymsg.c Tue Aug 4 02:56:31 2015 (r286268) >> @@ -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]; >> + char device[MAXNAMLEN] = _PATH_DEV; >> static char errbuf[1024]; >> char *p; >> int forked; > > Half of the string initialization is done by an auto initializer. > Initializations in declarations are specifically forbidden in style(9), > and this is a good example of how to obfuscate code using them. The > original version using a static initializer was almost as obfuscated, > but it was at least maximally efficient and had a technical reason > for using the initializer. > I understand initialization is done during compile time, while assignments are done in runtime. I personally prefer having the compiler do the effort instead of having the executable do it. Being this a style(9) issue I guess it should be fixed but it is a preexisting condition outside of the scope of the present patch: closing the overflow. >> @@ -71,9 +71,8 @@ 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)"); >> >> - strlcpy(device, _PATH_DEV, sizeof(device)); >> + strlcat(device, line, sizeof(device)); > > The other half of the initialization is done using strlcat(). > >> p = device + sizeof(_PATH_DEV) - 1; >> - strlcpy(p, line, sizeof(device) - sizeof(_PATH_DEV)); >> if (strncmp(p, "pts/", 4) == 0) >> p += 4; >> if (strchr(p, '/') != NULL) { > > In private mail, I pointed out lots more bad code here. The pointer > 'p' was introdiced to avoid writing out the expression for it in > more places, especially in the "pts/" check. But the "pts/" check > is badly written. It checks the copy of 'line' in 'device'. This > is an obfuscated way of checking 'line' itself. It might be more > secure the check the final string, but it is actually more secure > to check 'line' since this will detect slashes that lost by strlcat() > with no error checking. > > This method becomes even more unnatural when combined with the strcat(). > We use the knowledge of the prefix to find the place to start for the > check, but don't use this to find the place to start for the concatenation. > These places are the same, and unwinding the obfuscation in the check > requires knowing that they are the same. > I welcome a patch, but I don't want to run the risk of introducing more changes in this commit. This change is only about fixing a buffer overflow that breaks wall(1) when running it with bounds checking. Pedro.