Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1386871
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v3 14/21] fs: Allow superblock owner to change ownership of inodes with unmappable ids |
| Date | 2016-04-25 22:40 +0200 |
| Message-ID | <rrVsu-3wP-21@gated-at.bofh.it> (permalink) |
| References | <rqLlw-44Z-5@gated-at.bofh.it> <rqLvc-49a-27@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Quoting Seth Forshee (seth.forshee@canonical.com):
> In a userns mount some on-disk inodes may have ids which do not
> map into s_user_ns, in which case the in-kernel inodes are owned
> by invalid users. The superblock owner should be able to change
> attributes of these inodes but cannot. However it is unsafe to
> grant the superblock owner privileged access to all inodes in the
> superblock since proc, sysfs, etc. use DAC to protect files which
> may not belong to s_user_ns. The problem is restricted to only
> inodes where the owner or group is an invalid user.
>
> We can work around this by allowing users with CAP_CHOWN in
> s_user_ns to change an invalid owner or group id, so long as the
> other id is either invalid or mappable in s_user_ns. After
> changing ownership the user will be privileged towards the inode
> and thus able to change other attributes.
>
> As an precaution, checks for invalid ids are added to the proc
> and kernfs setattr interfaces. These filesystems are not expected
> to have inodes with invalid ids, but if it does happen any
> setattr operations will return -EPERM.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
bug a request below,
> ---
> fs/attr.c | 47 +++++++++++++++++++++++++++++++++++++++--------
> fs/kernfs/inode.c | 2 ++
> fs/proc/base.c | 2 ++
> fs/proc/generic.c | 3 +++
> fs/proc/proc_sysctl.c | 2 ++
> 5 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 3cfaaac4a18e..a8b0931654a5 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -16,6 +16,43 @@
> #include <linux/evm.h>
> #include <linux/ima.h>
>
> +static bool chown_ok(const struct inode *inode, kuid_t uid)
> +{
> + struct user_namespace *user_ns;
> +
> + if (uid_eq(current_fsuid(), inode->i_uid) && uid_eq(uid, inode->i_uid))
> + return true;
> + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN))
> + return true;
> +
> + user_ns = inode->i_sb->s_user_ns;
> + if (!uid_valid(inode->i_uid) &&
> + (!gid_valid(inode->i_gid) || kgid_has_mapping(user_ns, inode->i_gid)) &&
This confused me to no end :) Perhaps a "is_unmapped_valid_gid()" helper
would make it clearer what this is meant to do? Or else maybe a comment
above chown_ok(), explaining that
1. for a blockdev, the uid is converted at inode read so that it is
either mapped or invalid
2. for sysfs / etc, uid can be valid but not mapped into the userns
> + ns_capable(user_ns, CAP_CHOWN))
> + return true;
> +
> + return false;
> +}
> +
> +static bool chgrp_ok(const struct inode *inode, kgid_t gid)
> +{
> + struct user_namespace *user_ns;
> +
> + if (uid_eq(current_fsuid(), inode->i_uid) &&
> + (in_group_p(gid) || gid_eq(gid, inode->i_gid)))
> + return true;
> + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN))
> + return true;
> +
> + user_ns = inode->i_sb->s_user_ns;
> + if (!gid_valid(inode->i_gid) &&
> + (!uid_valid(inode->i_uid) || kuid_has_mapping(user_ns, inode->i_uid)) &&
> + ns_capable(user_ns, CAP_CHOWN))
> + return true;
> +
> + return false;
> +}
> +
> /**
> * inode_change_ok - check if attribute changes to an inode are allowed
> * @inode: inode to check
> @@ -58,17 +95,11 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr)
> return 0;
>
> /* Make sure a caller can chown. */
> - if ((ia_valid & ATTR_UID) &&
> - (!uid_eq(current_fsuid(), inode->i_uid) ||
> - !uid_eq(attr->ia_uid, inode->i_uid)) &&
> - !capable_wrt_inode_uidgid(inode, CAP_CHOWN))
> + if ((ia_valid & ATTR_UID) && !chown_ok(inode, attr->ia_uid))
> return -EPERM;
>
> /* Make sure caller can chgrp. */
> - if ((ia_valid & ATTR_GID) &&
> - (!uid_eq(current_fsuid(), inode->i_uid) ||
> - (!in_group_p(attr->ia_gid) && !gid_eq(attr->ia_gid, inode->i_gid))) &&
> - !capable_wrt_inode_uidgid(inode, CAP_CHOWN))
> + if ((ia_valid & ATTR_GID) && !chgrp_ok(inode, attr->ia_gid))
> return -EPERM;
>
> /* Make sure a caller can chmod. */
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 16405ae88d2d..2e97a337ee5f 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -117,6 +117,8 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
>
> if (!kn)
> return -EINVAL;
> + if (!uid_valid(inode->i_uid) || !gid_valid(inode->i_gid))
> + return -EPERM;
>
> mutex_lock(&kernfs_mutex);
> error = inode_change_ok(inode, iattr);
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index b1755b23893e..648d623e2158 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -711,6 +711,8 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr)
>
> if (attr->ia_valid & ATTR_MODE)
> return -EPERM;
> + if (!uid_valid(inode->i_uid) || !gid_valid(inode->i_gid))
> + return -EPERM;
>
> error = inode_change_ok(inode, attr);
> if (error)
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index ff3ffc76a937..1461570c552c 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -105,6 +105,9 @@ static int proc_notify_change(struct dentry *dentry, struct iattr *iattr)
> struct proc_dir_entry *de = PDE(inode);
> int error;
>
> + if (!uid_valid(inode->i_uid) || !gid_valid(inode->i_gid))
> + return -EPERM;
> +
> error = inode_change_ok(inode, iattr);
> if (error)
> return error;
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index fe5b6e6c4671..f5d575157194 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -752,6 +752,8 @@ static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr)
>
> if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> return -EPERM;
> + if (!uid_valid(inode->i_uid) || !gid_valid(inode->i_gid))
> + return -EPERM;
>
> error = inode_change_ok(inode, attr);
> if (error)
> --
> 1.9.1
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v3 00/21] Support fuse mounts in user namespaces Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:40 +0200
[PATCH v3 17/21] capabilities: Allow privileged user in s_user_ns to set security.* xattrs Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:40 +0200
[PATCH v3 01/21] fs: fix a posible leak of allocated superblock Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:40 +0200
[PATCH v3 15/21] fs: Don't remove suid for CAP_FSETID in s_user_ns Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:40 +0200
[PATCH v3 16/21] fs: Allow superblock owner to access do_remount_sb() Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 05/21] block_dev: Check permissions towards block device inode when mounting Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 04/21] block_dev: Support checking inode permissions in lookup_bdev() Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 12/21] fs: Refuse uid/gid changes which don't map into s_user_ns Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 14/21] fs: Allow superblock owner to change ownership of inodes with unmappable ids Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
Re: [PATCH v3 14/21] fs: Allow superblock owner to change ownership of inodes with unmappable ids "Serge E. Hallyn" <serge@hallyn.com> - 2016-04-25 22:40 +0200
Re: [PATCH v3 14/21] fs: Allow superblock owner to change ownership of inodes with unmappable ids Seth Forshee <seth.forshee@canonical.com> - 2016-04-26 21:50 +0200
[PATCH v3 11/21] cred: Reject inodes with invalid ids in set_create_file_as() Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 19/21] fuse: Support fuse filesystems outside of init_user_ns Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 21/21] fuse: Allow user namespace mounts Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 20/21] fuse: Restrict allow_other to the superblock's namespace or a descendant Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 10/21] fs: Check for invalid i_uid in may_follow_link() Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 09/21] Smack: Handle labels consistently in untrusted mounts Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 18/21] fuse: Add support for pid namespaces Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 08/21] userns: Replace in_userns with current_in_userns Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 17:50 +0200
[PATCH v3 06/21] fs: Treat foreign mounts as nosuid Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 18:00 +0200
[PATCH v3 07/21] selinux: Add support for unprivileged mounts from user namespaces Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 18:00 +0200
[PATCH v3 03/21] fs: Allow sysfs and cgroupfs to share super blocks between user namespaces Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 18:00 +0200
Re: [PATCH v3 03/21] fs: Allow sysfs and cgroupfs to share super blocks between user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2016-04-25 21:10 +0200
[PATCH v3 02/21] fs: Remove check of s_user_ns for existing mounts in fs_fully_visible() Seth Forshee <seth.forshee@canonical.com> - 2016-04-22 18:00 +0200
csiph-web