From owner-freebsd-hackers@FreeBSD.ORG Wed May 27 13:16:29 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 D4B06106566B for ; Wed, 27 May 2009 13:16:29 +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 571058FC2B for ; Wed, 27 May 2009 13:16:27 +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=a2DtkdXwfjqEYhcgwh7u/Hk4C6iZUDsa0Hz1gGWq9W9qnxphx3LcPlB/ItEcwa5yWR6KL7OcaksFiR2cFs+DPbvRCPxG6asjB+1l/RqIeAqRe8Lv6dyfatG/VWsk+ZZpD0AKQqhLjkbwAMUwz5l4KDlyMwj7cNzq0uHjljuZXPY=; Received: from daemon.grid.kiae.ru (daemon.grid.kiae.ru [144.206.66.47]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1M9Iza-000Jj8-O9; Wed, 27 May 2009 17:16:26 +0400 Date: Wed, 27 May 2009 17:16:25 +0400 From: Eygene Ryabinkin To: Dag-Erling Sm??rgrav Message-ID: References: <23727599.post@talk.nabble.com> <86prdvipwe.fsf@ds4.des.no> <0vGjPHEq7MqxjtFmBufY+mBxlR4@7oUjtCwN654QcDr16CH+kAk8bJg> <86vdnmiz30.fsf@ds4.des.no> <15QQC+1YeDzOjf35dqyJmioc1ik@XX1fo6zQUfC4h0jjRC6IBz3oNH4> <86prdug1p0.fsf@ds4.des.no> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nFreZHaLTZJo0R7j" Content-Disposition: inline In-Reply-To: <86prdug1p0.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 13:16:30 -0000 --nFreZHaLTZJo0R7j Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Wed, May 27, 2009 at 02:39:07PM +0200, Dag-Erling Sm??rgrav wrote: > I was working on head. The code is (mostly) the same, just shifted > somewhere between ~50 and ~90 lines depending on where you look. Your > patch should apply cleanly. > > BTW, you made a lot of whitespace changes in namei.h. This is generally > frowned upon, as it makes the functional change almost impossible to > spot in the diff. Yes, spit the patch into two pieces. Thanks for the reminder! > > And yes, I know what was meant by '(cnp->cn_flags & ISSYMLINK) == 0' > > ;)) > > I know you know :) I was just pointing out that the comment is > misleading. Changed it too. All three pieces are attached. Regarding the 'ln -s /etc/motd file; ln -s file/ anotherone': do you (or anyone reading this) think that 'cat anotherone' should really show the contents of /etc/motd or patch's behaviour is good? -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ # --nFreZHaLTZJo0R7j 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 03483c8e800680a8b8a3d3f0d1debdf7fd883906 Mon Sep 17 00:00:00 2001 =46rom: Eygene Ryabinkin Date: Wed, 27 May 2009 13:13:16 +0400 Subject: [PATCH 1/3] 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 | 3 ++- 2 files changed, 18 insertions(+), 9 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..70e902c 100644 --- a/sys/sys/namei.h +++ b/sys/sys/namei.h @@ -146,7 +146,8 @@ struct nameidata { #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 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 --nFreZHaLTZJo0R7j Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="vfs_lookup-trailing-symlink-with-slash-fix-whitespace.diff" Content-Transfer-Encoding: quoted-printable =46rom 2539d4f31a2f85504672e8113343242782e737a7 Mon Sep 17 00:00:00 2001 =46rom: Eygene Ryabinkin Date: Wed, 27 May 2009 17:06:39 +0400 Subject: [PATCH 2/3] namei.h: realign numbers Functional no-op, just for the eye's pleasure. Signed-off-by: Eygene Ryabinkin --- sys/sys/namei.h | 39 ++++++++++++++++++++------------------- 1 files changed, 20 insertions(+), 19 deletions(-) diff --git a/sys/sys/namei.h b/sys/sys/namei.h index 70e902c..c84a823 100644 --- a/sys/sys/namei.h +++ b/sys/sys/namei.h @@ -127,25 +127,26 @@ 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 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 --=20 1.6.3.1 --nFreZHaLTZJo0R7j Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="vfs_lookup-trailing-symlink-with-slash-fix-comment.diff" Content-Transfer-Encoding: quoted-printable =46rom e92f5e9751e04d458c3d8fcbd53d3cd727b1e75f Mon Sep 17 00:00:00 2001 =46rom: Eygene Ryabinkin Date: Wed, 27 May 2009 17:08:46 +0400 Subject: [PATCH 3/3] vfs_lookup: change misleading comment in namei() Signed-off-by: Eygene Ryabinkin --- sys/kern/vfs_lookup.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c index dc801fd..860bea0 100644 --- a/sys/kern/vfs_lookup.c +++ b/sys/kern/vfs_lookup.c @@ -227,7 +227,7 @@ namei(struct nameidata *ndp) vfslocked =3D (ndp->ni_cnd.cn_flags & GIANTHELD) !=3D 0; ndp->ni_cnd.cn_flags &=3D ~GIANTHELD; /* - * Check for symbolic link + * If not a symbolic link, we're done. */ if ((cnp->cn_flags & ISSYMLINK) =3D=3D 0) { if ((cnp->cn_flags & (SAVENAME | SAVESTART)) =3D=3D 0) { --=20 1.6.3.1 --nFreZHaLTZJo0R7j--