From owner-freebsd-hackers@FreeBSD.ORG Tue Jun 2 09:52:06 2009 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 08D951065670 for ; Tue, 2 Jun 2009 09:52:06 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from 0.mx.codelabs.ru (0.mx.codelabs.ru [144.206.177.45]) by mx1.freebsd.org (Postfix) with ESMTP id 84A1C8FC28 for ; Tue, 2 Jun 2009 09:52:05 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) DomainKey-Signature: a=rsa-sha1; q=dns; c=simple; s=one; d=codelabs.ru; h=Received:Date:From:To:Cc:Subject:Message-ID:Reply-To:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:Sender; b=qYNcWFa5z8Dv6FGilfxA2CPgbJrCNZF/+E2yQIzaZHKWCiE/Ku99bacL24sr6yuejbyt7BB2JBsyhCCo/X9s7NbMklUoby0fo3nEAYj10+JbhI2exXrOlUPXhth8fiaW+nL2lt7xsOoGov8T3ij5+WDX9yNCvLh5z8S2/QArw7I=; Received: from void.codelabs.ru (void.codelabs.ru [144.206.177.25]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1MBQf3-0002wp-Tt; Tue, 02 Jun 2009 13:52:02 +0400 Date: Tue, 2 Jun 2009 13:51:59 +0400 From: Eygene Ryabinkin To: Dag-Erling Sm??rgrav Message-ID: References: <23727599.post@talk.nabble.com> <86prdvipwe.fsf@ds4.des.no> <20090527233110.E4243@delplex.bde.org> <86r5yaijef.fsf@ds4.des.no> <20090529210855.V1643@besplex.bde.org> <86vdnju9z1.fsf@ds4.des.no> <86r5y7u9r3.fsf@ds4.des.no> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <86r5y7u9r3.fsf@ds4.des.no> Sender: rea-fbsd@codelabs.ru Cc: freebsd-hackers@FreeBSD.org, Jakub Lach , Bruce Evans Subject: Re: FYI Lighttpd 1.4.23 /kernel (trailing '/' on regular file symlink) vulnerability X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: rea-fbsd@codelabs.ru List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Jun 2009 09:52:06 -0000 Dag-Erling, good day. Fri, May 29, 2009 at 06:58:08PM +0200, Dag-Erling Sm??rgrav wrote: > Index: sys/kern/vfs_lookup.c > =================================================================== > --- sys/kern/vfs_lookup.c (revision 193028) > +++ sys/kern/vfs_lookup.c (working copy) > @@ -454,7 +454,6 @@ > int docache; /* == 0 do not cache last component */ > int wantparent; /* 1 => wantparent or lockparent flag */ > int rdonly; /* lookup read-only flag bit */ > - int trailing_slash; > int error = 0; > int dpunlocked = 0; /* dp has already been unlocked */ > struct componentname *cnp = &ndp->ni_cnd; > @@ -529,12 +528,10 @@ > * trailing slashes to handle symlinks, existing non-directories > * and non-existing files that won't be directories specially later. > */ > - trailing_slash = 0; > while (*cp == '/' && (cp[1] == '/' || cp[1] == '\0')) { > cp++; > ndp->ni_pathlen--; > if (*cp == '\0') { > - trailing_slash = 1; > *ndp->ni_next = '\0'; /* XXX for direnter() ... */ > cnp->cn_flags |= TRAILINGSLASH; > } > @@ -711,7 +708,7 @@ > error = EROFS; > goto bad; > } > - if (*cp == '\0' && trailing_slash && > + if (*cp == '\0' && (cnp->cn_flags & TRAILINGSLASH) && > !(cnp->cn_flags & WILLBEDIR)) { For the current code state, check "*cp == '\0'" seems to be redundant: VOP_LOOKUP had failed at this point and we're running with EJUSTRETURN. And EJUSTRETURN can happen only if ISLASTCN is set, that in turn implies that *ndp->ni_next is 0 and ndp->ni_next is equal to the cp. Seems like this change makes symlink behavior more consistent: if any previous component had a trailing slash, then we will bail out unless we intend to create/rename directory. By the way, comment before the test "if (rdonly)' seems to be slightly misleading: we could be not only creating the file but could be renaming at this point as well. Perhaps 'creating' should be changed to 'creating/renaming'. > error = ENOENT; > goto bad; > @@ -788,7 +785,7 @@ > * Check for symbolic link > */ > if ((dp->v_type == VLNK) && > - ((cnp->cn_flags & FOLLOW) || trailing_slash || > + ((cnp->cn_flags & FOLLOW) || (cnp->cn_flags & TRAILINGSLASH) || > *ndp->ni_next == '/')) { > cnp->cn_flags |= ISSYMLINK; > if (dp->v_iflag & VI_DOOMED) { Seems like here we can also check for the trailing slash to be set on any previous invocation (or the current one), since we're following symlinks in the case of directories, so we should follow them to the end. > BTW, what does the "XXX for direnter()" comment mean? It means that the trailing slashes are eliminated because direnter() for some FSes may choke on it. It essentially duplicates the chunk of a big comment before the block. 'ngp->ni_next' is set to 'cp' before the block and at that time 'cp' points to a slash that delimits directories or null character that terminates non-slashed name. Perhaps 'XXX for direnter()' should be changed to something like 'strip trailing slashes in cnp->cn_nameptr'. By the way, comment just after 'nextname' label is misleading: we can be there with symbolic link, but it won't be followed. So "Not a symbolic link" can be changed to something like "Not a symbolic link that will be followed". -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ #