Fix buffer overflow in syslogd, wall and talkd.

[op]

Summary

This error was found with FORTIFY_SOURCE, and it's a real problem. I'm able to crash the syslogd with enabled FORTIFY_SOURCE.
The problem is the broken pointer usage in strlcpy, which renders the strlcpy's check unusable.

Obtained from HardenedBSD.

[jmg]

I'm fine w/ this change.

usr.bin/wall/ttymsg.c

65 ↗

(On Diff #7531)

there doesn't seem to be a good reason to keep device static.

[delphij]

Change looks fine to me.

I agree with jmg@ that there is no reason that device should be static because the pointer is strictly internal to the function unlike errbuf would be returned, but I think that can go in a different changeset.

[op]

• remove static from device variable

[pfg]

Thank you ... will commit soon and MFC after 3 days.

Question: should I try to fold this into the release?

[op]

In D3254#65727, @pfg wrote:

Thank you ... will commit soon and MFC after 3 days.

Question: should I try to fold this into the release?

It would be nice, but not must have. If there isn't another exploitable bug, whereby able to write to the utmpx related db files (these requires root access or setuid), the attacker could not exploit this bug. The only way to control the line (ut->ut_line) is through the utmpx files.

[pfg]

In D3254#65759, @op wrote:

In D3254#65727, @pfg wrote:

...
Question: should I try to fold this into the release?

It would be nice, but not must have. If there isn't another exploitable bug, whereby able to write to the utmpx related db files (these requires root access or setuid), the attacker could not exploit this bug. The only way to control the line (ut->ut_line) is through the utmpx files.

OK, honestly it is rather tight schedule to push fixes into Release, and that's why I prefer not to go that way..

[pfg]

And, of course, Bruce did some post-commit review, to let us know this wasn't the best possible fix.

head/usr.bin/wall/ttymsg.c

65

From bde@:

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.

66

From bde@:

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.

74

From bde@:

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.

75

From bde:

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.

[jmg]

head/usr.bin/wall/ttymsg.c

65

either of on this one.. we should keep the micro op of not having it static IMO

66

but we don't know what errbuf is used for, a larger change is to make errbuf allocated, and require the caller to free it, but that is likely more invasive, but better change.

74

I managed to miss this on my first read through, and this is a good option..

[op]

head/usr.bin/wall/ttymsg.c

65

bde@:

Is this optimization really necessary in 2015 in a not performance sensitive code-path?

/*
 * Display the contents of a uio structure on a terminal.  Used by wall(1),
 * syslogd(8), and talkd(8).  Forks and finishes in child if write would block,
 * waiting up to tmout seconds.  Returns pointer to error string on unexpected
 * error; string is not newline-terminated.  Various "normal" errors are
 * ignored (exclusive-use, lack of permission, etc.).
 */
const char *
ttymsg(struct iovec *iov, int iovcnt, const char *line, int tmout)

This function are always called with 1 or 10 seconds! of timeout:

root@opbsd-fortify:/usr/src/usr.sbin/syslogd # grep -A1 ttymsg *
Makefile:SRCS=  syslogd.c ttymsg.c
Makefile-
Binary file syslogd matches
--
syslogd.c:#define       TTYMSGTIME      1               /* timeout passed to ttymsg */
syslogd.c-#define       RCVBUF_MINSIZE  (80 * 1024)     /* minimum size of dgram rcv buffer */
--
syslogd.c:#include "ttymsg.h"
syslogd.c-
--
syslogd.c:              errno = 0;      /* ttymsg() only sometimes returns an errno */
syslogd.c:              if ((msgret = ttymsg(iov, IOV_SIZE, f->f_un.f_fname, 10))) {
syslogd.c-                      f->f_type = F_UNUSED;
--
syslogd.c:                      if ((p = ttymsg(iov, iovlen, ut->ut_line,
syslogd.c-                          TTYMSGTIME)) != NULL) {
--
syslogd.c:                              if ((p = ttymsg(iov, iovlen, ut->ut_line,
syslogd.c-                                  TTYMSGTIME)) != NULL) {

[pfg]

head/usr.bin/wall/ttymsg.c

65

bde sadly doesn't cuse code review software.

bde called it "silly optimization".
I agree with jmg@ and op@ that we don't need it.

66

Unstatic'ing this breaks the build.