From owner-freebsd-security  Thu Aug 31 15:03:08 1995
Return-Path: security-owner
Received: (from majordom@localhost)
          by freefall.FreeBSD.org (8.6.11/8.6.6) id PAA28345
          for security-outgoing; Thu, 31 Aug 1995 15:03:08 -0700
Received: from precipice.shockwave.com (precipice.shockwave.com [171.69.108.33])
          by freefall.FreeBSD.org (8.6.11/8.6.6) with ESMTP id PAA28337
          for <freebsd-security@freebsd.org>; Thu, 31 Aug 1995 15:03:05 -0700
Received: from localhost (localhost [127.0.0.1]) by precipice.shockwave.com (8.6.12/8.6.12) with SMTP id PAA03378; Thu, 31 Aug 1995 15:01:20 -0700
Message-Id: <199508312201.PAA03378@precipice.shockwave.com>
To: Bruce Evans <bde@zeta.org.au>
cc: peter@haywire.DIALix.COM, freebsd-security@freebsd.org
Subject: Re: Eric Allman's syslog.c fixes 
In-reply-to: Your message of "Fri, 01 Sep 1995 07:44:47 +1000."
             <199508312144.HAA10330@godzilla.zeta.org.au> 
Date: Thu, 31 Aug 1995 15:01:20 -0700
From: Paul Traina <pst@shockwave.com>
Sender: security-owner@freebsd.org
Precedence: bulk


  From: Bruce Evans <bde@zeta.org.au>
  Subject: Re: Eric Allman's syslog.c fixes
  >Well, this is it, BTW....  Obviously, this has portability stuff in it 
  >that can come out.  Note, it's berkeley version 8.8.  4.4Lite was version 
  >8.4, and Lite2 is version 8.5.  If anybody's got any complaints with this 
  >version of the code, we need to hear about it ASAP, before it gets 
  >published. 
  
  Unfortunately it has many of the bugs that we noticed in the review of pst's
  version.
  
  >	if (LogTag == NULL)
  >		LogTag = __progname;
  >	if (LogTag != NULL) {
  >		sprintf(p, "%s", LogTag);
  		^^^^^^^
  >		p += strlen(p);
  >	}
  
  This can overrun (or cause overruns later) if LogTag is very log.  Perhaps
  this doesn't matter because users can't change LogTag.

I'd rather keep the check here.
  
  >	/* Substitute error message for %m. */
  >	for (t = fmt_cpy; ch = *fmt; ++fmt)
  >		if (ch == '%' && fmt[1] == 'm') {
  >			++fmt;
  >			sprintf(t, "%s", strerror(saved_errno));
  			^^^^^^^
  >			t += strlen(t);
  >		} else
  >			*t++ = ch;
  			^^^^
  >	*t = '\0';
  	^^
  
  More overrun possibilities.  Perhaps they don't matter because users can't
  change the format.

That was my basic feeling.
  
  >#if USESNPRINTF
  >	cnt = maxsend - (p - tbuf) + 1;
  >	p += vsnprintf(p, cnt, fmt_cpy, ap);
  	^^^^
  >	cnt = p - tbuf;
  	^^^
  >#else
  >....
  >#endif
  
  vsnprintf() returns the number of characters that would be written if
  they fitted, so the final pointer and count are bogus if not everything
  fitted.  The most interesting case is if a couple of GB would be written
  and the pointer wraps around.

Yep, bummer.
  
  >	/*
  >	 * Output the message to the console; don't worry about blocking,
  >	 * if console blocks everything will.  Make sure the error reported
  >	 * is the one from the syslogd failure.
  >	 */
  >	if (LogStat & LOG_CONS &&
  >	    (fd = open(_PATH_CONSOLE, O_WRONLY, 0)) >= 0) {
  >		(void)strcat(tbuf, "\r\n");
  >		cnt += 2;
  >		p = strchr(tbuf, '>') + 1;
  >		(void)write(fd, p, cnt - (p - tbuf));
  		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  >		(void)close(fd);
  >	}
  
  The bogus pointer and count may be considered as a feature :-).  They
  may cause junk to be written to the log as evidence of attempted breakins.
  
  Bruce