Date: Wed, 27 May 2009 17:16:25 +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: <nhZ4ZNM2NtGGBpfrd4LGzlLPCPs@10Ilc7MfiXA2JVIRVQpZfk7cTQ4> In-Reply-To: <86prdug1p0.fsf@ds4.des.no> References: <23727599.post@talk.nabble.com> <86prdvipwe.fsf@ds4.des.no> <0vGjPHEq7MqxjtFmBufY%2BmBxlR4@7oUjtCwN654QcDr16CH%2BkAk8bJg> <86vdnmiz30.fsf@ds4.des.no> <15QQC%2B1YeDzOjf35dqyJmioc1ik@XX1fo6zQUfC4h0jjRC6IBz3oNH4> <86prdug1p0.fsf@ds4.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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
{_.-``-' {_/ #
[-- Attachment #2 --]
From 03483c8e800680a8b8a3d3f0d1debdf7fd883906 Mon Sep 17 00:00:00 2001
From: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
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'
[ == Unpatched variant == ]
$ file file/
file/: ASCII English text
[ == Patched variant == ]
$ file file/
file/: cannot open `file/' (Not a directory)
-----
See also: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/21768
See also: http://lists.freebsd.org/pipermail/freebsd-security/2009-May/005219.html
Signed-off-by: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
---
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 &= ~LOCKSHARED;
fdp = p->p_fd;
+ /* Drop internal flag: we will set it ourselves if we'll need it. */
+ cnp->cn_flags &= ~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 = dp = tdp;
}
+ /* Set "slashed" flag if we found slash at the end of the name */
+ if (trailing_slash && (cnp->cn_flags & ISLASTCN)) {
+ cnp->cn_flags |= SLASHTARGET;
+ }
+
/*
* Check for symbolic link
*/
@@ -710,14 +718,6 @@ unionlookup:
goto success;
}
- /*
- * Check for bogus trailing slashes.
- */
- if (trailing_slash && dp->v_type != VDIR) {
- error = 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 != VDIR) {
+ error = 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 */
#define NDHASGIANT(NDP) (((NDP)->ni_cnd.cn_flags & GIANTHELD) != 0)
--
1.6.3.1
[-- Attachment #3 --]
From 2539d4f31a2f85504672e8113343242782e737a7 Mon Sep 17 00:00:00 2001
From: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
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 <rea-fbsd@codelabs.ru>
---
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 */
--
1.6.3.1
[-- Attachment #4 --]
From e92f5e9751e04d458c3d8fcbd53d3cd727b1e75f Mon Sep 17 00:00:00 2001
From: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
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 <rea-fbsd@codelabs.ru>
---
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 = (ndp->ni_cnd.cn_flags & GIANTHELD) != 0;
ndp->ni_cnd.cn_flags &= ~GIANTHELD;
/*
- * Check for symbolic link
+ * If not a symbolic link, we're done.
*/
if ((cnp->cn_flags & ISSYMLINK) == 0) {
if ((cnp->cn_flags & (SAVENAME | SAVESTART)) == 0) {
--
1.6.3.1
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?nhZ4ZNM2NtGGBpfrd4LGzlLPCPs>