Date: Fri, 31 Jul 2015 17:12:45 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Pedro F. Giffuni" <pfg@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r286102 - head/usr.bin/wall Message-ID: <20150731162408.M1843@besplex.bde.org> In-Reply-To: <201507310112.t6V1CWh8034232@repo.freebsd.org> References: <201507310112.t6V1CWh8034232@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 31 Jul 2015, Pedro F. Giffuni wrote:
> Log:
> Buffer overflow in wall(1).
>
> This affected syslogd, wall and talkd.
> Detected by FORTIFY_SOURCE GSoC (with clang).
Old versions got this wrong by using strcpy() instead of strlcpy().
The fix using strlcpy() got this wrong by using a wrong buffer size.
This change gets this wrong by churning to a different and slightly
worse method.
> Modified: head/usr.bin/wall/ttymsg.c
> ==============================================================================
> --- head/usr.bin/wall/ttymsg.c Fri Jul 31 00:31:52 2015 (r286101)
> +++ head/usr.bin/wall/ttymsg.c Fri Jul 31 01:12:31 2015 (r286102)
> @@ -62,7 +62,7 @@ ttymsg(struct iovec *iov, int iovcnt, co
> struct iovec localiov[7];
> ssize_t left, wret;
> int cnt, fd;
> - static char device[MAXNAMLEN] = _PATH_DEV;
> + char device[MAXNAMLEN] = _PATH_DEV;
This removes the rather silly micro-optimization of using a static
buffer with just the few characters in _PATH_DEV in it held constant.
The buffer is now allocated locally and initialized by copying _PATH_DEV
to it on every call. If you want this pessimization, don't keep the
obfuscation of initializing it in its declaration. Use strcpy() or
snprintf() to initialize.
> static char errbuf[1024];
Another static buffer. The function is obviously not reentrant. This
large static buffer mainly wastes space all the time instead of only
when the function is called.
> char *p;
> int forked;
> @@ -71,8 +71,8 @@ ttymsg(struct iovec *iov, int iovcnt, co
> if (iovcnt > (int)(sizeof(localiov) / sizeof(localiov[0])))
> return ("too many iov's (change code in wall/ttymsg.c)");
>
> + strlcat(device, line, sizeof(device));
This depends on the slow reinitialization on every entry. The combined
initialization (initializer + strlcat) is an obfuscated way of
concatenating 2 strings. The clearest way is snprintf with %s%s format.
style(9) actually specifies using snprintf() indirectly. It never
dreamt of snprintf(), but it says to use printf() and not build up
output and bugs using fputs(), puts(), putchar(), whatever.
> p = device + sizeof(_PATH_DEV) - 1;
> - strlcpy(p, line, sizeof(device));
The correct fix was just to use the right buffer size here. The pointer
was adjusted to skip _PATH_BUF at the beginning of the buffer, but the
buffer size was not adjusted to match.
Similar complications occur when strings are built up using snprintf().
It is not as easy to use as printf(), so style(9)'s indirect requirement
might not apply. You have to keep track of the remaining part of the
buffer at every step, while printf() keeps track of the file pointer
automatically.
The error checking is still null. The code is still broken if buffer
overflow actually occurred, since then open() was attempted on a
garbage device name. But most device names are short, and none can
be decided by untrusted users.
printf() is again easier to use. It has a sticky error state, so that
if an error occurs it can at least be detected after several steps.
I once saw some code that actually checked for printf() errors.
> if (strncmp(p, "pts/", 4) == 0)
> p += 4;
> if (strchr(p, '/') != NULL) {
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150731162408.M1843>