From owner-freebsd-security  Wed Sep  6 18:19:03 1995
Return-Path: security-owner
Received: (from majordom@localhost)
          by freefall.freebsd.org (8.6.11/8.6.6) id SAA02441
          for security-outgoing; Wed, 6 Sep 1995 18:19:03 -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 SAA02435
          for <security@freebsd.org>; Wed, 6 Sep 1995 18:18:59 -0700
Received: (from bde@localhost) by godzilla.zeta.org.au (8.6.9/8.6.9) id LAA10056; Thu, 7 Sep 1995 11:13:46 +1000
Date: Thu, 7 Sep 1995 11:13:46 +1000
From: Bruce Evans <bde@zeta.org.au>
Message-Id: <199509070113.LAA10056@godzilla.zeta.org.au>
To: pst@shockwave.com, security@freebsd.org
Subject: Re: diffs for syslog.c
Sender: security-owner@freebsd.org
Precedence: bulk

>+/*
>+ * Some rather anal checks to make sure we don't overflow our stack.
>+ * We want to make sure no one can attack either the program stack or
>+ * syslogd's stack.  All writes are limited by SPACELEFT, and an additional
>+ * overflow check is performed to insure that the travelling pointer has
                                    ensure [too English?]
>+ * not exceeded the bounds of the buffer (the return value from snprintf
>+ * is the number of characters it expected to write, not the number of
>+ * characters is did write if the limit was reached).
                it
>+ *
>+ * The overflow check could be eliminated if we changed v/snprintf or
>+ * made "safe" versions of those routines.
>+ */
>+
>+#define	SPACELEFT(buffer, current) (sizeof (buffer) - \
>+				    ((char *)(current) - (char *)(buffer)))

This is wrong if size_t is larger than ptrdiff_t.  Then the result is
unsigned and large if `current' is after the end of the buffer, as it
may be the `fmt' conversion (the overflow check doesn't get done early
enough to help).   Also, the pointer difference is undefined ig
`current' isn't in the buffer.

>+#define OVERFLOW(buffer, current)  ((current) > \
>+				    (char *)(buffer) + sizeof (buffer))
						     ^
Anal code should check for overflow here.  In fact, the check can't be
written like this in Standard C.  `fooptr + offset' is undefined if the
offset isn't a valid array index (or maybe one larger).   The correct
test is something like

	int nwritten;
	ssize_t spaceleft;
	...
	spaceleft = SPACELEFT(...);
	assert(spaceleft >= 0);
	nwritten = vsnprintf(..., spaceleft, ...);
	assert(nwritten >= 0);
	if (nwritten > spaceleft)
		goto overflow;

Bruce