Re: [PATCH] Introduce is_owner_or_cap() to wrap CAP_FOWNER use with fsuid check

From: Serge E. Hallyn
Date: Tue Jul 17 2007 - 11:35:32 EST

Next message: david: "Re: Hibernation considerations"
Previous message: david: "Re: Hibernation considerations"
In reply to: Satyam Sharma: "[PATCH] Introduce is_owner_or_cap() to wrap CAP_FOWNER use with fsuid check"
Next in thread: Serge E. Hallyn: "Re: [PATCH] Introduce is_owner_or_cap() to wrap CAP_FOWNER use with fsuid check"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Quoting Satyam Sharma (ssatyam@xxxxxxxxxxxxxx):
> From: Satyam Sharma <ssatyam@xxxxxxxxxxxxxx>
>
> [PATCH] Introduce is_owner_or_cap() to wrap CAP_FOWNER use with fsuid check
>
> Introduce is_owner_or_cap() macro in fs.h, and convert over relevant
> users to it. This is done because we want to avoid bugs in the future

I like the idea - it will make future user namespace/per-container
capability patches much cleaner by localizing a lot of the changes.

(haven't tested the patch yet, but it looks trivial enough, and I will
test later today)

thanks,
-serge

> where we check for only effective fsuid of the current task against a
> file's owning uid, without simultaneously checking for CAP_FOWNER as
> well, thus violating its semantics.
> [ XFS uses special macros and structures, and in general looked ...
> untouchable, so we leave it alone -- but it has been looked over. ]
>
> The (current->fsuid != inode->i_uid) check in generic_permission() and
> exec_permission_lite() is left alone, because those operations are
> covered by CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH. Similarly operations
> falling under the purview of CAP_CHOWN and CAP_LEASE are also left alone.
>
> Signed-off-by: Satyam Sharma <ssatyam@xxxxxxxxxxxxxx>
>
> ---
>
> fs/attr.c | 4 ++--
> fs/ext2/acl.c | 2 +-
> fs/ext2/ioctl.c | 4 ++--
> fs/ext3/acl.c | 2 +-
> fs/ext3/ioctl.c | 6 +++---
> fs/ext4/acl.c | 2 +-
> fs/ext4/ioctl.c | 6 +++---
> fs/fcntl.c | 2 +-
> fs/generic_acl.c | 2 +-
> fs/gfs2/acl.c | 2 +-
> fs/hfsplus/ioctl.c | 2 +-
> fs/jffs2/acl.c | 2 +-
> fs/jfs/ioctl.c | 2 +-
> fs/jfs/xattr.c | 2 +-
> fs/namei.c | 2 +-
> fs/ocfs2/ioctl.c | 2 +-
> fs/reiserfs/ioctl.c | 5 ++---
> fs/reiserfs/xattr_acl.c | 2 +-
> fs/utimes.c | 2 +-
> fs/xattr.c | 3 +--
> include/linux/fs.h | 4 ++++
> security/selinux/hooks.c | 2 +-
> 22 files changed, 32 insertions(+), 30 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index a0a0c7b..f8dfc22 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -42,7 +42,7 @@ int inode_change_ok(struct inode *inode, struct iattr *attr)
>
> /* Make sure a caller can chmod. */
> if (ia_valid & ATTR_MODE) {
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> goto error;
> /* Also check the setgid bit! */
> if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
> @@ -52,7 +52,7 @@ int inode_change_ok(struct inode *inode, struct iattr *attr)
>
> /* Check for setting the inode time. */
> if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
> - if (current->fsuid != inode->i_uid && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> goto error;
> }
> fine:
> diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> index 7c420b8..e58669e 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -464,7 +464,7 @@ ext2_xattr_set_acl(struct inode *inode, int type, const void *value,
>
> if (!test_opt(inode->i_sb, POSIX_ACL))
> return -EOPNOTSUPP;
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> if (value) {
> diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
> index e85c482..3bcd254 100644
> --- a/fs/ext2/ioctl.c
> +++ b/fs/ext2/ioctl.c
> @@ -36,7 +36,7 @@ int ext2_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EACCES;
>
> if (get_user(flags, (int __user *) arg))
> @@ -74,7 +74,7 @@ int ext2_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
> case EXT2_IOC_GETVERSION:
> return put_user(inode->i_generation, (int __user *) arg);
> case EXT2_IOC_SETVERSION:
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
> if (IS_RDONLY(inode))
> return -EROFS;
> diff --git a/fs/ext3/acl.c b/fs/ext3/acl.c
> index 1e5038d..d34e996 100644
> --- a/fs/ext3/acl.c
> +++ b/fs/ext3/acl.c
> @@ -489,7 +489,7 @@ ext3_xattr_set_acl(struct inode *inode, int type, const void *value,
>
> if (!test_opt(inode->i_sb, POSIX_ACL))
> return -EOPNOTSUPP;
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> if (value) {
> diff --git a/fs/ext3/ioctl.c b/fs/ext3/ioctl.c
> index 965006d..4a2a02c 100644
> --- a/fs/ext3/ioctl.c
> +++ b/fs/ext3/ioctl.c
> @@ -41,7 +41,7 @@ int ext3_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EACCES;
>
> if (get_user(flags, (int __user *) arg))
> @@ -122,7 +122,7 @@ flags_err:
> __u32 generation;
> int err;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
> if (IS_RDONLY(inode))
> return -EROFS;
> @@ -181,7 +181,7 @@ flags_err:
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EACCES;
>
> if (get_user(rsv_window_size, (int __user *)arg))
> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
> index 9e88254..a8bae8c 100644
> --- a/fs/ext4/acl.c
> +++ b/fs/ext4/acl.c
> @@ -489,7 +489,7 @@ ext4_xattr_set_acl(struct inode *inode, int type, const void *value,
>
> if (!test_opt(inode->i_sb, POSIX_ACL))
> return -EOPNOTSUPP;
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> if (value) {
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 500567d..7b4aa45 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -40,7 +40,7 @@ int ext4_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EACCES;
>
> if (get_user(flags, (int __user *) arg))
> @@ -121,7 +121,7 @@ flags_err:
> __u32 generation;
> int err;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
> if (IS_RDONLY(inode))
> return -EROFS;
> @@ -180,7 +180,7 @@ flags_err:
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EACCES;
>
> if (get_user(rsv_window_size, (int __user *)arg))
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 8e382a5..3f22e9f 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -215,7 +215,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
>
> /* O_NOATIME can only be set by the owner or superuser */
> if ((arg & O_NOATIME) && !(filp->f_flags & O_NOATIME))
> - if (current->fsuid != inode->i_uid && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> /* required for strict SunOS emulation */
> diff --git a/fs/generic_acl.c b/fs/generic_acl.c
> index 9ccb789..995d63b 100644
> --- a/fs/generic_acl.c
> +++ b/fs/generic_acl.c
> @@ -78,7 +78,7 @@ generic_acl_set(struct inode *inode, struct generic_acl_operations *ops,
>
> if (S_ISLNK(inode->i_mode))
> return -EOPNOTSUPP;
> - if (current->fsuid != inode->i_uid && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
> if (value) {
> acl = posix_acl_from_xattr(value, size);
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index 6e80844..1047a8c 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -74,7 +74,7 @@ int gfs2_acl_validate_remove(struct gfs2_inode *ip, int access)
> {
> if (!GFS2_SB(&ip->i_inode)->sd_args.ar_posix_acl)
> return -EOPNOTSUPP;
> - if (current->fsuid != ip->i_inode.i_uid && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(&ip->i_inode))
> return -EPERM;
> if (S_ISLNK(ip->i_inode.i_mode))
> return -EOPNOTSUPP;
> diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
> index 79fd104..b60c0af 100644
> --- a/fs/hfsplus/ioctl.c
> +++ b/fs/hfsplus/ioctl.c
> @@ -38,7 +38,7 @@ int hfsplus_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EACCES;
>
> if (get_user(flags, (int __user *)arg))
> diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c
> index a46101e..65b3a1b 100644
> --- a/fs/jffs2/acl.c
> +++ b/fs/jffs2/acl.c
> @@ -435,7 +435,7 @@ static int jffs2_acl_setxattr(struct inode *inode, int type, const void *value,
> struct posix_acl *acl;
> int rc;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> if (value) {
> diff --git a/fs/jfs/ioctl.c b/fs/jfs/ioctl.c
> index fe063af..3c8663b 100644
> --- a/fs/jfs/ioctl.c
> +++ b/fs/jfs/ioctl.c
> @@ -69,7 +69,7 @@ int jfs_ioctl(struct inode * inode, struct file * filp, unsigned int cmd,
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EACCES;
>
> if (get_user(flags, (int __user *) arg))
> diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
> index b2375f0..9b7f2cd 100644
> --- a/fs/jfs/xattr.c
> +++ b/fs/jfs/xattr.c
> @@ -697,7 +697,7 @@ static int can_set_system_xattr(struct inode *inode, const char *name,
> struct posix_acl *acl;
> int rc;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> /*
> diff --git a/fs/namei.c b/fs/namei.c
> index 5e2d98d..defaa47 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1576,7 +1576,7 @@ int may_open(struct nameidata *nd, int acc_mode, int flag)
>
> /* O_NOATIME can only be set by the owner or superuser */
> if (flag & O_NOATIME)
> - if (current->fsuid != inode->i_uid && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> /*
> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
> index bd68c3f..87dcece 100644
> --- a/fs/ocfs2/ioctl.c
> +++ b/fs/ocfs2/ioctl.c
> @@ -63,7 +63,7 @@ static int ocfs2_set_inode_attr(struct inode *inode, unsigned flags,
> goto bail_unlock;
>
> status = -EACCES;
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> goto bail_unlock;
>
> if (!S_ISDIR(inode->i_mode))
> diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c
> index b484d29..11a0fcc 100644
> --- a/fs/reiserfs/ioctl.c
> +++ b/fs/reiserfs/ioctl.c
> @@ -51,8 +51,7 @@ int reiserfs_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
> if (IS_RDONLY(inode))
> return -EROFS;
>
> - if ((current->fsuid != inode->i_uid)
> - && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> if (get_user(flags, (int __user *)arg))
> @@ -81,7 +80,7 @@ int reiserfs_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
> case REISERFS_IOC_GETVERSION:
> return put_user(inode->i_generation, (int __user *)arg);
> case REISERFS_IOC_SETVERSION:
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
> if (IS_RDONLY(inode))
> return -EROFS;
> diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
> index 5296a29..b7e4fa4 100644
> --- a/fs/reiserfs/xattr_acl.c
> +++ b/fs/reiserfs/xattr_acl.c
> @@ -21,7 +21,7 @@ xattr_set_acl(struct inode *inode, int type, const void *value, size_t size)
>
> if (!reiserfs_posixacl(inode->i_sb))
> return -EOPNOTSUPP;
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> if (value) {
> diff --git a/fs/utimes.c b/fs/utimes.c
> index 83a7e69..682eb63 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -106,7 +106,7 @@ long do_utimes(int dfd, char __user *filename, struct timespec *times, int flags
> if (IS_IMMUTABLE(inode))
> goto dput_and_out;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER)) {
> + if (!is_owner_or_cap(inode)) {
> if (f) {
> if (!(f->f_mode & FMODE_WRITE))
> goto dput_and_out;
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4523aca..a44fd92 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -60,8 +60,7 @@ xattr_permission(struct inode *inode, const char *name, int mask)
> if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> return -EPERM;
> if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
> - (mask & MAY_WRITE) && (current->fsuid != inode->i_uid) &&
> - !capable(CAP_FOWNER))
> + (mask & MAY_WRITE) && !is_owner_or_cap(inode))
> return -EPERM;
> }
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e687808..aa33a3f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -283,6 +283,7 @@ extern int dir_notify_enable;
> #include <linux/init.h>
> #include <linux/pid.h>
> #include <linux/mutex.h>
> +#include <linux/capability.h>
>
> #include <asm/atomic.h>
> #include <asm/semaphore.h>
> @@ -988,6 +989,9 @@ enum {
> #define put_fs_excl() atomic_dec(&current->fs_excl)
> #define has_fs_excl() atomic_read(&current->fs_excl)
>
> +#define is_owner_or_cap(inode) \
> + ((current->fsuid == (inode)->i_uid) || capable(CAP_FOWNER))
> +
> /* not quite ready to be deprecated, but... */
> extern void lock_super(struct super_block *);
> extern void unlock_super(struct super_block *);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 78c3f98..520b999 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2318,7 +2318,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, char *name, void *value
> if (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)
> return -EOPNOTSUPP;
>
> - if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> + if (!is_owner_or_cap(inode))
> return -EPERM;
>
> AVC_AUDIT_DATA_INIT(&ad,FS);
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: david: "Re: Hibernation considerations"
Previous message: david: "Re: Hibernation considerations"
In reply to: Satyam Sharma: "[PATCH] Introduce is_owner_or_cap() to wrap CAP_FOWNER use with fsuid check"
Next in thread: Serge E. Hallyn: "Re: [PATCH] Introduce is_owner_or_cap() to wrap CAP_FOWNER use with fsuid check"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]