Date: Thu, 15 Sep 2005 13:27:03 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Trevor Blackwell <tlb@tlb.org> Cc: freebsd-bugs@FreeBSD.org, FreeBSD-gnats-submit@FreeBSD.org Subject: Re: bin/86135: Fwd: Latent buffer overflow in getcwd Message-ID: <20050915120351.Q43928@delplex.bde.org> In-Reply-To: <1126728802.42486.3239.camel@lab> References: <1126728802.42486.3239.camel@lab>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050915120351.Q43928>