Re: [PATCH 1/1] file caps: update selinux xattr hooks

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@xxxxxxxxxxxxx):
> On Thu, 2007-06-28 at 13:22 -0500, Serge E. Hallyn wrote:
> > This fixes a shortcoming of the cap_setfcap patch I sent earlier,
> > pointed out by Stephen Smalley.
> > 
> > Seems to compile and boot on my little systems.
> > 
> > thanks,
> > -serge
> > 
> > >From d729000b922a2877a48ce2b5a03a9366d8c65d04 Mon Sep 17 00:00:00 2001
> > From: Serge E. Hallyn <serue@xxxxxxxxxx>
> > Date: Thu, 28 Jun 2007 11:57:19 -0400
> > Subject: [PATCH 1/1] file caps: update selinux xattr hooks
> > 
> > SELinux does not call out to it's secondary module for setxattr
> > or removexattr mediation, as the secondary module would
> > incorrectly prevent writing of selinux xattrs.  This means
> > that when selinux and capability are both loaded, admins will
> > be able to write file capabilities with CAP_SYS_ADMIN as before,
> > not with CAP_SETFCAP.
> > 
> > Update the selinux hooks to hardcode logic for the special
> > consideration for file caps.
> > 
> > I changed the flow of the removexattr hook to reduce the amount
> > of indentation I was getting.  It was probably written the way
> > it was for a reason, and if it was, I apologize and will
> > rewrite :)  If it wasn't, hopefully this way is ok.
> > 
> > Signed-off-by: Serge E. Hallyn <serue@xxxxxxxxxx>
> > ---
> >  security/selinux/hooks.c |   75 +++++++++++++++++++++++++++++----------------
> >  1 files changed, 48 insertions(+), 27 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index af42820..db0a4ed 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2289,6 +2289,30 @@ static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)
> >  	return dentry_has_perm(current, mnt, dentry, FILE__GETATTR);
> >  }
> >  
> > +/* called by selinux_inode_setxattr to mediate setting
> > + * of non-selinux xattrs */
> > +static int selinux_inode_setotherxattr(struct dentry *dentry, char *name)
> > +{
> > +	if (strncmp(name, XATTR_SECURITY_PREFIX,
> > +		     sizeof XATTR_SECURITY_PREFIX - 1))
> > +		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
> > +
> > +	/* a file capability requires cap_setfcap */
> > +	if (!strcmp(name, XATTR_NAME_CAPS)) {
> > +		if (!capable(CAP_SETFCAP))
> > +			return -EPERM;
> > +		else
> > +			return 0;
> > +	}
> > +
> > +	/* A different attribute in the security namespace.
> > +	   Restrict to administrator. */
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> > +
> > +	return 0;
> > +}
> 
> In reworking the flow of this code, you've changed the behavior (more so
> than you intended) - your checking above only applies the FILE__SETATTR
> check if dealing with a non-security attribute, whereas the original
> logic (below) applied that check to all non-selinux attributes.  So with
> your new logic, we don't get any process-to-object check for
> security.cap or security.<other>, and thus lose the domain-to-type check
> or the level-to-level check.


Thanks Stephen, does the following version appear correct?  It just
checks for a different cap for security.capability, then if granted
goes on to check FILE__GETATTR before granting setxattr or removexattr
on any security.* xattr.

thanks,
-serge