FYI Lighttpd 1.4.23 /kernel (trailing '/' on regular file symlink) vulnerability

[Eygene Ryabinkin]

Wed, May 27, 2009 at 06:44:35PM +0200, Dag-Erling Sm??rgrav wrote:
> Eygene Ryabinkin <rea-fbsd at 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
    {_.-``-'         {_/            #
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vfs-testsuite-dirs-and-double-links.diff
Type: text/x-diff
Size: 2822 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20090527/60322528/vfs-testsuite-dirs-and-double-links.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vfs-testsuite-devices-as-destination.diff
Type: text/x-diff
Size: 934 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20090527/60322528/vfs-testsuite-devices-as-destination.bin