linux-kernel - Re: [PATCH 17/19] fuse: Support fuse filesystems outside of init_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151204204122.GE147214@ubuntu-hedt>
Date:	Fri, 4 Dec 2015 14:41:22 -0600
From:	Seth Forshee <seth.forshee@...onical.com>
To:	"Serge E. Hallyn" <serge@...lyn.com>
Cc:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Miklos Szeredi <miklos@...redi.hu>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	Richard Weinberger <richard.weinberger@...il.com>,
	Austin S Hemmelgarn <ahferroin7@...il.com>,
	linux-bcache@...r.kernel.org, dm-devel@...hat.com,
	linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mtd@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
	fuse-devel@...ts.sourceforge.net,
	linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov
Subject: Re: [PATCH 17/19] fuse: Support fuse filesystems outside of
 init_user_ns

On Fri, Dec 04, 2015 at 02:03:55PM -0600, Serge E. Hallyn wrote:
> Quoting Seth Forshee (seth.forshee@...onical.com):
> > 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 the
> > userspace fuse process.
> > 
> > Currently no use cases are known for letting the userspace fuse
> > daemon switch namespaces after opening /dev/fuse. Given this
> > fact, and in order to keep the implementation as simple as
> > possible and ease security auditing, the user namespace from
> > which /dev/fuse is opened is used for all id translations. This
> > is required to be the same namespace as s_user_ns to maintain
> > behavior consistent with other filesystems which can be mounted
> > in user namespaces.
> > 
> > For cuse the namespace used for the connection is also simply
> > current_user_ns() at the time /dev/cuse is opened.
> > 
> > Signed-off-by: Seth Forshee <seth.forshee@...onical.com>
> > ---
> >  fs/fuse/cuse.c   |  3 +-
> >  fs/fuse/dev.c    | 13 +++++----
> >  fs/fuse/dir.c    | 81 ++++++++++++++++++++++++++++++++++-------------------
> >  fs/fuse/fuse_i.h | 14 ++++++----
> >  fs/fuse/inode.c  | 85 ++++++++++++++++++++++++++++++++++++++++++--------------
> >  5 files changed, 136 insertions(+), 60 deletions(-)
> > 
> > diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
> > index eae2c11268bc..a10aca57bfe4 100644
> > --- a/fs/fuse/cuse.c
> > +++ b/fs/fuse/cuse.c
> > @@ -48,6 +48,7 @@
> >  #include <linux/stat.h>
> >  #include <linux/module.h>
> >  #include <linux/uio.h>
> > +#include <linux/user_namespace.h>
> >  
> >  #include "fuse_i.h"
> >  
> > @@ -498,7 +499,7 @@ static int cuse_channel_open(struct inode *inode, struct file *file)
> >  	if (!cc)
> >  		return -ENOMEM;
> >  
> > -	fuse_conn_init(&cc->fc);
> > +	fuse_conn_init(&cc->fc, current_user_ns());
> >  
> >  	fud = fuse_dev_alloc(&cc->fc);
> >  	if (!fud) {
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index a4f6f30d6d86..11b4cb0a0e2f 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(fc->user_ns, current_fsuid());
> > +	req->in.h.gid = from_kgid(fc->user_ns, current_fsgid());
> >  	req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> >  }
> >  
> > @@ -186,7 +186,8 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
> >  	__set_bit(FR_WAITING, &req->flags);
> >  	if (for_background)
> >  		__set_bit(FR_BACKGROUND, &req->flags);
> > -	if (req->in.h.pid == 0) {
> > +	if (req->in.h.pid == 0 || req->in.h.uid == (uid_t)-1 ||
> > +	    req->in.h.gid == (gid_t)-1) {
> >  		fuse_put_request(fc, req);
> >  		return ERR_PTR(-EOVERFLOW);
> >  	}
> > @@ -1248,7 +1249,8 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
> >  	struct fuse_in *in;
> >  	unsigned reqsize;
> >  
> > -	if (task_active_pid_ns(current) != fc->pid_ns)
> > +	if (task_active_pid_ns(current) != fc->pid_ns ||
> > +	    current_user_ns() != fc->user_ns)
> 
> Do you think this should be using current_in_user_ns(fc->user_ns) ?
> 
> Opening a file, forking (maybe unsharing) then acting on the file is
> pretty standard fare which this would break.
> 
> (same for pidns, i guess, as well as for write below)

I'd rather leave it as is. It might be okay, but even if current_user_ns
is a child of fc->user_ns it could end up seeing ids that aren't valid
for it's namespaces, and that might cause issues for filesystems which
aren't expecting it.

But if you have a use case for a process in a child namespace servicing
requests for a mount, I'd be willing to reconsider.
--
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/