svn commit: r286268 - head/usr.bin/wall
Pedro Giffuni
pfg at FreeBSD.org
Tue Aug 4 17:47:33 UTC 2015
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.
More information about the svn-src-all
mailing list