From owner-freebsd-hackers  Mon Jan 13 19:53:24 1997
Return-Path: <owner-hackers>
Received: (from root@localhost)
          by freefall.freebsd.org (8.8.4/8.8.4) id TAA00773
          for hackers-outgoing; Mon, 13 Jan 1997 19:53:24 -0800 (PST)
Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.19])
          by freefall.freebsd.org (8.8.4/8.8.4) with ESMTP id TAA00768
          for <hackers@FreeBSD.org>; Mon, 13 Jan 1997 19:53:21 -0800 (PST)
Received: (from bde@localhost) by godzilla.zeta.org.au (8.8.3/8.6.9) id OAA20727; Tue, 14 Jan 1997 14:47:57 +1100
Date: Tue, 14 Jan 1997 14:47:57 +1100
From: Bruce Evans <bde@zeta.org.au>
Message-Id: <199701140347.OAA20727@godzilla.zeta.org.au>
To: bde@zeta.org.au, davidn@unique.usn.blaze.net.au
Subject: Re: unused variable in su
Cc: hackers@FreeBSD.org, joerg_wunsch@uriah.heep.sax.de
Sender: owner-hackers@FreeBSD.org
X-Loop: FreeBSD.org
Precedence: bulk

>> It should be strdup().  Using strncpy() or snprintf() to handle buffer
>> overflows by truncating the string is sloppy.
>
>No, it is defensive programming, pure and simple.
>
>The buffer already IS a reasonable size - in fact, it is the
>maximum legal path size. If the string copied into it is too

Actually, it's the size of the longest path that is guaranteed to
be acceptable to system calls that look up pathnames.  Longer
paths may work with non-system calls.  E.g., getpwd() may return
a path longer than MAXPATHLEN.

>long, then it is going to get rejected by the execvp() anyway
>- only the error number changes - EEXIST vs. ENAMETOOLONG.

No, after you've truncated it to length MAXPATHLEN - 1, execv()
may exec the wrong program. (execvp() would be a security hole in
su,  execvp() is careful about this problem.  Try this:

	PATH=/111...:$PATH

where there are about MAXPATHLEN 1's in the first component.
execvp() will print a lot of annoying error messages.)

>There is an advantage the snprintf() case, at least you can
>detect if the formatted string would have overflowed. Although
>it seems that the snprintf() return value is not often (enough)
>checked. IMHO, it should be, perhaps even via syslog(), if
>early notification of possible attacks is useful.

How about using functions strancpy(), ..., asnprintf() that abort
if the string is too long?  This would be better than silently
truncating the string.  It would also take less code for the
strncpy() case since null trermination would be guaranteed (else
abort).

Bruce