Message-ID: <543E8983.5090001@mit.edu>
Date:	Wed, 15 Oct 2014 07:49:39 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Seth Forshee <seth.forshee@...onical.com>,
	Miklos Szeredi <miklos@...redi.hu>
CC:	fuse-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"Serge H. Hallyn" <serge.hallyn@...ntu.com>
Subject: Re: [PATCH v4 2/5] fuse: Support fuse filesystems outside of init_user_ns

On 10/14/2014 07:25 AM, Seth Forshee wrote:
> Update fuse to translate uids and gids to/from the user namspace
> of the process servicing requests on /dev/fuse. Any ids which do
> not map into the namespace will result in errors. inodes will
> also be marked bad when unmappable ids are received from
> userspace.
> 
> Due to security concerns the namespace used should be fixed,
> otherwise a user might be able to gain elevated privileges or
> influence processes that the user would otherwise be unable to
> manipulate. Thus the namespace of the mounting process is used
> for all translations, and this namespace is required to be the
> same as the one in use when /dev/fuse was opened.
> 

I'm not sure that this is necessary if my nosuid patch goes in, but I
also don't think it makes any sense to hold this up while we find a
perfect solution.

Is there a decent way to extend this to different translation schemes in
the future (e.g. a flag at fs setup that could be used)?

--Andy

