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

[Eygene Ryabinkin]

Dag-Erling, good day.

Fri, May 29, 2009 at 06:58:08PM +0200, Dag-Erling Sm??rgrav wrote:
> Index: sys/kern/vfs_lookup.c
> ===================================================================
> --- sys/kern/vfs_lookup.c	(revision 193028)
> +++ sys/kern/vfs_lookup.c	(working copy)
> @@ -454,7 +454,6 @@
>  	int docache;			/* == 0 do not cache last component */
>  	int wantparent;			/* 1 => wantparent or lockparent flag */
>  	int rdonly;			/* lookup read-only flag bit */
> -	int trailing_slash;
>  	int error = 0;
>  	int dpunlocked = 0;		/* dp has already been unlocked */
>  	struct componentname *cnp = &ndp->ni_cnd;
> @@ -529,12 +528,10 @@
>  	 * trailing slashes to handle symlinks, existing non-directories
>  	 * and non-existing files that won't be directories specially later.
>  	 */
> -	trailing_slash = 0;
>  	while (*cp == '/' && (cp[1] == '/' || cp[1] == '\0')) {
>  		cp++;
>  		ndp->ni_pathlen--;
>  		if (*cp == '\0') {
> -			trailing_slash = 1;
>  			*ndp->ni_next = '\0';	/* XXX for direnter() ... */
>  			cnp->cn_flags |= TRAILINGSLASH;
>  		}
> @@ -711,7 +708,7 @@
>  			error = EROFS;
>  			goto bad;
>  		}
> -		if (*cp == '\0' && trailing_slash &&
> +		if (*cp == '\0' && (cnp->cn_flags & TRAILINGSLASH) &&
>  		     !(cnp->cn_flags & WILLBEDIR)) {

For the current code state, check "*cp == '\0'" seems to be redundant:
VOP_LOOKUP had failed at this point and we're running with EJUSTRETURN.
And EJUSTRETURN can happen only if ISLASTCN is set, that in turn implies
that *ndp->ni_next is 0 and ndp->ni_next is equal to the cp.

Seems like this change makes symlink behavior more consistent: if any
previous component had a trailing slash, then we will bail out unless
we intend to create/rename directory.

By the way, comment before the test "if (rdonly)' seems to be slightly
misleading: we could be not only creating the file but could be renaming
at this point as well.  Perhaps 'creating' should be changed to
'creating/renaming'.

>  			error = ENOENT;
>  			goto bad;
> @@ -788,7 +785,7 @@
>  	 * Check for symbolic link
>  	 */
>  	if ((dp->v_type == VLNK) &&
> -	    ((cnp->cn_flags & FOLLOW) || trailing_slash ||
> +	    ((cnp->cn_flags & FOLLOW) || (cnp->cn_flags & TRAILINGSLASH) ||
>  	     *ndp->ni_next == '/')) {
>  		cnp->cn_flags |= ISSYMLINK;
>  		if (dp->v_iflag & VI_DOOMED) {

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.

> BTW, what does the "XXX for direnter()" comment mean?

It means that the trailing slashes are eliminated because direnter() for
some FSes may choke on it.  It essentially duplicates the chunk of a big
comment before the block.  'ngp->ni_next' is set to 'cp' before the
block and at that time 'cp' points to a slash that delimits directories
or null character that terminates non-slashed name.

Perhaps 'XXX for direnter()' should be changed to something like
'strip trailing slashes in cnp->cn_nameptr'.

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