Quoting Seth Forshee (seth.fors...@canonical.com):
> Update fuse to support mounts from within user namespaces. This
> is mostly a matter of translating uids and gids into the
> namespace of the process reading requests before handing the
> requests off to userspace.
>
> Due to security concerns the namespace used should be fixed,
> otherwise a user might be able to pass the fuse fd to
> init_user_ns and inject suid files owned by a user outside the
> namespace in order to gain elevated privileges. For fuse we
> stash current_user_ns() when a filesystem is first mounted and
> abort the mount if this namespace is different than the one used
> to open the fd passed in the mount options.
>
> The allow_others options could also be a problem, as a userns
> mount could bypass system policy for this option and thus open
> the possiblity of DoS attacks. This is prevented by restricting
> the scope of allow_other to apply only to that superblock's
> userns and its children, giving the expected behavior within the
> userns while preventing DoS attacks on more privileged contexts.
>
> Signed-off-by: Seth Forshee <seth.fors...@canonical.com>
Thanks, Seth, just two little questions below.
> ---
> fs/fuse/dev.c | 4 ++--
> fs/fuse/dir.c | 46 +++++++++++++++++++++++++++++++++-------------
> fs/fuse/fuse_i.h | 4 ++++
> fs/fuse/inode.c | 28 ++++++++++++++++++++--------
> 4 files changed, 59 insertions(+), 23 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 839caebd34f1..03c8785ed731 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -127,8 +127,8 @@ static void __fuse_put_request(struct fuse_req *req)
>
> static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
> {
> - req->in.h.uid = from_kuid_munged(&init_user_ns, current_fsuid());
> - req->in.h.gid = from_kgid_munged(&init_user_ns, current_fsgid());
> + req->in.h.uid = from_kuid_munged(fc->user_ns, current_fsuid());
> + req->in.h.gid = from_kgid_munged(fc->user_ns, current_fsgid());
> req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> }
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index de1d84af9f7c..c0b9968db6a1 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -905,8 +905,8 @@ static void fuse_fillattr(struct inode *inode, struct
> fuse_attr *attr,
> stat->ino = attr->ino;
> stat->mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777);
> stat->nlink = attr->nlink;
> - stat->uid = make_kuid(&init_user_ns, attr->uid);
> - stat->gid = make_kgid(&init_user_ns, attr->gid);
> + stat->uid = make_kuid(fc->user_ns, attr->uid);
> + stat->gid = make_kgid(fc->user_ns, attr->gid);
> stat->rdev = inode->i_rdev;
> stat->atime.tv_sec = attr->atime;
> stat->atime.tv_nsec = attr->atimensec;
> @@ -1085,12 +1085,20 @@ int fuse_reverse_inval_entry(struct super_block *sb,
> u64 parent_nodeid,
> */
> int fuse_allow_current_process(struct fuse_conn *fc)
> {
> - const struct cred *cred;
> + const struct cred *cred = current_cred();
>
> - if (fc->flags & FUSE_ALLOW_OTHER)
> - return 1;
> + if (fc->flags & FUSE_ALLOW_OTHER) {
> + if (kuid_has_mapping(fc->user_ns, cred->euid) &&
> + kuid_has_mapping(fc->user_ns, cred->suid) &&
> + kuid_has_mapping(fc->user_ns, cred->uid) &&
> + kgid_has_mapping(fc->user_ns, cred->egid) &&
> + kgid_has_mapping(fc->user_ns, cred->sgid) &&
> + kgid_has_mapping(fc->user_ns, cred->gid))
Should fsuid be checked here?
> + return 1;
> +
> + return 0;
> + }
>
> - cred = current_cred();
> if (uid_eq(cred->euid, fc->user_id) &&
> uid_eq(cred->suid, fc->user_id) &&
> uid_eq(cred->uid, fc->user_id) &&
> @@ -1556,17 +1564,25 @@ static bool update_mtime(unsigned ivalid, bool
> trust_local_mtime)
> return true;
> }
>
> -static void iattr_to_fattr(struct iattr *iattr, struct fuse_setattr_in *arg,
> - bool trust_local_cmtime)
> +static int iattr_to_fattr(struct fuse_conn *fc, struct iattr *iattr,
> + struct fuse_setattr_in *arg, bool trust_local_cmtime)
> {
> unsigned ivalid = iattr->ia_valid;
>
> if (ivalid & ATTR_MODE)
> arg->valid |= FATTR_MODE, arg->mode = iattr->ia_mode;
> - if (ivalid & ATTR_UID)
> - arg->valid |= FATTR_UID, arg->uid = from_kuid(&init_user_ns,
> iattr->ia_uid);
> - if (ivalid & ATTR_GID)
> - arg->valid |= FATTR_GID, arg->gid = from_kgid(&init_user_ns,
> iattr->ia_gid);
> + if (ivalid & ATTR_UID) {
> + arg->uid = from_kuid(fc->user_ns, iattr->ia_uid);
> + if (arg->uid == (uid_t)-1)
Any reason not to use uid_valid() here (and gid_valid() below)?
> + return -EINVAL;
> + arg->valid |= FATTR_UID;
> + }
> + if (ivalid & ATTR_GID) {
> + arg->gid = from_kgid(fc->user_ns, iattr->ia_gid);
> + if (arg->gid == (gid_t)-1)
> + return -EINVAL;
> + arg->valid |= FATTR_GID;
> + }
> if (ivalid & ATTR_SIZE)
> arg->valid |= FATTR_SIZE, arg->size = iattr->ia_size;
> if (ivalid & ATTR_ATIME) {
> @@ -1588,6 +1604,8 @@ static void iattr_to_fattr(struct iattr *iattr, struct
> fuse_setattr_in *arg,
> arg->ctime = iattr->ia_ctime.tv_sec;
> arg->ctimensec = iattr->ia_ctime.tv_nsec;
> }
> +
> + return 0;
> }
>
> /*
> @@ -1741,7 +1759,9 @@ int fuse_do_setattr(struct inode *inode, struct iattr
> *attr,
>
> memset(&inarg, 0, sizeof(inarg));
> memset(&outarg, 0, sizeof(outarg));
> - iattr_to_fattr(attr, &inarg, trust_local_cmtime);
> + err = iattr_to_fattr(fc, attr, &inarg, trust_local_cmtime);
> + if (err)
> + goto error;
> if (file) {
> struct fuse_file *ff = file->private_data;
> inarg.valid |= FATTR_FH;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index a3ded071e2c6..2cfd0ca3407a 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -22,6 +22,7 @@
> #include <linux/rbtree.h>
> #include <linux/poll.h>
> #include <linux/workqueue.h>
> +#include <linux/user_namespace.h>
> #include <linux/pid_namespace.h>
>
> /** Max number of pages that can be used in a single read request */
> @@ -387,6 +388,9 @@ struct fuse_conn {
> /** The group id for this mount */
> kgid_t group_id;
>
> + /** The user namespace for this mount */
> + struct user_namespace *user_ns;
> +
> /** The pid namespace for this mount */
> struct pid_namespace *pid_ns;
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index c6d8473b1a80..f3a3ded82f85 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -167,8 +167,8 @@ void fuse_change_attributes_common(struct inode *inode,
> struct fuse_attr *attr,
> inode->i_ino = fuse_squash_ino(attr->ino);
> inode->i_mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777);
> set_nlink(inode, attr->nlink);
> - inode->i_uid = make_kuid(&init_user_ns, attr->uid);
> - inode->i_gid = make_kgid(&init_user_ns, attr->gid);
> + inode->i_uid = make_kuid(fc->user_ns, attr->uid);
> + inode->i_gid = make_kgid(fc->user_ns, attr->gid);
> inode->i_blocks = attr->blocks;
> inode->i_atime.tv_sec = attr->atime;
> inode->i_atime.tv_nsec = attr->atimensec;
> @@ -496,6 +496,8 @@ static int parse_fuse_opt(char *opt, struct
> fuse_mount_data *d, int is_bdev)
> memset(d, 0, sizeof(struct fuse_mount_data));
> d->max_read = ~0;
> d->blksize = FUSE_DEFAULT_BLKSIZE;
> + d->user_id = make_kuid(current_user_ns(), 0);
> + d->group_id = make_kgid(current_user_ns(), 0);
>
> while ((p = strsep(&opt, ",")) != NULL) {
> int token;
> @@ -578,8 +580,10 @@ static int fuse_show_options(struct seq_file *m, struct
> dentry *root)
> struct super_block *sb = root->d_sb;
> struct fuse_conn *fc = get_fuse_conn_super(sb);
>
> - seq_printf(m, ",user_id=%u", from_kuid_munged(&init_user_ns,
> fc->user_id));
> - seq_printf(m, ",group_id=%u", from_kgid_munged(&init_user_ns,
> fc->group_id));
> + seq_printf(m, ",user_id=%u",
> + from_kuid_munged(fc->user_ns, fc->user_id));
> + seq_printf(m, ",group_id=%u",
> + from_kgid_munged(fc->user_ns, fc->group_id));
> if (fc->flags & FUSE_DEFAULT_PERMISSIONS)
> seq_puts(m, ",default_permissions");
> if (fc->flags & FUSE_ALLOW_OTHER)
> @@ -618,6 +622,7 @@ void fuse_conn_init(struct fuse_conn *fc)
> fc->attr_version = 1;
> get_random_bytes(&fc->scramble_key, sizeof(fc->scramble_key));
> fc->pid_ns = get_pid_ns(task_active_pid_ns(current));
> + fc->user_ns = get_user_ns(current_user_ns());
> }
> EXPORT_SYMBOL_GPL(fuse_conn_init);
>
> @@ -956,6 +961,7 @@ static void fuse_send_init(struct fuse_conn *fc, struct
> fuse_req *req)
> static void fuse_free_conn(struct fuse_conn *fc)
> {
> put_pid_ns(fc->pid_ns);
> + put_user_ns(fc->user_ns);
> kfree_rcu(fc, rcu);
> }
>
> @@ -1042,8 +1048,14 @@ static int fuse_fill_super(struct super_block *sb,
> void *data, int silent)
> if (!file)
> goto err;
>
> - if ((file->f_op != &fuse_dev_operations) ||
> - (file->f_cred->user_ns != &init_user_ns))
> + /*
> + * Require mount to happen from the same user namespace which
> + * opened /dev/fuse, otherwise users might be able to
> + * elevate privileges by opening in init_user_ns then
> + * mounting from a different namespace without MNT_NOSUID.
> + */
> + if (file->f_op != &fuse_dev_operations ||
> + file->f_cred->user_ns != current_user_ns())
> goto err_fput;
>
> fc = kmalloc(sizeof(*fc), GFP_KERNEL);
> @@ -1157,7 +1169,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
> static struct file_system_type fuse_fs_type = {
> .owner = THIS_MODULE,
> .name = "fuse",
> - .fs_flags = FS_HAS_SUBTYPE,
> + .fs_flags = FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
> .mount = fuse_mount,
> .kill_sb = fuse_kill_sb_anon,
> };
> @@ -1189,7 +1201,7 @@ static struct file_system_type fuseblk_fs_type = {
> .name = "fuseblk",
> .mount = fuse_mount_blk,
> .kill_sb = fuse_kill_sb_blk,
> - .fs_flags = FS_REQUIRES_DEV | FS_HAS_SUBTYPE,
> + .fs_flags = FS_REQUIRES_DEV | FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
> };
> MODULE_ALIAS_FS("fuseblk");
>
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
