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

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

[Serge E. Hallyn]

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@us.ibm.com>
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@us.ibm.com>
---
 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;
+}
+
 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 +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;
-		}
-
-		/* 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);
+
+	/* 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)
-- 
1.5.1.1.GIT

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

[Stephen Smalley]

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@us.ibm.com>
> 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@us.ibm.com>
> ---
>  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

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

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> 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@us.ibm.com>
> > 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@us.ibm.com>
> > ---
> >  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

Crud.  I *thought* I had a clue what I was doing.

Will give it another try.

thanks,
-serge

> 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-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> 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@us.ibm.com>
> > 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@us.ibm.com>
> > ---
> >  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

>From 5ec50bc22d3320565002658433829f7dc5bc0aa5 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue@us.ibm.com>
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 :)

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 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
-- 
1.5.1.1.GIT

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

[Stephen Smalley]

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 <serue@us.ibm.com>
> 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 <serue@us.ibm.com>
> ---
>  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

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

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> 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 <serue@us.ibm.com>
> > 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).

Great, here is the new patch then.

thanks,
-serge


>From 3aaf0621dd6903ef7b73cc17df4d72140ad1e922 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue@us.ibm.com>
Date: Mon, 2 Jul 2007 14:14:40 -0400
Subject: [PATCH 1/1] file caps: update selinux xattr hooks (v4)

SELinux does not call out to its 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.  Also consolidate the handling
of non-selinux xattrs in removexattr and setxattr.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 security/selinux/hooks.c |   50 +++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index af42820..54e8a47 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)
@@ -2385,20 +2393,8 @@ 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;
-		}
-
-		/* Not an attribute we recognize, so just check the
-		   ordinary setattr permission. Might want a separate
-		   permission for removexattr. */
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
-	}
+	if (strcmp(name, XATTR_NAME_SELINUX))
+		return selinux_inode_setotherxattr(dentry, name);
 
 	/* No one is allowed to remove a SELinux security label.
 	   You can change the label, but all data must be labeled. */
-- 
1.5.1.1.GIT