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-head mailing list