Quoting Eric W. Biederman (ebiederm@xxxxxxxxxxxx):
> Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> writes:
>
> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> >> Quoting Mimi Zohar (zohar@xxxxxxxxxxxxxxxxxx):
> >> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
