Re: [PATCH 3/3] use inode_change_ok for setattr permission checking

[Timothy Shimmin <tes@sgi.com>]

Christoph Hellwig wrote:
> Instead of implementing our own checks use inode_change_ok to check for
> nessecary permission in setattr.  

Yeah, the 1st bit I quite like and is similar to what I did in some
nfs4acl code, as you know.
We put all the EPERM cases early on which is nice.

> There is a slight change in behaviour
> as inode_change_ok doesn't allow i_mode updates to add the suid or sgid
> without superuser privilegues while the old XFS code just stripped away
> those bits from the file mode.
> 
This bit is of concern for me. And I want to understand.
It seems confusing.

So for xfs, we currently have it that if we try to set the suid/sgid bit
on the mode, and we are not the owner/group-member,
and we don't have CAP_FSETID,
then we clear that bit out of the mode (setuid/setgid) that we were going to set
below.

Now in inode_change_ok() we have some relevant code:

        /* Make sure a caller can chmod. */
        if (ia_valid & ATTR_MODE) {
                if (!is_owner_or_cap(inode))
                        goto error;
                /* Also check the setgid bit! */
                if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
                                inode->i_gid) && !capable(CAP_FSETID))
                        attr->ia_mode &= ~S_ISGID;
        }

It "looks" like it is doing a similar thing for the S_ISGID case but
not for the S_ISUID case.

And then we have similar code in inode_setattr()
        if (ia_valid & ATTR_MODE) {
                umode_t mode = attr->ia_mode;

                if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
                        mode &= ~S_ISGID;
                inode->i_mode = mode;
        }

But what about the suid case?

And also, what is with the ATTR_KILL_* bits?
Lemme look...

should_remove_suid:
  CAP_FSETID -> return 0
  else -> return (S_ISUID ? ATTR_KILL_SUID : 0) | (S_ISGID & S_IXGRP ? ATTR_KILL_SGID : 0)

do_truncate:
        /* Remove suid/sgid on truncate too */
        newattrs.ia_valid |= should_remove_suid(dentry);
        err = notify_change(dentry, &newattrs);

Oh okay,
so in notify_change() we clear the S_SUID/S_SGID bits in the cases that
ATTR_KILL_SUID/ATTR_KILL_SGID are set and let lower setattr
funcs interpret the KILL bits (as said in a comment).
Hmmm....but it first clears the bits out of the attr->ia_mode
so it does interpret them.

This seems a bit twisty to follow.

--Tim

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2008-09-29 18:27:29.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2008-09-29 18:27:32.000000000 +0200
> @@ -158,56 +161,6 @@ xfs_setattr(
>  
>  	xfs_ilock(ip, lock_flags);
>  
> -	/* boolean: are we the file owner? */
> -	file_owner = (current_fsuid(credp) == ip->i_d.di_uid);
> -
> -	/*
> -	 * Change various properties of a file.
> -	 * Only the owner or users with CAP_FOWNER
> -	 * capability may do these things.
> -	 */
> -	if (mask & (ATTR_MODE|ATTR_UID|ATTR_GID)) {
> -		/*
> -		 * CAP_FOWNER overrides the following restrictions:
> -		 *
> -		 * The user ID of the calling process must be equal
> -		 * to the file owner ID, except in cases where the
> -		 * CAP_FSETID capability is applicable.
> -		 */
> -		if (!file_owner && !capable(CAP_FOWNER)) {
> -			code = XFS_ERROR(EPERM);
> -			goto error_return;
> -		}
> -
> -		/*
> -		 * CAP_FSETID overrides the following restrictions:
> -		 *
> -		 * The effective user ID of the calling process shall match
> -		 * the file owner when setting the set-user-ID and
> -		 * set-group-ID bits on that file.
> -		 *
> -		 * The effective group ID or one of the supplementary group
> -		 * IDs of the calling process shall match the group owner of
> -		 * the file when setting the set-group-ID bit on that file
> -		 */
> -		if (mask & ATTR_MODE) {
> -			mode_t m = 0;
> -
> -			if ((iattr->ia_mode & S_ISUID) && !file_owner)
> -				m |= S_ISUID;
> -			if ((iattr->ia_mode & S_ISGID) &&
> -			    !in_group_p((gid_t)ip->i_d.di_gid))
> -				m |= S_ISGID;
> -#if 0
> -			/* Linux allows this, Irix doesn't. */
> -			if ((iattr->ia_mode & S_ISVTX) && !S_ISDIR(ip->i_d.di_mode))
> -				m |= S_ISVTX;
> -#endif
> -			if (m && !capable(CAP_FSETID))
> -				iattr->ia_mode &= ~m;
> -		}
> -	}
> -