Quoting Eric W. Biederman (ebied...@xmission.com):
> "Serge E. Hallyn" <se...@hallyn.com> writes:
>
> > Quoting Eric W. Biederman (ebied...@xmission.com):
> >> Mimi Zohar <zo...@linux.vnet.ibm.com> writes:
> >>
> >> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> >> >> Quoting Mimi Zohar (zo...@linux.vnet.ibm.com):
> >> >> > 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?
>
> Yes.
>
> That would seem to simplify the logic to ensure the policy we enforce is
> consistent with what is on disk.
I'll give that a shot. I think the reason I did it this way was that I'm
still kind of stuck in the not-magic way of thinking about it. But yeah
with the kernel magically writing inthe kuid there's probably no reason not
to.