> Cc: Eric W. Biederman <ebiederm@...ssion.com>
> Cc: Serge H. Hallyn <serge.hallyn@...ntu.com>
> Signed-off-by: Seth Forshee <seth.forshee@...onical.com>
> ---
>  fs/fuse/dev.c    |  4 +--
>  fs/fuse/dir.c    | 81 ++++++++++++++++++++++++++++++++++++--------------------
>  fs/fuse/fuse_i.h | 12 ++++++---
>  fs/fuse/inode.c  | 73 +++++++++++++++++++++++++++++++++++++++-----------
>  4 files changed, 121 insertions(+), 49 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..123db1e06c78 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -253,9 +253,12 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
>  		if (err || (outarg.attr.mode ^ inode->i_mode) & S_IFMT)
>  			goto invalid;
>  
> -		fuse_change_attributes(inode, &outarg.attr,
> -				       entry_attr_timeout(&outarg),
> -				       attr_version);
> +		err = fuse_change_attributes(inode, &outarg.attr,
> +					     entry_attr_timeout(&outarg),
> +					     attr_version);
> +		if (err)
> +			goto invalid;
> +
>  		fuse_change_entry_timeout(entry, &outarg);
>  	} else if (inode) {
>  		fi = get_fuse_inode(inode);
> @@ -340,8 +343,9 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, struct qstr *name,
>  	*inode = fuse_iget(sb, outarg->nodeid, outarg->generation,
>  			   &outarg->attr, entry_attr_timeout(outarg),
>  			   attr_version);
> -	err = -ENOMEM;
> -	if (!*inode) {
> +	if (IS_ERR(*inode)) {
> +		err = PTR_ERR(*inode);
> +		*inode = NULL;
>  		fuse_queue_forget(fc, forget, outarg->nodeid, 1);
>  		goto out;
>  	}
> @@ -473,11 +477,11 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	ff->open_flags = outopen.open_flags;
>  	inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
>  			  &outentry.attr, entry_attr_timeout(&outentry), 0);
> -	if (!inode) {
> +	if (IS_ERR(inode)) {
>  		flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
>  		fuse_sync_release(ff, flags);
>  		fuse_queue_forget(fc, forget, outentry.nodeid, 1);
> -		err = -ENOMEM;
> +		err = PTR_ERR(inode);
>  		goto out_err;
>  	}
>  	kfree(forget);
> @@ -588,9 +592,9 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_req *req,
>  
>  	inode = fuse_iget(dir->i_sb, outarg.nodeid, outarg.generation,
>  			  &outarg.attr, entry_attr_timeout(&outarg), 0);
> -	if (!inode) {
> +	if (IS_ERR(inode)) {
>  		fuse_queue_forget(fc, forget, outarg.nodeid, 1);
> -		return -ENOMEM;
> +		return PTR_ERR(inode);
>  	}
>  	kfree(forget);
>  
> @@ -905,8 +909,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 = inode->i_uid;
> +	stat->gid = inode->i_gid;
>  	stat->rdev = inode->i_rdev;
>  	stat->atime.tv_sec = attr->atime;
>  	stat->atime.tv_nsec = attr->atimensec;
> @@ -969,10 +973,10 @@ static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
>  			make_bad_inode(inode);
>  			err = -EIO;
>  		} else {
> -			fuse_change_attributes(inode, &outarg.attr,
> -					       attr_timeout(&outarg),
> -					       attr_version);
> -			if (stat)
> +			err = fuse_change_attributes(inode, &outarg.attr,
> +						     attr_timeout(&outarg),
> +						     attr_version);
> +			if (!err && stat)
>  				fuse_fillattr(inode, &outarg.attr, stat);
>  		}
>  	}
> @@ -1302,9 +1306,11 @@ static int fuse_direntplus_link(struct file *file,
>  			fi->nlookup++;
>  			spin_unlock(&fc->lock);
>  
> -			fuse_change_attributes(inode, &o->attr,
> -					       entry_attr_timeout(o),
> -					       attr_version);
> +			err = fuse_change_attributes(inode, &o->attr,
> +						     entry_attr_timeout(o),
> +						     attr_version);
> +			if (err)
> +				goto out;
>  
>  			/*
>  			 * The other branch to 'found' comes via fuse_iget()
> @@ -1322,8 +1328,10 @@ static int fuse_direntplus_link(struct file *file,
>  
>  	inode = fuse_iget(dir->i_sb, o->nodeid, o->generation,
>  			  &o->attr, entry_attr_timeout(o), attr_version);
> -	if (!inode)
> +	if (IS_ERR(inode)) {
> +		err = PTR_ERR(inode);
>  		goto out;
> +	}
>  
>  	alias = d_materialise_unique(dentry, inode);
>  	err = PTR_ERR(alias);
> @@ -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)
> +			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;
> @@ -1778,8 +1798,13 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>  		/* FIXME: clear I_DIRTY_SYNC? */
>  	}
>  
> -	fuse_change_attributes_common(inode, &outarg.attr,
> -				      attr_timeout(&outarg));
> +	err = fuse_change_attributes_common(inode, &outarg.attr,
> +				            attr_timeout(&outarg));
> +	if (err) {
> +		spin_unlock(&fc->lock);
> +		goto error;
> +	}
> +
>  	oldsize = inode->i_size;
>  	/* see the comment in fuse_change_attributes() */
>  	if (!is_wb || is_truncate || !S_ISREG(inode->i_mode))
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index a3ded071e2c6..81187ba04e4a 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;
>  
> @@ -713,11 +717,11 @@ void fuse_init_symlink(struct inode *inode);
>  /**
>   * Change attributes of an inode
>   */
> -void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
> -			    u64 attr_valid, u64 attr_version);
> +int fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
> +			   u64 attr_valid, u64 attr_version);
>  
> -void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
> -				   u64 attr_valid);
> +int fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
> +				  u64 attr_valid);
>  
>  /**
>   * Initialize the client device
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index e137969815a3..b88b5a780228 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -155,11 +155,22 @@ static ino_t fuse_squash_ino(u64 ino64)
>  	return ino;
>  }
>  
> -void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
> -				   u64 attr_valid)
> +int fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
> +				  u64 attr_valid)
>  {
>  	struct fuse_conn *fc = get_fuse_conn(inode);
>  	struct fuse_inode *fi = get_fuse_inode(inode);
> +	kuid_t uid;
> +	kgid_t gid;
> +
> +	uid = make_kuid(fc->user_ns, attr->uid);
> +	gid = make_kgid(fc->user_ns, attr->gid);
> +	if (!uid_valid(uid) || !gid_valid(gid)) {
> +		make_bad_inode(inode);
> +		return -EIO;
> +	}
> +	inode->i_uid = uid;
> +	inode->i_gid = gid;
>  
>  	fi->attr_version = ++fc->attr_version;
>  	fi->i_time = attr_valid;
> @@ -167,8 +178,6 @@ 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_blocks  = attr->blocks;
>  	inode->i_atime.tv_sec   = attr->atime;
>  	inode->i_atime.tv_nsec  = attr->atimensec;
> @@ -195,26 +204,32 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
>  		inode->i_mode &= ~S_ISVTX;
>  
>  	fi->orig_ino = attr->ino;
> +	return 0;
>  }
>  
> -void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
> -			    u64 attr_valid, u64 attr_version)
> +int fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
> +			   u64 attr_valid, u64 attr_version)
>  {
>  	struct fuse_conn *fc = get_fuse_conn(inode);
>  	struct fuse_inode *fi = get_fuse_inode(inode);
>  	bool is_wb = fc->writeback_cache;
>  	loff_t oldsize;
>  	struct timespec old_mtime;
> +	int err;
>  
>  	spin_lock(&fc->lock);
>  	if ((attr_version != 0 && fi->attr_version > attr_version) ||
>  	    test_bit(FUSE_I_SIZE_UNSTABLE, &fi->state)) {
>  		spin_unlock(&fc->lock);
> -		return;
> +		return 0;
>  	}
>  
>  	old_mtime = inode->i_mtime;
> -	fuse_change_attributes_common(inode, attr, attr_valid);
> +	err = fuse_change_attributes_common(inode, attr, attr_valid);
> +	if (err) {
> +		spin_unlock(&fc->lock);
> +		return err;
> +	}
>  
>  	oldsize = inode->i_size;
>  	/*
> @@ -249,6 +264,8 @@ void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
>  		if (inval)
>  			invalidate_inode_pages2(inode->i_mapping);
>  	}
> +
> +	return 0;
>  }
>  
>  static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
> @@ -302,7 +319,7 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>   retry:
>  	inode = iget5_locked(sb, nodeid, fuse_inode_eq, fuse_inode_set, &nodeid);
>  	if (!inode)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	if ((inode->i_state & I_NEW)) {
>  		inode->i_flags |= S_NOATIME;
> @@ -319,11 +336,23 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>  		goto retry;
>  	}
>  
> +	/*
> +	 * Must do this before incrementing nlookup, as the caller will
> +	 * send a forget for the node if this function fails.
> +	 */
> +	if (fuse_change_attributes(inode, attr, attr_valid, attr_version)) {
> +		/*
> +		 * inode_make_bad() already called by
> +		 * fuse_change_attributes()
> +		 */
> +		iput(inode);
> +		return ERR_PTR(-EIO);
> +	}
> +
>  	fi = get_fuse_inode(inode);
>  	spin_lock(&fc->lock);
>  	fi->nlookup++;
>  	spin_unlock(&fc->lock);
> -	fuse_change_attributes(inode, attr, attr_valid, attr_version);
>  
>  	return inode;
>  }
> @@ -496,6 +525,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 +609,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 +651,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);
>  
> @@ -626,6 +660,8 @@ void fuse_conn_put(struct fuse_conn *fc)
>  	if (atomic_dec_and_test(&fc->count)) {
>  		if (fc->destroy_req)
>  			fuse_request_free(fc->destroy_req);
> +		put_user_ns(fc->user_ns);
> +		fc->user_ns = NULL;
>  		put_pid_ns(fc->pid_ns);
>  		fc->pid_ns = NULL;
>  		fc->release(fc);
> @@ -643,12 +679,15 @@ EXPORT_SYMBOL_GPL(fuse_conn_get);
>  static struct inode *fuse_get_root_inode(struct super_block *sb, unsigned mode)
>  {
>  	struct fuse_attr attr;
> +	struct inode *inode;
> +
>  	memset(&attr, 0, sizeof(attr));
>  
>  	attr.mode = mode;
>  	attr.ino = FUSE_ROOT_ID;
>  	attr.nlink = 1;
> -	return fuse_iget(sb, 1, 0, &attr, 0, 0);
> +	inode = fuse_iget(sb, 1, 0, &attr, 0, 0);
> +	return IS_ERR(inode) ? NULL : inode;
>  }
>  
>  struct fuse_inode_handle {
> @@ -1043,8 +1082,12 @@ 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 to prevent potential attacks.
> +	 */
> +	if (file->f_op != &fuse_dev_operations ||
> +	    file->f_cred->user_ns != current_user_ns())
>  		goto err_fput;
>  
>  	fc = kmalloc(sizeof(*fc), GFP_KERNEL);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists