lkml.org 
[lkml]   [2010]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2010-05-31 06:27    [W:0.040 / U:29.368 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site