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

[Serge E. Hallyn]

Quoting Mimi Zohar (zo...@linux.vnet.ibm.com):
> On Fri, 2016-05-20 at 14:59 -0500, Serge E. Hallyn wrote:
> > Quoting Eric W. Biederman (ebied...@xmission.com):
> > > "Serge E. Hallyn"  writes:
> > > 
> > > > Quoting Eric W. Biederman (ebied...@xmission.com):
> > > >> Mimi Zohar  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() != 
> > > >> >> > > _user_ns) {
> > > >> >> > > +   cap_setxattr_make_nscap(dentry, value, 
> > > >> >> > > size, , );
> > > >> >> > > +   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, , , 
> > > >> >> ))
> > > >> >> 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() != _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? 

No, but we don't actually want that anyway.  The current behavior for
security.capability is that it works in all user namespaces.  So we
want to continue the 

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

[Serge E. Hallyn]

Quoting Mimi Zohar (zo...@linux.vnet.ibm.com):
> On Fri, 2016-05-20 at 14:59 -0500, Serge E. Hallyn wrote:
> > Quoting Eric W. Biederman (ebied...@xmission.com):
> > > "Serge E. Hallyn"  writes:
> > > 
> > > > Quoting Eric W. Biederman (ebied...@xmission.com):
> > > >> Mimi Zohar  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() != 
> > > >> >> > > _user_ns) {
> > > >> >> > > +   cap_setxattr_make_nscap(dentry, value, 
> > > >> >> > > size, , );
> > > >> >> > > +   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, , , 
> > > >> >> ))
> > > >> >> 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() != _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? 

No, but we don't actually want that anyway.  The current behavior for
security.capability is that it works in all user namespaces.  So we
want to continue the behavior that if root in the init_user_ns sets a

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

[Mimi Zohar]

On Fri, 2016-05-20 at 14:59 -0500, Serge E. Hallyn wrote:
> Quoting Eric W. Biederman (ebied...@xmission.com):
> > "Serge E. Hallyn"  writes:
> > 
> > > Quoting Eric W. Biederman (ebied...@xmission.com):
> > >> Mimi Zohar  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() != _user_ns) {
> > >> >> > > + cap_setxattr_make_nscap(dentry, value, size, 
> > >> >> > > , );
> > >> >> > > + 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, , , 
> > >> >> ))
> > >> >>   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() != _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

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

[Mimi Zohar]

On Fri, 2016-05-20 at 14:59 -0500, Serge E. Hallyn wrote:
> Quoting Eric W. Biederman (ebied...@xmission.com):
> > "Serge E. Hallyn"  writes:
> > 
> > > Quoting Eric W. Biederman (ebied...@xmission.com):
> > >> Mimi Zohar  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() != _user_ns) {
> > >> >> > > + cap_setxattr_make_nscap(dentry, value, size, 
> > >> >> > > , );
> > >> >> > > + 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, , , 
> > >> >> ))
> > >> >>   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() != _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

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

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebied...@xmission.com):
> "Serge E. Hallyn"  writes:
> 
> > Quoting Eric W. Biederman (ebied...@xmission.com):
> >> Mimi Zohar  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() != _user_ns) {
> >> >> > > +   cap_setxattr_make_nscap(dentry, value, size, 
> >> >> > > , );
> >> >> > > +   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, , , 
> >> >> ))
> >> >> 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() != _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.

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

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebied...@xmission.com):
> "Serge E. Hallyn"  writes:
> 
> > Quoting Eric W. Biederman (ebied...@xmission.com):
> >> Mimi Zohar  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() != _user_ns) {
> >> >> > > +   cap_setxattr_make_nscap(dentry, value, size, 
> >> >> > > , );
> >> >> > > +   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, , , 
> >> >> ))
> >> >> 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() != _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.

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

[Eric W. Biederman]

"Serge E. Hallyn"  writes:

> Quoting Eric W. Biederman (ebied...@xmission.com):
>> Mimi Zohar  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() != _user_ns) {
>> >> > > + cap_setxattr_make_nscap(dentry, value, size, 
>> >> > > , );
>> >> > > + 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, , , ))
>> >>   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() != _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

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

[Eric W. Biederman]

"Serge E. Hallyn"  writes:

