From owner-freebsd-hackers@FreeBSD.ORG Tue Jun 2 20:01: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 A5F071065707 for ; Tue, 2 Jun 2009 20:01: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 561918FC21 for ; Tue, 2 Jun 2009 20:01:06 +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=WJzzQ2jCadvhUGLisp+ZjzUnG7rd0EPzvIC6SHnQPzs/JtX37AY6uil0Fo15OVLYJ104+XI8yVJ+Pv8rtRZEqaqkDU+0XfPK8V7R96Zhcm0sj9DuBfdzCpWnvQi6nrpR+P3KK4M74b8utjhaTAx4M56BDGQ8BZRlTRQNaIyzk3A=; Received: from phoenix.codelabs.ru (ppp91-78-250-129.pppoe.mtu-net.ru [91.78.250.129]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1MBaAQ-000E4W-SX; Wed, 03 Jun 2009 00:01:03 +0400 Date: Wed, 3 Jun 2009 00:01:00 +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> <86skiiri1p.fsf@ds4.des.no> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <86skiiri1p.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 20:01:07 -0000 Tue, Jun 02, 2009 at 01:28:34PM +0200, Dag-Erling Sm??rgrav wrote: > Eygene Ryabinkin writes: > > For the current code state, check "*cp == '\0'" seems to be redundant: > > [...] By the way, comment before the test "if (rdonly)' seems to be > > slightly misleading [...] > > OK, I see that. I've removed the check and rewritten the comment. Thanks. > What about this "temporary assert"? > > /* > * This is a temporary assert to make sure I know what the > * behavior here was. > */ > KASSERT((cnp->cn_flags & (WANTPARENT|LOCKPARENT)) != 0, > ("lookup: Unhandled case.")); It is partly handled at the beginning of lookup: ----- wantparent = cnp->cn_flags & (LOCKPARENT | WANTPARENT); KASSERT(cnp->cn_nameiop == LOOKUP || wantparent, ("CREATE, DELETE, RENAME require LOCKPARENT or WANTPARENT.")); ----- So, only LOOKUP could have both {LOCK,WANT}PARENT to be unset. And it seems to be fine, because LOOKUP shouldn't care about parent vnode, either locked or just referenced. So this assertion seems to be really redundant. > > 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. > > I'm not sure I understand... I meant the following: you're trading 'trailing_slash' to the TRAILINGSLASH. The former is local to this call of lookup(), the latter persists throughout the namei() invocation, so it could be set by the previous lookup() calls. But this seems to be fine: since we were started to look for directory at some point, we should continue to do it. > > Perhaps 'XXX for direnter()' should be changed to something like > > 'strip trailing slashes in cnp->cn_nameptr'. > > I'll just remove it, since the previous comment clearly explains what is > going on. May be it's better to leave the comment, but replace it with more undestandable one: this instruction is a bit tricky and it makes one to think what the hell is going on. > > 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". > > /* > * Not a symbolic link that we will follow. Continue with the > * next component if there is any; otherwise, we're done. > */ Yes, very good. Thanks. -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ #