Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

["Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:
> 
> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> >> Quoting Mimi Zohar (zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> >
> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
> >> > > index 4861322..5c0e7ae 100644
> >> > > --- a/fs/xattr.c
> >> > > +++ b/fs/xattr.c
> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> >> > >  {
> >> > >  	struct inode *inode = dentry->d_inode;
> >> > >  	int error = -EOPNOTSUPP;
> >> > > +	void *wvalue = NULL;
> >> > > +	size_t wsize = 0;
> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
> >> > > 
> >> > > -	if (issec)
> >> > > +	if (issec) {
> >> > >  		inode->i_flags &= ~S_NOSEC;
> >> > > +		/* if root in a non-init user_ns tries to set
> >> > > +		 * security.capability, write a security.nscapability
> >> > > +		 * in its place */
> >> > > +		if (!strcmp(name, "security.capability") &&
> >> > > +				current_user_ns() != &init_user_ns) {
> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> >> > > +			if (!wvalue)
> >> > > +				return -EPERM;
> >> > > +			value = wvalue;
> >> > > +			size = wsize;
> >> > > +			name = "security.nscapability";
> >> > > +		}
> >> > 
> >> > The call to capable_wrt_inode_uidgid() is hidden behind
> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> >> > before the security.capability test?  This would lay the foundation for
> >> > doing something similar for IMA.
> >> 
> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
> >> 
> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
> >> 			return -EPERM;
> >> 
> >> would be cleaner.
> >
> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
> > making it generic.  Then the rest of us can follow your lead.  Its more
> > likely that you'll get it right.  At a high level, it might look like:
> >
> >                /* Permit root in a non-init user_ns to modify the security
> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
> >                  */
> >                 if ((current_user_ns() != &init_user_ns) &&
> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
> >
> > 			if  security..capability
> > 				call capability  /* set nscapability? */
> >
> > 			else if security.ima 
> > 				call ima 	/* set ns_ima? */
> > 		}
> 
> Hmm.  I am confused about this part of the strategy.
> 
> I don't understand the capability vs nscapability distinction.  It seems
> to add complexity without benefit.

...  Well, yes, we could simply make a new version of security.capability
xattr, and make rootid == 0 mean it was written by the init_user_ns.  Is
that what you mean?

-serge