Date: Thu, 7 Sep 1995 11:13:46 +1000 From: Bruce Evans <bde@zeta.org.au> To: pst@shockwave.com, security@freebsd.org Subject: Re: diffs for syslog.c Message-ID: <199509070113.LAA10056@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>+/*
>+ * 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199509070113.LAA10056>