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

[Eygene Ryabinkin]

Tue, Jun 02, 2009 at 01:28:34PM +0200, Dag-Erling Sm??rgrav wrote:
> Eygene Ryabinkin <rea-fbsd at codelabs.ru> writes:
> > For the current code state, check "*cp == '\0'" seems to be redundant:
> > [...]  By the way, comment before the test "if (rdonly)' seems to be
> > slightly misleading [...]
> 
> OK, I see that.  I've removed the check and rewritten the comment.

Thanks.

> What about this "temporary assert"?
> 
> 		/*
> 		 * This is a temporary assert to make sure I know what the
> 		 * behavior here was.
> 		 */
> 		KASSERT((cnp->cn_flags & (WANTPARENT|LOCKPARENT)) != 0,
> 		   ("lookup: Unhandled case."));

It is partly handled at the beginning of lookup:
-----
        wantparent = cnp->cn_flags & (LOCKPARENT | WANTPARENT);
        KASSERT(cnp->cn_nameiop == LOOKUP || wantparent,
            ("CREATE, DELETE, RENAME require LOCKPARENT or WANTPARENT."));
-----
So, only LOOKUP could have both {LOCK,WANT}PARENT to be unset.  And it
seems to be fine, because LOOKUP shouldn't care about parent vnode,
either locked or just referenced.  So this assertion seems to be really
redundant.

> > Seems like here we can also check for the trailing slash to be set on
> > any previous invocation (or the current one), since we're following
> > symlinks in the case of directories, so we should follow them to the
> > end.
> 
> I'm not sure I understand...

I meant the following: you're trading 'trailing_slash' to the
TRAILINGSLASH.  The former is local to this call of lookup(), the latter
persists throughout the namei() invocation, so it could be set by the
previous lookup() calls.  But this seems to be fine: since we were
started to look for directory at some point, we should continue to do
it.

> > Perhaps 'XXX for direnter()' should be changed to something like
> > 'strip trailing slashes in cnp->cn_nameptr'.
> 
> I'll just remove it, since the previous comment clearly explains what is
> going on.

May be it's better to leave the comment, but replace it with more
undestandable one: this instruction is a bit tricky and it makes one to
think what the hell is going on.

> > By the way, comment just after 'nextname' label is misleading: we can be
> > there with symbolic link, but it won't be followed.  So "Not a symbolic
> > link" can be changed to something like "Not a symbolic link that will
> > be followed".
> 
> 	/*
> 	 * Not a symbolic link that we will follow.  Continue with the
> 	 * next component if there is any; otherwise, we're done.
> 	 */

Yes, very good.

Thanks.
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #