Date: Tue, 4 Aug 2015 12:47:27 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r286268 - head/usr.bin/wall Message-ID: <55C0FAAF.4010704@FreeBSD.org> In-Reply-To: <20150804155237.L1126@besplex.bde.org> References: <201508040256.t742uWfQ053686@repo.freebsd.org> <20150804155237.L1126@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55C0FAAF.4010704>