Date: Wed, 06 Sep 1995 21:02:59 -0700 From: Paul Traina <pst@shockwave.com> To: Bruce Evans <bde@zeta.org.au> Cc: security@freebsd.org Subject: Re: diffs for syslog.c Message-ID: <199509070403.VAA00782@precipice.shockwave.com> In-Reply-To: Your message of "Thu, 07 Sep 1995 11:13:46 %2B1000." <199509070113.LAA10056@godzilla.zeta.org.au>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199509070403.VAA00782>