Date: Wed, 27 May 2009 13:52:21 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Dag-Erling Sm??rgrav <des@des.no> Cc: freebsd-hackers@freebsd.org, Jakub Lach <jakub_lach@mailplus.pl> Subject: Re: FYI Lighttpd 1.4.23 /kernel (trailing '/' on regular file symlink) vulnerability Message-ID: <0vGjPHEq7MqxjtFmBufY%2BmBxlR4@7oUjtCwN654QcDr16CH%2BkAk8bJg> In-Reply-To: <86prdvipwe.fsf@ds4.des.no> References: <23727599.post@talk.nabble.com> <86prdvipwe.fsf@ds4.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
--7JfCtLOvnd9MIVvH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Dag-Erling, *, good day. Tue, May 26, 2009 at 10:13:21PM +0200, Dag-Erling Sm??rgrav wrote: > [moving from security@ to hackers@] > > Jakub Lach <jakub_lach@mailplus.pl> writes: > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/21768 > > Like bde@ pointed out, the patch is incorrect. It moves the test for > v_type != VDIR up to a point where, in the case of a symlink, v_type is > always (by definition) VLNK. > > The reason why the current code does not work is that, in the symlink > case, the v_type != VDIR test is never reached: we will have jumped to > either bad2 or success. However, it should be safe to move the test to > after the success label, because trailing_slash is only ever true for > the last component of the path we were asked to look up (see lines 520 > through 535). May be the attached patch will fix the thing? It works for me for 7.2 with WITNESS and INVARIANTS enabled. It adds an additional flag, but this was the only thing I was able to invent to avoid ABI breakage. -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ # --7JfCtLOvnd9MIVvH 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 029b779c2fe005fe0d043fb3f1990957927e6a18 Mon Sep 17 00:00:00 2001 =46rom: Eygene Ryabinkin <rea-fbsd@codelabs.ru> 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 <rea-fbsd@codelabs.ru> --- sys/kern/vfs_lookup.c | 25 +++++++++++++++++-------- sys/sys/namei.h | 41 +++++++++++++++++++++-------------------- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c index 3770b55..75b1772 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 ~SLASHSYMLINK; + /* * Get a buffer for the name to be translated, and copy the * name into the buffer. @@ -683,6 +686,12 @@ unionlookup: ndp->ni_vp =3D dp =3D tdp; } =20 + /* Set slashed symlink flag if we found slash at the end of symlink */ + if (dp->v_type =3D=3D VLNK && trailing_slash && + (cnp->cn_flags & ISLASTCN)) { + cnp->cn_flags |=3D SLASHSYMLINK; + } + /* * Check for symbolic link */ @@ -710,14 +719,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 +742,14 @@ nextname: goto dirloop; } /* + * Check if we're processing slashed symlink and + * lookup target isn't a directory. + */ + if ((cnp->cn_flags & SLASHSYMLINK) && 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..d73da50 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 SLASHSYMLINK 0x10000000 /* last component was slashed symlink */ +#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 --7JfCtLOvnd9MIVvH--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0vGjPHEq7MqxjtFmBufY%2BmBxlR4>