Re: [PATCH v2 0/3] fuse: Add support for mounts from pid/user namespaces

[Seth Forshee <seth.forshee@canonical.com>]

On Wed, Sep 10, 2014 at 06:21:55PM +0200, Serge E. Hallyn wrote:
> Quoting Seth Forshee (seth.forshee@canonical.com):
> > On Tue, Sep 02, 2014 at 10:44:53AM -0500, Seth Forshee wrote:
> > > Another issue mentioned by Eric was what to use for i_[ug]id if the ids
> > > from userspace don't map into the user namespace, which is going to be a
> > > problem for any other filesystems which become mountable from user
> > > namespaces as well. We discussed a few options for addressing this, the
> > > most promising of which seems to be either using INVALID_[UG]ID for
> > > these inodes or creating vfs-wide "nobody" ids for this purpose. After
> > > thinking about it for a while I'm favoring using the invalid ids, but
> > > I'm hoping to solicit some more feedback.
> > > 
> > > For now these patches are using invalid ids if the user doesn't map into
> > > the namespace. I went through the vfs code and found one place where
> > > this could be handled better (addressed in patch 1 of the series). The
> > > only other issue I found was that currently no one, not even root, can
> > > change onwership of such inodes, but I suspect we can find a way around
> > > this.
> > 
> > I started playing around with using -2 as a global nobody id. The
> > changes below (made on top of this series) are working fine in light
> > testing. I'm still not sure about the security implications of doing
> > this though. Possibly the nobody id should be removed from init_user_ns
> > to prevent any use of the id to gain unauthroized access to such files.
> > Thoughts?
> 
> Can you explain the downsides of just using -1?  What are we able to do
> (as a fuse-in-container user) when we use -2 that we can't do when it
> uses -1?

The thing that happens with -1 is that checks like
capable_wrt_inode_uidgid() always fail on those inodes because
INVALID_UID isn't ever mapped into any namespace, even if you're
system-wide root. If we use a real id this check would at least pass in
init_user_ns, assuming that we don't have to remove -2 from init_user_ns
for security reasons.

Or we could modify these checks so that CAP_SYS_ADMIN always gets
permission or something like that, which might be the better way to go.

> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index c0b9968db6a1..893fc0d6ff96 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(fc->user_ns, attr->uid);
> > -	stat->gid = make_kgid(fc->user_ns, attr->gid);
> > +	stat->uid = make_kuid_munged(fc->user_ns, attr->uid);
> > +	stat->gid = make_kgid_munged(fc->user_ns, attr->gid);
> >  	stat->rdev = inode->i_rdev;
> >  	stat->atime.tv_sec = attr->atime;
> >  	stat->atime.tv_nsec = attr->atimensec;
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index f3a3ded82f85..330ac3d502a6 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(fc->user_ns, attr->uid);
> > -	inode->i_gid     = make_kgid(fc->user_ns, attr->gid);
> > +	inode->i_uid     = make_kuid_munged(fc->user_ns, attr->uid);
> > +	inode->i_gid     = make_kgid_munged(fc->user_ns, attr->gid);
> >  	inode->i_blocks  = attr->blocks;
> >  	inode->i_atime.tv_sec   = attr->atime;
> >  	inode->i_atime.tv_nsec  = attr->atimensec;
> > diff --git a/include/linux/uidgid.h b/include/linux/uidgid.h
> > index 6c302267f7cc..9054273af163 100644
> > --- a/include/linux/uidgid.h
> > +++ b/include/linux/uidgid.h
> > @@ -45,6 +45,9 @@ static inline gid_t __kgid_val(kgid_t gid)
> >  #define INVALID_UID KUIDT_INIT(-1)
> >  #define INVALID_GID KGIDT_INIT(-1)
> >  
> > +#define NOBODY_UID KUIDT_INIT(-2)
> > +#define NOBODY_GID KGIDT_INIT(-2)
> > +
> >  static inline bool uid_eq(kuid_t left, kuid_t right)
> >  {
> >  	return __kuid_val(left) == __kuid_val(right);
> > @@ -175,4 +178,44 @@ static inline bool kgid_has_mapping(struct user_namespace *ns, kgid_t gid)
> >  
> >  #endif /* CONFIG_USER_NS */
> >  
> > +/**
> > + *	make_kuid_munged - Map a user-namespace uid pair into a kuid
> > + * 	@from: User namespace that the uid is in
> > + * 	@uid:  User identifier
> > + *
> > + *	Maps a user-namespace uid pair into a kernel internal kuid,
> > + *	and returns that kuid.
> > + *
> > + *	Unlike make_kuid, make_kuid_munged never fails and always
> > + *	returns a valid uid. If @uid has no mapping in @from
> > + *	NOBODY_UID is returned.
> > + */
> > +static inline kuid_t make_kuid_munged(struct user_namespace *from, uid_t uid)
> > +{
> > +	kuid_t kuid = make_kuid(from, uid);
> > +	if (!uid_valid(kuid))
> > +		kuid = NOBODY_UID;
> > +	return kuid;
> > +}
> > +
> > +/**
> > + *	make_kgid_munged - Map a user-namespace gid pair into a kgid
> > + * 	@from: User namespace that the gid is in
> > + * 	@gid:  User identifier
> > + *
> > + *	Maps a user-namespace gid pair into a kernel internal kgid,
> > + *	and returns that kgid.
> > + *
> > + *	Unlike make_kgid, make_kgid_munged never fails and always
> > + *	returns a valid gid. If @gid has no mapping in @from
> > + *	NOBODY_GID is returned.
> > + */
> > +static inline kgid_t make_kgid_munged(struct user_namespace *from, gid_t gid)
> > +{
> > +	kgid_t kgid = make_kgid(from, gid);
> > +	if (!gid_valid(kgid))
> > +		kgid = NOBODY_GID;
> > +	return kgid;
> > +}
> > +
> >  #endif /* _LINUX_UIDGID_H */
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/