"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.
Eric
