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>