From owner-freebsd-bugs@FreeBSD.ORG Thu Sep 15 03:30:24 2005 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id DE9A616A41F for ; Thu, 15 Sep 2005 03:30:23 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 4C70643D68 for ; Thu, 15 Sep 2005 03:30:20 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.3/8.13.3) with ESMTP id j8F3UJX9033287 for ; Thu, 15 Sep 2005 03:30:19 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.3/8.13.1/Submit) id j8F3UJTr033286; Thu, 15 Sep 2005 03:30:19 GMT (envelope-from gnats) Date: Thu, 15 Sep 2005 03:30:19 GMT Message-Id: <200509150330.j8F3UJTr033286@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Bruce Evans Cc: Subject: Re: bin/86135: Fwd: Latent buffer overflow in getcwd X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Bruce Evans List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Sep 2005 03:30:24 -0000 The following reply was made to PR kern/86135; it has been noted by GNATS. From: Bruce Evans To: Trevor Blackwell Cc: FreeBSD-gnats-submit@FreeBSD.org, freebsd-bugs@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