Date: Wed, 27 May 2009 21:28:49 +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: <U0NhVtBGTmaNGU7BFYREWm3L/jc@al5XJ3LyQHdj2bEPlOOCMqn3f1Q> In-Reply-To: <86vdnmijgs.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> <nhZ4ZNM2NtGGBpfrd4LGzlLPCPs@10Ilc7MfiXA2JVIRVQpZfk7cTQ4> <86vdnmijgs.fsf@ds4.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
--opJtzjQTFsWo+cga
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Wed, May 27, 2009 at 06:44:35PM +0200, Dag-Erling Sm??rgrav wrote:
> Eygene Ryabinkin <rea-fbsd@codelabs.ru> writes:
> > [new three-part patch]
>
> I committed the namei.h cleanup patch and the vfs_lookup.c comment
> patch.
Thanks!
> I made a number of changes to the trailing-slash patch. Can you
> double-check it before I commit it?
Yes, comments are below.
> Index: sys/kern/vfs_lookup.c
> ===================================================================
> --- sys/kern/vfs_lookup.c (revision 192899)
> +++ sys/kern/vfs_lookup.c (working copy)
> @@ -147,6 +147,9 @@
> cnp->cn_flags &= ~LOCKSHARED;
> fdp = p->p_fd;
>
> + /* We will set this ourselves if we need it. */
> + cnp->cn_flags &= ~TRAILINGSLASH;
> +
> /*
> * Get a buffer for the name to be translated, and copy the
> * name into the buffer.
> @@ -533,6 +536,8 @@
> if (*cp == '\0') {
> trailing_slash = 1;
> *ndp->ni_next = '\0'; /* XXX for direnter() ... */
> + if (cnp->cn_flags & ISLASTCN)
> + cnp->cn_flags |= TRAILINGSLASH;
'if ()' looks suspicious: ISLASTCN is set some lines below so it could
be not yet flagged. Seems like we could omit 'if ()' clause but leave
it's body for the current state of the code -- it will be equivalent to
the mine's check.
But for the clarity, I will leave the full condition, 'trailing_slash &&
(cnp->cn_flags & ISLASTCN)' somewhere below the block with
-----
if (*ndp->ni_next == 0)
cnp->cn_flags |= ISLASTCN;
else
cnp->cn_flags &= ~ISLASTCN;
-----
My original intent was to push it to the bottom of the code to slightly
optimize code path: some checks above could already fail and we won't
have to perform our test. But now I feel that the best place for the
test is immediately below the cited chunk of the code.
The rest looks fine. Had you tried your variant of patch? May be I
am missing something and the test for ISLASTCN really in place?
By the way, I had somewhat extended your regression tests with the
intermediate symlink tests, directory tests and device-as-a-target
tests. Patches are attached. Will they go?
Thanks!
--
Eygene
_ ___ _.--. #
\`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard
/ ' ` , __.--' # to read the on-line manual
)/' _/ \ `-_, / # while single-stepping the kernel.
`-'" `"\_ ,_.-;_.-\_ ', fsc/as #
_.-'_./ {_.' ; / # -- FreeBSD Developers handbook
{_.-``-' {_/ #
--opJtzjQTFsWo+cga
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="vfs-testsuite-dirs-and-double-links.diff"
Content-Transfer-Encoding: quoted-printable
=46rom 8ed2a144245bdb714217f982f6ee1f7d0b784b1c Mon Sep 17 00:00:00 2001
=46rom: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
Date: Wed, 27 May 2009 20:55:50 +0400
Subject: [PATCH 4/5] vfs regression testuite: add double links and director=
y tests
Directory tests are to make sure that no regressions were introduced by
patches -- they should work on the systems without patched vfs_lookup as
at the patched ones.
Double link tests should verify that if any part of the symlink chain
has trailing slash, then target should be a directory.
Signed-off-by: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
---
tools/regression/vfs/trailing_slash.t | 67 +++++++++++++++++++++++++++++=
+++-
1 files changed, 66 insertions(+), 1 deletions(-)
diff --git a/tools/regression/vfs/trailing_slash.t b/tools/regression/vfs/t=
railing_slash.t
index fe6d799..5209979 100755
--- a/tools/regression/vfs/trailing_slash.t
+++ b/tools/regression/vfs/trailing_slash.t
@@ -6,8 +6,10 @@
# point to files. See kern/21768
#
=20
+testdir=3D"/tmp/testdir-$$"
testfile=3D"/tmp/testfile-$$"
testlink=3D"/tmp/testlink-$$"
+testlink1=3D"/tmp/testlink1-$$"
=20
tests=3D"
$testfile:$testlink:$testfile:0
@@ -18,8 +20,29 @@ $testfile/:$testlink:$testlink:1
$testfile/:$testlink:$testlink/:1
"
=20
+tests1=3D"
+$testfile:$testlink:$testlink:$testlink1:$testlink1:0
+$testfile:$testlink:$testlink/:$testlink1:$testlink1:1
+$testfile:$testlink:$testlink:$testlink1:$testlink1/:1
+$testfile:$testlink:$testlink/:$testlink1:$testlink1/:1
+$testfile/:$testlink:$testlink:$testlink1:$testlink1:1
+$testfile/:$testlink:$testlink/:$testlink1:$testlink1:1
+$testfile/:$testlink:$testlink:$testlink1:$testlink1/:1
+$testfile/:$testlink:$testlink/:$testlink1:$testlink1/:1
+"
+
+dirtests=3D"
+$testdir:$testlink:$testdir:0
+$testdir:$testlink:$testdir/:0
+$testdir:$testlink:$testlink:0
+$testdir:$testlink:$testlink/:0
+$testdir/:$testlink:$testlink:0
+$testdir/:$testlink:$testlink/:0
+"
+
touch $testfile || exit 1
-trap "rm $testfile $testlink" EXIT
+mkdir $testdir || exit 1
+trap "rm $testfile $testlink $testlink1; rmdir $testdir" EXIT
=20
set $tests
echo "1..$#"
@@ -40,3 +63,45 @@ for testspec ; do
n=3D$((n+1))
)
done
+
+set $tests1
+echo "1..$#"
+n=3D1
+for testspec ; do
+ (
+ IFS=3D:
+ set $testspec
+ unset IFS
+ ln -fs "$1" "$2" || exit 1
+ ln -fs "$3" "$4" || exit 1
+ cat "$5" >/dev/null 2>&1
+ ret=3D$?
+ if [ "$ret" -eq "$6" ] ; then
+ echo "ok $n"
+ else
+ echo "fail $n - expected $6, got $ret"
+ fi
+ n=3D$((n+1))
+ )
+done
+
+set $dirtests
+echo "1..$#"
+n=3D1
+for testspec ; do
+ (
+ IFS=3D:
+ set $testspec
+ unset IFS
+ rm -f "$2" || exit 1
+ ln -fs "$1" "$2" || exit 1
+ touch "$3" >/dev/null 2>&1
+ ret=3D$?
+ if [ "$ret" -eq "$4" ] ; then
+ echo "ok $n"
+ else
+ echo "fail $n - expected $4, got $ret"
+ fi
+ n=3D$((n+1))
+ )
+done
--=20
1.6.3.1
--opJtzjQTFsWo+cga
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="vfs-testsuite-devices-as-destination.diff"
Content-Transfer-Encoding: quoted-printable
=46rom 2da7b94bc81a81a41550a95821ed54744136400e Mon Sep 17 00:00:00 2001
=46rom: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
Date: Wed, 27 May 2009 21:02:05 +0400
Subject: [PATCH 5/5] vfs regression testsuite: add tests for device being t=
he destination
Signed-off-by: Eygene Ryabinkin <rea-fbsd@codelabs.ru>
---
tools/regression/vfs/trailing_slash.t | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/tools/regression/vfs/trailing_slash.t b/tools/regression/vfs/t=
railing_slash.t
index 5209979..cd82a8b 100755
--- a/tools/regression/vfs/trailing_slash.t
+++ b/tools/regression/vfs/trailing_slash.t
@@ -18,6 +18,10 @@ $testfile:$testlink:$testlink:0
$testfile:$testlink:$testlink/:1
$testfile/:$testlink:$testlink:1
$testfile/:$testlink:$testlink/:1
+/dev/null:$testlink:$testlink:0
+/dev/null:$testlink:$testlink/:1
+/dev/null/:$testlink:$testlink:1
+/dev/null/:$testlink:$testlink/:1
"
=20
tests1=3D"
--=20
1.6.3.1
--opJtzjQTFsWo+cga--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?U0NhVtBGTmaNGU7BFYREWm3L/jc>