On Mon, 2007-07-02 at 17:06 -0500, Serge E. Hallyn wrote: > 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 > > >From 5ec50bc22d3320565002658433829f7dc5bc0aa5 Mon Sep 17 00:00:00 2001 > From: Serge E. Hallyn <[EMAIL PROTECTED]> > Date: Mon, 2 Jul 2007 14:07:51 -0400 > Subject: [PATCH 1/1] file caps: update selinux xattr hooks (v2) > > 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. > > Note that the setxattr and removexattr logic for non selinux > attrs appears to be identical. So I do have another patch > where selinux_inode_setotherxattr takes an extra argument > u32 av (in case removexattr ever gets its own av permission) > so removexattr can shrink and just use that. But first I > thought I'd see if this version is even close correct :)
Yes, looks sane, and feel free to have both hooks use a common helper for non-selinux attributes. I don't think you even need to bother with the u32 av argument; if we later split the check, we can change it then (it isn't as though these functions need to have a stable interface). > > Signed-off-by: Serge E. Hallyn <[EMAIL PROTECTED]> > --- > security/selinux/hooks.c | 48 ++++++++++++++++++++++++++++----------------- > 1 files changed, 30 insertions(+), 18 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index af42820..336525c 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2289,6 +2289,25 @@ static int selinux_inode_getattr(struct vfsmount *mnt, > struct dentry *dentry) > return dentry_has_perm(current, mnt, dentry, FILE__GETATTR); > } > > +static int selinux_inode_setotherxattr(struct dentry *dentry, char *name) > +{ > + if (!strncmp(name, XATTR_SECURITY_PREFIX, > + sizeof XATTR_SECURITY_PREFIX - 1)) { > + if (!strcmp(name, XATTR_NAME_CAPS)) { > + if (!capable(CAP_SETFCAP)) > + return -EPERM; > + } else if (!capable(CAP_SYS_ADMIN)) { > + /* A different attribute in the security namespace. > + Restrict to administrator. */ > + return -EPERM; > + } > + } > + > + /* Not an attribute we recognize, so just check the > + ordinary setattr permission. */ > + return dentry_has_perm(current, NULL, dentry, FILE__SETATTR); > +} > + > static int selinux_inode_setxattr(struct dentry *dentry, char *name, void > *value, size_t size, int flags) > { > struct task_security_struct *tsec = current->security; > @@ -2299,19 +2318,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; > - } > - > - /* 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) > @@ -2387,11 +2395,15 @@ 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; > + sizeof XATTR_SECURITY_PREFIX - 1)) { > + if (!strcmp(name, XATTR_NAME_CAPS)) { > + if (!capable(CAP_SETFCAP)) > + return -EPERM; > + } else if (!capable(CAP_SYS_ADMIN)) { > + /* A different attribute in the security > + namespace. Restrict to administrator. */ > + return -EPERM; > + } > } > > /* Not an attribute we recognize, so just check the -- 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/