bin/86135: Fwd: Latent buffer overflow in getcwd

[Bruce Evans]

The following reply was made to PR kern/86135; it has been noted by GNATS.

From: Bruce Evans <bde at zeta.org.au>
To: Trevor Blackwell <tlb at tlb.org>
Cc: FreeBSD-gnats-submit at FreeBSD.org, freebsd-bugs at FreeBSD.org
Subject: Re: bin/86135: Fwd: Latent buffer overflow in getcwd
Date: Thu, 15 Sep 2005 13:27:03 +1000 (EST)

 On Wed, 14 Sep 2005, Trevor Blackwell wrote:
 
 >> Description:
 >
 > The libc getcwd has a latent bug, where it allocates a buffer of 1020 bytes and assumes
 > it to have MAXPATHLEN (=1024) bytes. Normal modern mallocs will allocate 1024 bytes
 > anyway, but a different malloc could cause an overrun, and changing MAXPATHLEN could cause
 > trouble, and it'll cause trouble with debugging mallocs.
 >
 > Allocating 1024-4 was an optimization assuming the existence of a malloc header, which
 > isn't the case nowadays. The most important think is that eup = up + upsize, but the most
 > robust plan is to allocate MAXPATHLEN bytes in case that changes.
 >
 >> How-To-Repeat:
 >       It wouldn't be easy to cause an actual corruption
 >
 >> Fix:
 >
 > Index: getcwd.c
 > ===================================================================
 > RCS file: /home/ncvs/src/lib/libc/gen/getcwd.c,v
 > retrieving revision 1.25
 > diff -c -r1.25 getcwd.c
 > *** getcwd.c    29 Oct 2003 10:45:01 -0000      1.25
 > --- getcwd.c    14 Sep 2005 18:25:48 -0000
 > ***************
 > *** 115,123 ****
 >         * Should always be enough (it's 340 levels).  If it's not, allocate
 >         * as necessary.  Special case the first stat, it's ".", not "..".
 >         */
 > !       if ((up = malloc(upsize = 1024 - 4)) == NULL)
 >                goto err;
 > !       eup = up + MAXPATHLEN;
 >        bup = up;
 >        up[0] = '.';
 >        up[1] = '\0';
 > --- 115,123 ----
 >         * Should always be enough (it's 340 levels).  If it's not, allocate
 >         * as necessary.  Special case the first stat, it's ".", not "..".
 >         */
 > !       if ((up = malloc(upsize = MAXPATHLEN)) == NULL)
 >                goto err;
 > !       eup = up + upsize;
 >        bup = up;
 >        up[0] = '.';
 >        up[1] = '\0';
 >
 
 I prefer to change only " - 4" to "" and MAXPATHLEN to upsize (" - 4" also
 needs to be removed in the comment).  This would match similar code
 involving ept and ptsize and keep the comment in sync with the code.
 MAXPATHLEN is not very relevant here -- the size needed is just the size of
 our buffer, and MAXPATHLEN bytes is neither usually necessary nor always
 sufficient, especially for "up", since as the preceding comment says, the
 buffer for "up is [just] for holding concatenations of "../".  (This comment
 is not quite correct.  The final path component of "up" can be any directory
 entry when a mount point is crossed.)  phk might say that the whole
 memory allocation stategy is wrong.  It might be better to allocate a huge
 buffer for "up" and never reallocate it.
 
 The committed version changes (1024 - 4) to MAXPATHLEN globally.  This
 creates lots of style bugs:
 - MAXPATHLEN is a misspelling of {PATH_MAX}.
 - MAXPATHLEN is a misspelling of 1024 IMO (see above).
 - The magic 340 in the above was (1024 - 4) / strlen("../").  Now its
    magic is deeper.  340 was wrong even when the initial upsize was known
    to be (1024 - 4) since it didn't allow for the NUL terminator or mount
    points.  The exact is something like
    1 + (initial_upsize - {NAME_MAX} - 1) / strlen("../").
 
 Nearby style bug: we use the doubling strategy for expanding "pt".  This
 is especially silly if the caller passed a silly initial size for it.
 
 Bruce