> Quoting Eric W. Biederman (ebied...@xmission.com):
>> Mimi Zohar  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() != _user_ns) {
>> >> > > + cap_setxattr_make_nscap(dentry, value, size, 
>> >> > > , );
>> >> > > + 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, , , ))
>> >>   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() != _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

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

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebied...@xmission.com):
> Mimi Zohar  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() != _user_ns) {
> >> > > +  cap_setxattr_make_nscap(dentry, value, size, 
> >> > > , );
> >> > > +  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, , , ))
> >>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() != _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

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

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebied...@xmission.com):
> Mimi Zohar  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() != _user_ns) {
> >> > > +  cap_setxattr_make_nscap(dentry, value, size, 
> >> > > , );
> >> > > +  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, , , ))
> >>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() != _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

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

[Eric W. Biederman]

Mimi Zohar  writes:

> On Fri, 2016-05-20 at 13:28 -0500, Eric W. Biederman wrote:
>> Mimi Zohar  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() != _user_ns) {
>> >> > > + cap_setxattr_make_nscap(dentry, value, size, 
>> >> > > , );
>> >> > > + 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, , , ))
>> >>   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() != _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.
>
> Only real root can write security xattrs, which prevents root in a
> namespace from writing the included security xattrs in a tar package
> from being installed.  Nobody is suggesting changing this behavior.
> Serge's solution is to allow an equivalent xattr to be written.  For
> capabilities, it would be ns_capability.   Similarly, IMA would write
> security.ns_ima.  (I'm not sure about SELinux or Smack.)

nscapability is an xattr in the security namespace.  So root in a
namespace can write to a specific security xattr.

I am saying that at least at a quick examinination that having two
attributes that control the same things appears to be a mistake, as it
just makes it easy for them to get out of sync and generally be
confusing.

>> If I am in a nested user namespace and I try to write a capability on a
>> file and it has nscapability I can't be allowed to (as a more privileged
>> user namespace already wrote it).
>> 
>> Not rewriting an existing attribute seems to be the only benefit I can
>> see to have both a capability and a nscapability attribute vs having
>> a new version of the capability attribute.
>> 
>> Am I missing something here?
>
> Only real root in the namespace can write the equivalent security xattr.
> I'm hoping this can be done without having to modify userspace apps.  (I
> hope that answers your question.)

But it is possible and actually desirable to have user namespaces nested
in user namespaces nested in user namespaces.  Which means the existing
xattr must be examined before being written.

At which point I don't see a gain by having both a capability and an
nscapability attribute.

>> Mimi as for 

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

[Eric W. Biederman]

Mimi Zohar  writes:

> On Fri, 2016-05-20 at 13:28 -0500, Eric W. Biederman wrote:
>> Mimi Zohar  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() != _user_ns) {
>> >> > > + cap_setxattr_make_nscap(dentry, value, size, 
>> >> > > , );
>> >> > > + 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, , , ))
>> >>   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() != _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.
>
> Only real root can write security xattrs, which prevents root in a
> namespace from writing the included security xattrs in a tar package
> from being installed.  Nobody is suggesting changing this behavior.
> Serge's solution is to allow an equivalent xattr to be written.  For
> capabilities, it would be ns_capability.   Similarly, IMA would write
> security.ns_ima.  (I'm not sure about SELinux or Smack.)

nscapability is an xattr in the security namespace.  So root in a
namespace can write to a specific security xattr.

I am saying that at least at a quick examinination that having two
attributes that control the same things appears to be a mistake, as it
just makes it easy for them to get out of sync and generally be
confusing.

>> If I am in a nested user namespace and I try to write a capability on a
>> file and it has nscapability I can't be allowed to (as a more privileged
>> user namespace already wrote it).
>> 
>> Not rewriting an existing attribute seems to be the only benefit I can
>> see to have both a capability and a nscapability attribute vs having
>> a new version of the capability attribute.
>> 
>> Am I missing something here?
>
> Only real root in the namespace can write the equivalent security xattr.
> I'm hoping this can be done without having to modify userspace apps.  (I
> hope that answers your question.)

But it is possible and actually desirable to have user namespaces nested
in user namespaces nested in user namespaces.  Which means the existing
xattr must be examined before being written.

At which point I don't see a gain by having both a capability and an
nscapability attribute.

>> Mimi as for generalizing the code for handling IMA I expect it makes
>> 

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

[Mimi Zohar]

On Fri, 2016-05-20 at 13:28 -0500, Eric W. Biederman wrote:
> Mimi Zohar  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() != _user_ns) {
> >> > > +  cap_setxattr_make_nscap(dentry, value, size, 
> >> > > , );
> >> > > +  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, , , ))
> >>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() != _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.

