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 <[EMAIL PROTECTED]> > 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 <[EMAIL PROTECTED]> > --- > 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. > @@ -2299,19 +2323,8 @@ static int selinux_inode_setxattr(struct dentry > *dentry, char *name, void *value > u32 newsid; > int rc = 0; > > - if (strcmp(name, XATTR_NAME_SELINUX)) { > - if (!strncmp(name, XATTR_SECURITY_PREFIX, > - sizeof XATTR_SECURITY_PREFIX - 1) && > - !capable(CAP_SYS_ADMIN)) { > - /* A different attribute in the security namespace. > - Restrict to administrator. */ > - return -EPERM; > - } Note that if setting a security.<non-selinux> attribute, we first check the capability but then fall through on success to the FILE__SETATTR check below. > - > - /* Not an attribute we recognize, so just check the > - ordinary setattr permission. */ > - return dentry_has_perm(current, NULL, dentry, FILE__SETATTR); > - } > + if (strcmp(name, XATTR_NAME_SELINUX)) > + return selinux_inode_setotherxattr(dentry, name); > > sbsec = inode->i_sb->s_security; > if (sbsec->behavior == SECURITY_FS_USE_MNTPOINT) > @@ -2385,24 +2398,32 @@ static int selinux_inode_listxattr (struct dentry > *dentry) > > static int selinux_inode_removexattr (struct dentry *dentry, char *name) > { > - if (strcmp(name, XATTR_NAME_SELINUX)) { > - if (!strncmp(name, XATTR_SECURITY_PREFIX, > - sizeof XATTR_SECURITY_PREFIX - 1) && > - !capable(CAP_SYS_ADMIN)) { > - /* A different attribute in the security namespace. > - Restrict to administrator. */ > - return -EPERM; > - } > + /* No one is allowed to remove a SELinux security label. > + You can change the label, but all data must be labeled. */ > + if (!strcmp(name, XATTR_NAME_SELINUX)) > + return -EACCES; > > - /* Not an attribute we recognize, so just check the > - ordinary setattr permission. Might want a separate > - permission for removexattr. */ > + /* Not an attribute we recognize, so just check the > + ordinary setattr permission. Might want a separate > + permission for removexattr. */ > + if (strncmp(name, XATTR_SECURITY_PREFIX, > + sizeof XATTR_SECURITY_PREFIX - 1)) > return dentry_has_perm(current, NULL, dentry, FILE__SETATTR); Same problem here - when you changed the flow, you unintentionally changed the behavior. > + > + /* a file capability requires cap_setfcap */ > + if (!strcmp(name, XATTR_NAME_CAPS)) { > + if (!capable(CAP_SETFCAP)) > + return -EPERM; > + else > + return 0; > } > > - /* No one is allowed to remove a SELinux security label. > - You can change the label, but all data must be labeled. */ > - return -EACCES; > + /* A different attribute in the security namespace. > + Restrict to administrator. */ > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + return 0; > } > > static const char *selinux_inode_xattr_getsuffix(void) -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/