On 08/01/15 09:34, Eric van Gyzen wrote:
On 7/31/15 8:29 PM, Pedro F. Giffuni wrote:
Author: pfg
Date: Sat Aug 1 01:29:55 2015
New Revision: 286144
URL: https://svnweb.freebsd.org/changeset/base/286144
Log:
Buffer overflow in wall(1).
Revert r286102 and apply a cleaner fix.
Tested for overflows by FORTIFY_SOURCE GSoC (with clang).
Suggested by: bde
Reviewed by: Oliver Pinter
Tested by: Oliver Pinter
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 23:40:18 2015 (r286143)
+++ head/usr.bin/wall/ttymsg.c Sat Aug 1 01:29:55 2015 (r286144)
@@ -62,7 +62,7 @@ ttymsg(struct iovec *iov, int iovcnt, co
struct iovec localiov[7];
ssize_t left, wret;
int cnt, fd;
- char device[MAXNAMLEN] = _PATH_DEV;
+ char device[MAXNAMLEN];
static char errbuf[1024];
char *p;
int forked;
@@ -71,8 +71,9 @@ 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));
+ strlcpy(device, _PATH_DEV, sizeof(device));
p = device + sizeof(_PATH_DEV) - 1;
+ strlcpy(p, line, sizeof(device) - sizeof(_PATH_DEV));
You're probably already sick of this change, but I would encourage this
instead:
strlcat(device, line, sizeof(device));
It looks exactly like the code I just replaced ;).
Your current code works, and it's even more efficient than strlcat.
Indeed, efficient is good and was the reason for the changes suggested
by bde. We evaluated snprintf but costs even more.
However, doing arithmetic on either the first or third argument to
strlcpy/strlcat is precisely what caused the overflow that you're
fixing. In fact, the size passed by the current code is one byte too
small. This is obviously not a real concern in this code, but it
demonstrates how easy it is to get these calculations wrong.
Yeah, it's a benign off-by-one :(. I will fix it by using the
return value of the original strlcpy so we it will be less confusing
at the cost of adding a new variable.
This said, the original bug is the first detected by FORTIFY_SOURCE that
is not detected by Coverity, so it will be very exciting to bring
FORTIFY_SOURCE to the tree.
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"