Only real root can write security xattrs, which prevents root in a
namespace from writing the included security xattrs in a tar package
from being installed.  Nobody is suggesting changing this behavior.
Serge's solution is to allow an equivalent xattr to be written.  For
capabilities, it would be ns_capability.   Similarly, IMA would write
security.ns_ima.  (I'm not sure about SELinux or Smack.)

> If I am in a nested user namespace and I try to write a capability on a
> file and it has nscapability I can't be allowed to (as a more privileged
> user namespace already wrote it).
> 
> Not rewriting an existing attribute seems to be the only benefit I can
> see to have both a capability and a nscapability attribute vs having
> a new version of the capability attribute.
> 
> Am I missing something here?

Only real root in the namespace can write the equivalent security xattr.
I'm hoping this can be done without having to modify userspace apps.  (I
hope that answers your question.)

> Mimi as for generalizing the code for handling IMA I expect it makes
> sense to refactor the code to have shared library functions (or whatever
> it takes to generalize the code) when you are ready to implement this
> kind of IMA attribute.  That way in the first implementation we can
> concentrate on getting the code clean and the sematics correct.
> 
> That alone seems to be taking a while.

There should be a generic solution that works for security xattrs, not
just capabilities.

Mimi

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

[Mimi Zohar]

On Fri, 2016-05-20 at 13:28 -0500, Eric W. Biederman wrote:
> Mimi Zohar  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() != _user_ns) {
> >> > > +  cap_setxattr_make_nscap(dentry, value, size, 
> >> > > , );
> >> > > +  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, , , ))
> >>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() != _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.

Only real root can write security xattrs, which prevents root in a
namespace from writing the included security xattrs in a tar package
from being installed.  Nobody is suggesting changing this behavior.
Serge's solution is to allow an equivalent xattr to be written.  For
capabilities, it would be ns_capability.   Similarly, IMA would write
security.ns_ima.  (I'm not sure about SELinux or Smack.)

> If I am in a nested user namespace and I try to write a capability on a
> file and it has nscapability I can't be allowed to (as a more privileged
> user namespace already wrote it).
> 
> Not rewriting an existing attribute seems to be the only benefit I can
> see to have both a capability and a nscapability attribute vs having
> a new version of the capability attribute.
> 
> Am I missing something here?

Only real root in the namespace can write the equivalent security xattr.
I'm hoping this can be done without having to modify userspace apps.  (I
hope that answers your question.)

> Mimi as for generalizing the code for handling IMA I expect it makes
> sense to refactor the code to have shared library functions (or whatever
> it takes to generalize the code) when you are ready to implement this
> kind of IMA attribute.  That way in the first implementation we can
> concentrate on getting the code clean and the sematics correct.
> 
> That alone seems to be taking a while.

There should be a generic solution that works for security xattrs, not
just capabilities.

Mimi

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

[Eric W. Biederman]

Mimi Zohar  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() != _user_ns) {
>> > > +cap_setxattr_make_nscap(dentry, value, size, 
>> > > , );
>> > > +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, , , ))
>>  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() != _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.

If I am in a nested user namespace and I try to write a capability on a
file and it has nscapability I can't be allowed to (as a more privileged
user namespace already wrote it).

Not rewriting an existing attribute seems to be the only benefit I can
see to have both a capability and a nscapability attribute vs having
a new version of the capability attribute.

Am I missing something here?

Mimi as for generalizing the code for handling IMA I expect it makes
sense to refactor the code to have shared library functions (or whatever
it takes to generalize the code) when you are ready to implement this
kind of IMA attribute.  That way in the first implementation we can
concentrate on getting the code clean and the sematics correct.

That alone seems to be taking a while.

Eric

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

[Eric W. Biederman]

Mimi Zohar  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() != _user_ns) {
>> > > +cap_setxattr_make_nscap(dentry, value, size, 
>> > > , );
>> > > +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, , , ))
>>  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() != _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.

If I am in a nested user namespace and I try to write a capability on a
file and it has nscapability I can't be allowed to (as a more privileged
user namespace already wrote it).

Not rewriting an existing attribute seems to be the only benefit I can
see to have both a capability and a nscapability attribute vs having
a new version of the capability attribute.

Am I missing something here?

