From owner-freebsd-security  Tue Aug 29 14:24:36 1995
Return-Path: security-owner
Received: (from majordom@localhost)
          by freefall.FreeBSD.org (8.6.11/8.6.6) id OAA24722
          for security-outgoing; Tue, 29 Aug 1995 14:24:36 -0700
Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.34])
          by freefall.FreeBSD.org (8.6.11/8.6.6) with ESMTP id OAA24716
          ; Tue, 29 Aug 1995 14:24:25 -0700
Received: (from bde@localhost) by godzilla.zeta.org.au (8.6.9/8.6.9) id HAA01499; Wed, 30 Aug 1995 07:21:24 +1000
Date: Wed, 30 Aug 1995 07:21:24 +1000
From: Bruce Evans <bde@zeta.org.au>
Message-Id: <199508292121.HAA01499@godzilla.zeta.org.au>
To: guido@spooky.lss.cp.philips.com, pst@freefall.FreeBSD.org
Subject: Re: please code review proposed fix for syslog problem
Cc: security@freefall.FreeBSD.org
Sender: security-owner@FreeBSD.org
Precedence: bulk

>One question: what if SPACELEFT(X) is smaller then zero? 
>snprintf assumes size_t which is unsigned....Should this be guarded
>against? (I always mix up signed unsigned operations and am
>to lazy to look it up right now)

Yes, it should.  However, all of the SPACELEFT()s except the one
in the loop for copying the format are guaranteed to be large and
positive unless LogTag is unreasonably long.  The problems only start
occurring when user-supplied data is handled.

>>   	if (LogTag != NULL) {
>>   		*p++ = ':';
>>   		*p++ = ' ';
>		^^^ can overwrite outside of the buffer as well.XXX

Not a big problem, because the user data hasn't been handled yet.

>>   	for (t = fmt_cpy; ch = *fmt; ++fmt)
>>   		if (ch == '%' && fmt[1] == 'm') {
>>   			++fmt;
>> ! 			t += snprintf(t, sizeof(fmt_cpy) - (t - fmt_cpy), "%s",
>> ! 				      strerror(saved_errno));

Another bug: snprintf() returns the number of chars that would be printed
if the fitted, so t may end up after the end of the array.  This causes
pedantically undefined behaviour.  In practice, (t - fmt_copy) will
probably be > sizeof(fmt_cpy), so the space will be "negative" (actually
about SIZE_T_MAX).

Robust library code sure is hard to write.  I prefer kernel code :-).

Bruce