Re: [PATCH v2] fs: block cross-uid sticky symlinks

[Kees Cook <kees.cook@canonical.com>]

Hi Eric,

On Sun, May 30, 2010 at 11:54:23PM -0400, Eric Paris wrote:
> We need to call this function in the SELinux case.  So you'll need a
> patch like the one attached (not even compiled but I think it is right)
> [..]
>  static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *nameidata)
> [..]
> +       rc = cap_inode_follow_link(dentry, nameidata);

Yeah, when I quickly checked SELinux and AppArmor, it seemed that they
were always calling down to all commoncaps functions, but it looks like
not in all cases.  I think that Eric Biederman's observations here makes
the most sense: this check needs to happen without involving the LSMs
at all.

> > +int cap_inode_follow_link(struct dentry *dentry,
> > +			  struct nameidata *nameidata)
> > +{
> > +	const struct inode *parent = dentry->d_parent->d_inode;
> > +	const struct inode *inode = dentry->d_inode;
> > +	const struct cred *cred = current_cred();
> > +
> > +	if (weak_sticky_symlinks)
> > +		return 0;
> > +
> > +	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
> > +	    parent->i_uid != inode->i_uid &&
> > +	    cred->fsuid != inode->i_uid) {
> > +		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
> > +			"following attempted in sticky-directory by "
> > +			"%s (fsuid %d)\n", current->comm, cred->fsuid);
> > +		return -EACCES;
> > +	}
> > +	return 0;
> > +}
> 
> What stops us from racing between the assignment of parent and it's
> first use with a rename on our object and rmdir on the old parent?  I'm
> wondering if we need to be doing this test holding dentry->d_lock (which
> is what protects dentry->d_parent if I recall correctly)
> 
> Certainly doesn't fix all of the raciness, but I think it would close
> the opps part.  Maybe someone who knows the VFS better can tell me if I
> am misguided.

The only other use of d_parent I can see there is in may_delete().  With
vfs_unlink() calling that, it would seem to be racey too if we needed to
hold a lock for that.  But it's not clear to me in vfs_follow_link is doing
locking somehow.

Thanks,

-Kees

-- 
Kees Cook
Ubuntu Security Team