Mimi as for generalizing the code for handling IMA I expect it makes
sense to refactor the code to have shared library functions (or whatever
it takes to generalize the code) when you are ready to implement this
kind of IMA attribute.  That way in the first implementation we can
concentrate on getting the code clean and the sematics correct.

That alone seems to be taking a while.

Eric

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

[Mimi Zohar]

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() != _user_ns) {
> > > + cap_setxattr_make_nscap(dentry, value, size, , 
> > > );
> > > + 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, , , ))
>   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() != _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? */
}

Mimi

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

[Mimi Zohar]

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() != _user_ns) {
> > > + cap_setxattr_make_nscap(dentry, value, size, , 
> > > );
> > > + 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, , , ))
>   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() != _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? */
}

Mimi

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

[Serge E. Hallyn]

Quoting Mimi Zohar (zo...@linux.vnet.ibm.com):
> On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> > This patch introduces a new security.nscapability xattr.  It
> > is mostly like security.capability, but also lists a 'rootid'.
> > This is the uid_t (in init_user_ns) of the root id (uid 0 in a
> > namespace) in whose namespaces the file capabilities may take
> > effect.
> > 
> > A privileged (cap_setfcap) process in the initial user ns may
> > set and read this xattr directly.  However, its real intent is
> > to be used as a transparent fallback in user namespaces.
> > 
> > Root in a user ns cannot be trusted to write security.capability
> > xattrs, because any user on the host could map his own uid to root
> > in a namespace, write the xattr, and execute the file with privilege
> > on the host.
> > 
> > With this patch, when root in a user ns asks to write security.capability,
> > the kernel will transparently write a security.nscapability xattr
> > instead, filling in the kuid of the calling user's root uid.  Subsequently,
> > any task executing the file which has the noted k_uid as its root uid,
> > or which is in a descendent user_ns of such a user_ns, will run the
> > file with capabilities.
> > 
> > When reading the security.capability xattr from a non-init user_ns, a valid
> > security.nscapability will be shown if it exists.  Such a task is not
> > allowed to read security.nscapability.  This could be accomodated, however
> 
> Add the word "directly" as "to read security.nscapability directly".

Updated in my git tree.

> > it requires the kernel to convert the kuid_t to a valid uid in the reader's
> > user_ns.  So for now it's simply not supported.
> 
> I really like the idea that the kernel transparently replaces
> nscapability for capability.
> 
> > Only a single security.nscapability xattr may be written.  This patch
> > could be expanded to allow a list of capabilities and rootids, however
> > I do not believe that to be a worthwhile use case.
> 
> Ok
> 
> > This allows a simple setxattr to work, allows tar/untar to
> > work, and allows us to tar in one namespace and untar in
> > another while preserving the capability, without risking
> > leaking privilege into a parent namespace.
> > 
> > Note - listxattr is not being handled here.  So results of that can be
> > inconsistent with get/setxattr.  Fixing that will require yet more
> > deceit in fs/xattr.c.
> > 
> > Note2 - it may be less sneaky to hide all the magic behind the
> > security.nscapability xattr.  So userspace would need to know to
> > use that xattr name when needed, but with the same format as
> > security.capability.  The kuid_t rootid would be filled in by the
> > kernel.  That's a middle ground between my last patch and this one.
> 
> The less userspace needs to differentiate between running in a namespace
> and not, the better.
> 
> Note3 - capability is currently protected by EVM, when enabled.  Should
> ns_capability also be a protected xattr?

Hm - that would protect it from offline attacks, but allow the container
to update it, right?  That sounds good.

