From owner-freebsd-bugs@FreeBSD.ORG Thu Sep 15 03:27:09 2005 Return-Path: X-Original-To: freebsd-bugs@FreeBSD.org Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id D6A8116A41F; Thu, 15 Sep 2005 03:27:09 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1.pacific.net.au [61.8.0.84]) by mx1.FreeBSD.org (Postfix) with ESMTP id 47F3343D45; Thu, 15 Sep 2005 03:27:09 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.0.87]) by mailout1.pacific.net.au (8.13.4/8.13.4/Debian-3) with ESMTP id j8F3R5YF011403; Thu, 15 Sep 2005 13:27:05 +1000 Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy2.pacific.net.au (8.13.4/8.13.4/Debian-3) with ESMTP id j8F3R3mg030499; Thu, 15 Sep 2005 13:27:04 +1000 Date: Thu, 15 Sep 2005 13:27:03 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Trevor Blackwell In-Reply-To: <1126728802.42486.3239.camel@lab> Message-ID: <20050915120351.Q43928@delplex.bde.org> References: <1126728802.42486.3239.camel@lab> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-bugs@FreeBSD.org, FreeBSD-gnats-submit@FreeBSD.org Subject: Re: bin/86135: Fwd: Latent buffer overflow in getcwd X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Sep 2005 03:27:10 -0000 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