Re: svn commit: r286102 - head/usr.bin/wall
On Fri, 31 Jul 2015, Pedro Giffuni wrote: On 07/31/15 02:12, Bruce Evans wrote: On Fri, 31 Jul 2015, Pedro F. Giffuni wrote: ... 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. But if I drop that static I get a bunch of errors: Yes. I forgot that it is is returned. Older not so good APIs often return a pointer to static data. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286102 - head/usr.bin/wall
On 07/31/15 02:12, Bruce Evans wrote: On Fri, 31 Jul 2015, Pedro F. Giffuni wrote: ... 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. But if I drop that static I get a bunch of errors: /usr/src/usr.bin/wall/ttymsg.c:82:11: error: address of stack memory associated with local variable 'errbuf' returned [-Werror,-Wreturn-stack-address] return (errbuf); ^~ /usr/src/usr.bin/wall/ttymsg.c:94:11: error: address of stack memory associated with local variable 'errbuf' returned [-Werror,-Wreturn-stack-address] return (errbuf); ^~ /usr/src/usr.bin/wall/ttymsg.c:134:13: error: address of stack memory associated with local variable 'errbuf' returned [-Werror,-Wreturn-stack-address] return (errbuf); ^~ /usr/src/usr.bin/wall/ttymsg.c:160:11: error: address of stack memory associated with local variable 'errbuf' returned [-Werror,-Wreturn-stack-address] return (errbuf); ^~ ... 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. I will do that, thanks. Pedro. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286102 - head/usr.bin/wall
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 ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
svn commit: r286102 - head/usr.bin/wall
Author: pfg Date: Fri Jul 31 01:12:31 2015 New Revision: 286102 URL: https://svnweb.freebsd.org/changeset/base/286102 Log: Buffer overflow in wall(1). This affected syslogd, wall and talkd. Detected by FORTIFY_SOURCE GSoC (with clang). Submitted by: Oliver Pinter Differential Revision:https://reviews.freebsd.org/D3254 Reviewed by: delphij, jmg MFC after:3 days Modified: head/usr.bin/wall/ttymsg.c 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; static char errbuf[1024]; 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)); p = device + sizeof(_PATH_DEV) - 1; - strlcpy(p, line, sizeof(device)); if (strncmp(p, pts/, 4) == 0) p += 4; if (strchr(p, '/') != NULL) { ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org