> > Signed-off-by: Serge Hallyn 
> > ---
> >  fs/xattr.c  |  18 ++-
> >  include/linux/capability.h  |   8 +-
> >  include/uapi/linux/capability.h |  19 +++
> >  include/uapi/linux/xattr.h  |   3 +
> >  security/commoncap.c| 253 
> > ++--
> >  5 files changed, 291 insertions(+), 10 deletions(-)
> > 
> > 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() != _user_ns) {
> > +   cap_setxattr_make_nscap(dentry, value, size, , 
> > );
> > +   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 

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

[Serge E. Hallyn]

Quoting Mimi Zohar (zo...@linux.vnet.ibm.com):
> On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> > This patch introduces a new security.nscapability xattr.  It
> > is mostly like security.capability, but also lists a 'rootid'.
> > This is the uid_t (in init_user_ns) of the root id (uid 0 in a
> > namespace) in whose namespaces the file capabilities may take
> > effect.
> > 
> > A privileged (cap_setfcap) process in the initial user ns may
> > set and read this xattr directly.  However, its real intent is
> > to be used as a transparent fallback in user namespaces.
> > 
> > Root in a user ns cannot be trusted to write security.capability
> > xattrs, because any user on the host could map his own uid to root
> > in a namespace, write the xattr, and execute the file with privilege
> > on the host.
> > 
> > With this patch, when root in a user ns asks to write security.capability,
> > the kernel will transparently write a security.nscapability xattr
> > instead, filling in the kuid of the calling user's root uid.  Subsequently,
> > any task executing the file which has the noted k_uid as its root uid,
> > or which is in a descendent user_ns of such a user_ns, will run the
> > file with capabilities.
> > 
> > When reading the security.capability xattr from a non-init user_ns, a valid
> > security.nscapability will be shown if it exists.  Such a task is not
> > allowed to read security.nscapability.  This could be accomodated, however
> 
> Add the word "directly" as "to read security.nscapability directly".

Updated in my git tree.

> > it requires the kernel to convert the kuid_t to a valid uid in the reader's
> > user_ns.  So for now it's simply not supported.
> 
> I really like the idea that the kernel transparently replaces
> nscapability for capability.
> 
> > Only a single security.nscapability xattr may be written.  This patch
> > could be expanded to allow a list of capabilities and rootids, however
> > I do not believe that to be a worthwhile use case.
> 
> Ok
> 
> > This allows a simple setxattr to work, allows tar/untar to
> > work, and allows us to tar in one namespace and untar in
> > another while preserving the capability, without risking
> > leaking privilege into a parent namespace.
> > 
> > Note - listxattr is not being handled here.  So results of that can be
> > inconsistent with get/setxattr.  Fixing that will require yet more
> > deceit in fs/xattr.c.
> > 
> > Note2 - it may be less sneaky to hide all the magic behind the
> > security.nscapability xattr.  So userspace would need to know to
> > use that xattr name when needed, but with the same format as
> > security.capability.  The kuid_t rootid would be filled in by the
> > kernel.  That's a middle ground between my last patch and this one.
> 
> The less userspace needs to differentiate between running in a namespace
> and not, the better.
> 
> Note3 - capability is currently protected by EVM, when enabled.  Should
> ns_capability also be a protected xattr?

Hm - that would protect it from offline attacks, but allow the container
to update it, right?  That sounds good.

> > Signed-off-by: Serge Hallyn 
> > ---
> >  fs/xattr.c  |  18 ++-
> >  include/linux/capability.h  |   8 +-
> >  include/uapi/linux/capability.h |  19 +++
> >  include/uapi/linux/xattr.h  |   3 +
> >  security/commoncap.c| 253 
> > ++--
> >  5 files changed, 291 insertions(+), 10 deletions(-)
> > 
> > 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() != _user_ns) {
> > +   cap_setxattr_make_nscap(dentry, value, size, , 
> > );
> > +   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 

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

[Mimi Zohar]

On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> This patch introduces a new security.nscapability xattr.  It
> is mostly like security.capability, but also lists a 'rootid'.
> This is the uid_t (in init_user_ns) of the root id (uid 0 in a
> namespace) in whose namespaces the file capabilities may take
> effect.
> 
> A privileged (cap_setfcap) process in the initial user ns may
> set and read this xattr directly.  However, its real intent is
> to be used as a transparent fallback in user namespaces.
> 
> Root in a user ns cannot be trusted to write security.capability
> xattrs, because any user on the host could map his own uid to root
> in a namespace, write the xattr, and execute the file with privilege
> on the host.
> 
> With this patch, when root in a user ns asks to write security.capability,
> the kernel will transparently write a security.nscapability xattr
> instead, filling in the kuid of the calling user's root uid.  Subsequently,
> any task executing the file which has the noted k_uid as its root uid,
> or which is in a descendent user_ns of such a user_ns, will run the
> file with capabilities.
> 
> When reading the security.capability xattr from a non-init user_ns, a valid
> security.nscapability will be shown if it exists.  Such a task is not
> allowed to read security.nscapability.  This could be accomodated, however

Add the word "directly" as "to read security.nscapability directly".

> it requires the kernel to convert the kuid_t to a valid uid in the reader's
> user_ns.  So for now it's simply not supported.

I really like the idea that the kernel transparently replaces
nscapability for capability.

> Only a single security.nscapability xattr may be written.  This patch
> could be expanded to allow a list of capabilities and rootids, however
> I do not believe that to be a worthwhile use case.

Ok

> This allows a simple setxattr to work, allows tar/untar to
> work, and allows us to tar in one namespace and untar in
> another while preserving the capability, without risking
> leaking privilege into a parent namespace.
> 
> Note - listxattr is not being handled here.  So results of that can be
> inconsistent with get/setxattr.  Fixing that will require yet more
> deceit in fs/xattr.c.
> 
> Note2 - it may be less sneaky to hide all the magic behind the
> security.nscapability xattr.  So userspace would need to know to
> use that xattr name when needed, but with the same format as
> security.capability.  The kuid_t rootid would be filled in by the
> kernel.  That's a middle ground between my last patch and this one.

The less userspace needs to differentiate between running in a namespace
and not, the better.

Note3 - capability is currently protected by EVM, when enabled.  Should
ns_capability also be a protected xattr?

> Signed-off-by: Serge Hallyn 
> ---
>  fs/xattr.c  |  18 ++-
>  include/linux/capability.h  |   8 +-
>  include/uapi/linux/capability.h |  19 +++
>  include/uapi/linux/xattr.h  |   3 +
>  security/commoncap.c| 253 
> ++--
>  5 files changed, 291 insertions(+), 10 deletions(-)
> 
> 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() != _user_ns) {
> + cap_setxattr_make_nscap(dentry, value, size, , 
> );
> + 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.

(Will continue reviewing ...)

Mimi

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

[Mimi Zohar]

On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> This patch introduces a new security.nscapability xattr.  It
> is mostly like security.capability, but also lists a 'rootid'.
> This is the uid_t (in init_user_ns) of the root id (uid 0 in a
> namespace) in whose namespaces the file capabilities may take
> effect.
> 
> A privileged (cap_setfcap) process in the initial user ns may
> set and read this xattr directly.  However, its real intent is
> to be used as a transparent fallback in user namespaces.
> 
> Root in a user ns cannot be trusted to write security.capability
> xattrs, because any user on the host could map his own uid to root
> in a namespace, write the xattr, and execute the file with privilege
> on the host.
> 
> With this patch, when root in a user ns asks to write security.capability,
> the kernel will transparently write a security.nscapability xattr
> instead, filling in the kuid of the calling user's root uid.  Subsequently,
> any task executing the file which has the noted k_uid as its root uid,
> or which is in a descendent user_ns of such a user_ns, will run the
> file with capabilities.
> 
> When reading the security.capability xattr from a non-init user_ns, a valid
> security.nscapability will be shown if it exists.  Such a task is not
> allowed to read security.nscapability.  This could be accomodated, however

Add the word "directly" as "to read security.nscapability directly".

> it requires the kernel to convert the kuid_t to a valid uid in the reader's
> user_ns.  So for now it's simply not supported.

I really like the idea that the kernel transparently replaces
nscapability for capability.

> Only a single security.nscapability xattr may be written.  This patch
> could be expanded to allow a list of capabilities and rootids, however
> I do not believe that to be a worthwhile use case.

Ok

> This allows a simple setxattr to work, allows tar/untar to
> work, and allows us to tar in one namespace and untar in
> another while preserving the capability, without risking
> leaking privilege into a parent namespace.
> 
> Note - listxattr is not being handled here.  So results of that can be
> inconsistent with get/setxattr.  Fixing that will require yet more
> deceit in fs/xattr.c.
> 
> Note2 - it may be less sneaky to hide all the magic behind the
> security.nscapability xattr.  So userspace would need to know to
> use that xattr name when needed, but with the same format as
> security.capability.  The kuid_t rootid would be filled in by the
> kernel.  That's a middle ground between my last patch and this one.

The less userspace needs to differentiate between running in a namespace
and not, the better.

Note3 - capability is currently protected by EVM, when enabled.  Should
ns_capability also be a protected xattr?

> Signed-off-by: Serge Hallyn 
> ---
>  fs/xattr.c  |  18 ++-
>  include/linux/capability.h  |   8 +-
>  include/uapi/linux/capability.h |  19 +++
>  include/uapi/linux/xattr.h  |   3 +
>  security/commoncap.c| 253 
> ++--
>  5 files changed, 291 insertions(+), 10 deletions(-)
> 
> 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() != _user_ns) {
> + cap_setxattr_make_nscap(dentry, value, size, , 
> );
> + 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.

(Will continue reviewing ...)

Mimi