From owner-freebsd-hackers@FreeBSD.ORG Wed May 27 12:07:13 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 3815C106568C for ; Wed, 27 May 2009 12:07:13 +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 C34978FC2B for ; Wed, 27 May 2009 12:07:12 +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=DGPnnqWodrlW1brGyows1ybtD9ulS1wgOmoLkUskWeg1zK2/8zmJhaQTcPEp7Ck9Y0zKoupMNnhJbvxwMUervJ/bWnCecaZ/sPO6NIygDDO1KKrEjC315vydUGfCc6+xs0/OTU9skBnA0BgfBwEriSPCX+uQc8b1+pHP/DRnlTI=; Received: from void.codelabs.ru (void.codelabs.ru [144.206.177.25]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1M9HuZ-000Ceh-MY; Wed, 27 May 2009 16:07:11 +0400 Date: Wed, 27 May 2009 16:07:09 +0400 From: Eygene Ryabinkin To: Dag-Erling Sm??rgrav Message-ID: <15QQC+1YeDzOjf35dqyJmioc1ik@XX1fo6zQUfC4h0jjRC6IBz3oNH4> References: <23727599.post@talk.nabble.com> <86prdvipwe.fsf@ds4.des.no> <0vGjPHEq7MqxjtFmBufY+mBxlR4@7oUjtCwN654QcDr16CH+kAk8bJg> <86vdnmiz30.fsf@ds4.des.no> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="Nq2Wo0NMKNjxTN9z" Content-Disposition: inline In-Reply-To: <86vdnmiz30.fsf@ds4.des.no> Sender: rea-fbsd@codelabs.ru Cc: freebsd-hackers@freebsd.org, Jakub Lach 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: Wed, 27 May 2009 12:07:14 -0000 --Nq2Wo0NMKNjxTN9z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Wed, May 27, 2009 at 01:07:15PM +0200, Dag-Erling Sm??rgrav wrote: > Eygene Ryabinkin writes: > > May be the attached patch will fix the thing? > > I'm not entirely convinced. Try the regression test I wrote > (head/tools/regression/vfs/trailing_slash.t) I see: you mean that the bare '/' at the end of everything but directory should produce ENOTDIR. OK, patch was modified and now it passes all your checks. > > It adds an additional flag, but this was the only thing I was able to > > invent to avoid ABI breakage. > > The flag is a good idea, but I think the correct place to handle this is > in namei(), around line 290 The problem with the check in namei() itself is the cleanup of all locks that were held in the lookup(). If lookup() is finished without error, then the burden of cleanup is ours (namei's). I could duplicate the stuff, but why? lookup() already does it and it's better to keep the things in one place. The logics is laid as follows: if lookup() processes the last component and it had seen the trailing slash, the flag is set. When we have no more targets to get from the current path inside lookup(), check if slashed flag is set and reject anything that is non-directory. Such strategy should also handle the cases of dereferencing (FOLLOWs) of all symbolic links and when some link has slash at the end of the target name: 'ln -s /etc/motd somefile; ln -s somefile/ anotherfile; cat anotherfile' will fail on the last command. If one agrees on such behaviour, such test could be also added to the regression suite. > (don't be fooled by the comment on line 270; > the code inside the if statement is for the *non*-symlink case). Me sees this on the line 226, but may be I hadn't updated my 7.x. And yes, I know what was meant by '(cnp->cn_flags & ISSYMLINK) == 0' ;)) -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ # --Nq2Wo0NMKNjxTN9z Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="vfs_lookup-trailing-symlink-with-slash.diff" Content-Transfer-Encoding: quoted-printable =46rom 6109a710c794c4a68073d4299639cd858f762d24 Mon Sep 17 00:00:00 2001 =46rom: Eygene Ryabinkin Date: Wed, 27 May 2009 13:13:16 +0400 Subject: [PATCH] vfs lookups: properly handle the case of slash at the end = of symlink If symlink points to a non-directory object but the name has trailing slash, then the current lookup/namei implementation will dereference symlink and return dereferenced object instead of symlink even if NOFOLLOW mode is used. That's not good at all :(( Simple test: ----- $ ln -s /etc/motd file $ file file file: symbolic link to `/etc/motd' [ =3D=3D Unpatched variant =3D=3D ] $ file file/ file/: ASCII English text [ =3D=3D Patched variant =3D=3D ] $ file file/ file/: cannot open `file/' (Not a directory) ----- See also: http://www.freebsd.org/cgi/query-pr.cgi?pr=3Dkern/21768 See also: http://lists.freebsd.org/pipermail/freebsd-security/2009-May/0052= 19.html Signed-off-by: Eygene Ryabinkin --- sys/kern/vfs_lookup.c | 24 ++++++++++++++++-------- sys/sys/namei.h | 41 +++++++++++++++++++++-------------------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c index 3770b55..dc801fd 100644 --- a/sys/kern/vfs_lookup.c +++ b/sys/kern/vfs_lookup.c @@ -138,6 +138,9 @@ namei(struct nameidata *ndp) cnp->cn_flags &=3D ~LOCKSHARED; fdp =3D p->p_fd; =20 + /* Drop internal flag: we will set it ourselves if we'll need it. */ + cnp->cn_flags &=3D ~SLASHTARGET; + /* * Get a buffer for the name to be translated, and copy the * name into the buffer. @@ -683,6 +686,11 @@ unionlookup: ndp->ni_vp =3D dp =3D tdp; } =20 + /* Set "slashed" flag if we found slash at the end of the name */ + if (trailing_slash && (cnp->cn_flags & ISLASTCN)) { + cnp->cn_flags |=3D SLASHTARGET; + } + /* * Check for symbolic link */ @@ -710,14 +718,6 @@ unionlookup: goto success; } =20 - /* - * Check for bogus trailing slashes. - */ - if (trailing_slash && dp->v_type !=3D VDIR) { - error =3D ENOTDIR; - goto bad2; - } - nextname: /* * Not a symbolic link. If more pathname, @@ -741,6 +741,14 @@ nextname: goto dirloop; } /* + * Check if we're processing slashed name + * and lookup target isn't a directory. + */ + if ((cnp->cn_flags & SLASHTARGET) && dp->v_type !=3D VDIR) { + error =3D ENOTDIR; + goto bad2; + } + /* * Disallow directory write attempts on read-only filesystems. */ if (rdonly && diff --git a/sys/sys/namei.h b/sys/sys/namei.h index ac3550d..42e9601 100644 --- a/sys/sys/namei.h +++ b/sys/sys/namei.h @@ -127,26 +127,27 @@ struct nameidata { * name being sought. The caller is responsible for releasing the * buffer and for vrele'ing ni_startdir. */ -#define RDONLY 0x0000200 /* lookup with read-only semantics */ -#define HASBUF 0x0000400 /* has allocated pathname buffer */ -#define SAVENAME 0x0000800 /* save pathname buffer */ -#define SAVESTART 0x0001000 /* save starting directory */ -#define ISDOTDOT 0x0002000 /* current component name is .. */ -#define MAKEENTRY 0x0004000 /* entry is to be added to name cache */ -#define ISLASTCN 0x0008000 /* this is last component of pathname */ -#define ISSYMLINK 0x0010000 /* symlink needs interpretation */ -#define ISWHITEOUT 0x0020000 /* found whiteout */ -#define DOWHITEOUT 0x0040000 /* do whiteouts */ -#define WILLBEDIR 0x0080000 /* new files will be dirs; allow trailing / */ -#define ISUNICODE 0x0100000 /* current component name is unicode*/ -#define ISOPEN 0x0200000 /* caller is opening; return a real vnode. */ -#define NOCROSSMOUNT 0x0400000 /* do not cross mount points */ -#define NOMACCHECK 0x0800000 /* do not perform MAC checks */ -#define MPSAFE 0x1000000 /* namei() must acquire Giant if needed. */ -#define GIANTHELD 0x2000000 /* namei() is holding giant. */ -#define AUDITVNODE1 0x4000000 /* audit the looked up vnode information */ -#define AUDITVNODE2 0x8000000 /* audit the looked up vnode information */ -#define PARAMASK 0xffffe00 /* mask of parameter descriptors */ +#define RDONLY 0x00000200 /* lookup with read-only semantics */ +#define HASBUF 0x00000400 /* has allocated pathname buffer */ +#define SAVENAME 0x00000800 /* save pathname buffer */ +#define SAVESTART 0x00001000 /* save starting directory */ +#define ISDOTDOT 0x00002000 /* current component name is .. */ +#define MAKEENTRY 0x00004000 /* entry is to be added to name cache */ +#define ISLASTCN 0x00008000 /* this is last component of pathname */ +#define ISSYMLINK 0x00010000 /* symlink needs interpretation */ +#define ISWHITEOUT 0x00020000 /* found whiteout */ +#define DOWHITEOUT 0x00040000 /* do whiteouts */ +#define WILLBEDIR 0x00080000 /* new files will be dirs; allow trailing / */ +#define ISUNICODE 0x00100000 /* current component name is unicode*/ +#define ISOPEN 0x00200000 /* caller is opening; return a real vnode. */ +#define NOCROSSMOUNT 0x00400000 /* do not cross mount points */ +#define NOMACCHECK 0x00800000 /* do not perform MAC checks */ +#define MPSAFE 0x01000000 /* namei() must acquire Giant if needed. */ +#define GIANTHELD 0x02000000 /* namei() is holding giant. */ +#define AUDITVNODE1 0x04000000 /* audit the looked up vnode information */ +#define AUDITVNODE2 0x08000000 /* audit the looked up vnode information */ +#define SLASHTARGET 0x10000000 /* last component of the name was slashed */ +#define PARAMASK 0x1ffffe00 /* mask of parameter descriptors */ =20 #define NDHASGIANT(NDP) (((NDP)->ni_cnd.cn_flags & GIANTHELD) !=3D 0) =20 --=20 1.6.3.1 --Nq2Wo0NMKNjxTN9z--