On Fri, 2016-05-20 at 14:59 -0500, Serge E. Hallyn wrote:
> 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.
Totally confused. Will this method allow multiple instances of the
xattr on disk?
Mimi
