From owner-freebsd-security  Wed Sep  6 21:04:05 1995
Return-Path: security-owner
Received: (from majordom@localhost)
          by freefall.freebsd.org (8.6.11/8.6.6) id VAA07300
          for security-outgoing; Wed, 6 Sep 1995 21:04:05 -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 VAA07294
          for <security@freebsd.org>; Wed, 6 Sep 1995 21:04:03 -0700
Received: from localhost (localhost [127.0.0.1]) by precipice.shockwave.com (8.6.12/8.6.12) with SMTP id VAA00782; Wed, 6 Sep 1995 21:03:00 -0700
Message-Id: <199509070403.VAA00782@precipice.shockwave.com>
To: Bruce Evans <bde@zeta.org.au>
cc: security@freebsd.org
Subject: Re: diffs for syslog.c 
In-reply-to: Your message of "Thu, 07 Sep 1995 11:13:46 +1000."
             <199509070113.LAA10056@godzilla.zeta.org.au> 
Date: Wed, 06 Sep 1995 21:02:59 -0700
From: Paul Traina <pst@shockwave.com>
Sender: security-owner@freebsd.org
Precedence: bulk


  From: Bruce Evans <bde@zeta.org.au>
  Subject: Re: diffs for syslog.c
  >+/*
  >+ * 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.

Thanks for the code review,  I think we have some disagreements.  I don't
have an ANSI spec in front of me,  so please bear with me if I'm being a
fool.

It is my impression that the standard would have the pointer expression
evaluated first.  I believe the result of the pointer arithmetic here is
an integer offset, not a pointer.

This signed integer value would be promoted to type size_t for an arithmetic
expression.

  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 if `current' isn't in the buffer.

See below (1)
  
  >+#define OVERFLOW(buffer, current)  ((current) > \
  >+				    (char *)(buffer) + sizeof (buffer))
  						     ^
  Anal code should check for overflow here.

OK, yes, if buffer really was at the top of memory space, you're absolutely
correct.  How would you propose to code that overflow check?

  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).

I disagree.  Performing a dereference of the value is undefined, but the
pointer arithmetic is still valid and defined.

  The correct test is something like
  
  	int nwritten;
  	ssize_t spaceleft;
  	...
  	spaceleft = SPACELEFT(...);
  	assert(spaceleft >= 0);
  	nwritten = vsnprintf(..., spaceleft, ...);
  	assert(nwritten >= 0);

When would you see nwritten return less than 0?  It's a signed integer,
and the 'error' case is 0.

  	if (nwritten > spaceleft)
  		goto overflow;
  
  Bruce