Date: Wed, 27 May 2009 16:07:09 +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: <15QQC%2B1YeDzOjf35dqyJmioc1ik@XX1fo6zQUfC4h0jjRC6IBz3oNH4> In-Reply-To: <86vdnmiz30.fsf@ds4.des.no> References: <23727599.post@talk.nabble.com> <86prdvipwe.fsf@ds4.des.no> <0vGjPHEq7MqxjtFmBufY%2BmBxlR4@7oUjtCwN654QcDr16CH%2BkAk8bJg> <86vdnmiz30.fsf@ds4.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <rea-fbsd@codelabs.ru> 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 <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 | 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?15QQC%2B1YeDzOjf35dqyJmioc1ik>