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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 30 May 2010 21:23:37 -0700
From:	Kees Cook <kees.cook@...onical.com>
To:	Eric Paris <eparis@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org,
	Randy Dunlap <rdunlap@...otime.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jiri Kosina <jkosina@...e.cz>,
	Dave Young <hidave.darkstar@...il.com>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	James Morris <jmorris@...ei.org>,
	David Howells <dhowells@...hat.com>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Tim Gardner <tim.gardner@...onical.com>,
	"Serge E. Hallyn" <serue@...ibm.com>, sds@...ho.nsa.gov,
	selinux@...ho.nsa.gov
Subject: Re: [PATCH v2] fs: block cross-uid sticky symlinks

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/