[RFC PATCH] lsm: fixup the inode xattr capability handling

[RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

The current security_inode_setxattr() and security_inode_removexattr()
hooks rely on individual LSMs to either call into the associated
capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
return a magic value of 1 to indicate that the LSM layer itself should
perform the capability checks.  Unfortunately, with the default return
value for these LSM hooks being 0, an individual LSM hook returning a
1 will cause the LSM hook processing to exit early, potentially
skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
of the LSMs which currently register inode xattr hooks should end up
returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
executing last there should be no real harm in stopping processing of
the LSM hooks.  However, the reliance on the individual LSMs to either
call the capability hooks themselves, or signal the LSM with a return
value of 1, is fragile and relies on a specific set of LSMs being
enabled.  This patch is an effort to resolve, or minimize, these
issues.

Before we discuss the solution, there are a few observations and
considerations that we need to take into account:
* BPF LSM registers an implementation for every LSM hook, and that
  implementation simply returns the hook's default return value, a
  0 in this case.  We want to ensure that the default BPF LSM behavior
  results in the capability checks being called.
* SELinux and Smack do not expect the traditional capability checks
  to be applied to the xattrs that they "own".
* SELinux and Smack are currently written in such a way that the
  xattr capability checks happen before any additional LSM specific
  access control checks.  SELinux does apply SELinux specific access
  controls to all xattrs, even those not "owned" by SELinux.
* IMA and EVM also register xattr hooks but assume that the LSM layer
  and specific LSMs have already authorized the basic xattr operation.

In order to ensure we perform the capability based access controls
before the individual LSM access controls, perform only one capability
access control check for each operation, and clarify the logic around
applying the capability controls, we need a mechanism to determine if
any of the enabled LSMs "own" a particular xattr and want to take
responsibility for controlling access to that xattr.  The solution in
this patch is to create a new LSM hook, 'inode_xattr_skipcap', that is
not exported to the rest of the kernel via a security_XXX() function,
but is used by the LSM layer to determine if a LSM wants to control
access to a given xattr and avoid the traditional capability controls.
Registering an inode_xattr_skipcap hook is optional, if a LSM declines
to register an implementation, or uses an implementation that simply
returns the default value (0), there is no effect as the LSM continues
to enforce the capability based controls (unless another LSM takes
ownership of the xattr).  If none of the LSMs signal that the
capability checks should be skipped, the capability check is performed
and if access is granted the individual LSM xattr access control hooks
are executed, keeping with the DAC-before-LSM convention.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 include/linux/lsm_hook_defs.h |  1 +
 security/security.c           | 70 ++++++++++++++++++++++++-----------
 security/selinux/hooks.c      | 28 ++++++++++----
 security/smack/smack_lsm.c    | 31 +++++++++++++++-
 4 files changed, 98 insertions(+), 32 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 334e00efbde4..6e54dae3256b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -144,6 +144,7 @@ LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
 LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, int ia_valid)
 LSM_HOOK(int, 0, inode_getattr, const struct path *path)
+LSM_HOOK(int, 0, inode_xattr_skipcap, const char *name)
 LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *name, const void *value,
 	 size_t size, int flags)
diff --git a/security/security.c b/security/security.c
index 7e118858b545..1f5c68e2a62a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2278,7 +2278,20 @@ int security_inode_getattr(const struct path *path)
  * @size: size of xattr value
  * @flags: flags
  *
- * Check permission before setting the extended attributes.
+ * This hook performs the desired permission checks before setting the extended
+ * attributes (xattrs) on @dentry.  It is important to note that we have some
+ * additional logic before the main LSM implementation calls to detect if we
+ * need to perform an additional capability check at the LSM layer.
+ *
+ * Normally we enforce a capability check prior to executing the various LSM
+ * hook implementations, but if a LSM wants to avoid this capability check,
+ * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
+ * xattrs that it wants to avoid the capability check, leaving the LSM fully
+ * responsible for enforcing the access control for the specific xattr.  If all
+ * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
+ * or return a 0 (the default return value), the capability check is still
+ * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
+ * check is performed.
  *
  * Return: Returns 0 if permission is granted.
  */
@@ -2286,20 +2299,20 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, const char *name,
 			    const void *value, size_t size, int flags)
 {
-	int ret;
+	int rc;
 
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
-	/*
-	 * SELinux and Smack integrate the cap call,
-	 * so assume that all LSMs supplying this call do so.
-	 */
-	ret = call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
-			    flags);
 
-	if (ret == 1)
-		ret = cap_inode_setxattr(dentry, name, value, size, flags);
-	return ret;
+	/* enforce the capability checks at the lsm layer, if needed */
+	if (!call_int_hook(inode_xattr_skipcap, name)) {
+		rc = cap_inode_setxattr(dentry, name, value, size, flags);
+		if (rc)
+			return rc;
+	}
+
+	return call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
+			     flags);
 }
 
 /**
@@ -2452,26 +2465,39 @@ int security_inode_listxattr(struct dentry *dentry)
  * @dentry: file
  * @name: xattr name
  *
- * Check permission before removing the extended attribute identified by @name
- * for @dentry.
+ * This hook performs the desired permission checks before setting the extended
+ * attributes (xattrs) on @dentry.  It is important to note that we have some
+ * additional logic before the main LSM implementation calls to detect if we
+ * need to perform an additional capability check at the LSM layer.
+ *
+ * Normally we enforce a capability check prior to executing the various LSM
+ * hook implementations, but if a LSM wants to avoid this capability check,
+ * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
+ * xattrs that it wants to avoid the capability check, leaving the LSM fully
+ * responsible for enforcing the access control for the specific xattr.  If all
+ * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
+ * or return a 0 (the default return value), the capability check is still
+ * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
+ * check is performed.
  *
  * Return: Returns 0 if permission is granted.
  */
 int security_inode_removexattr(struct mnt_idmap *idmap,
 			       struct dentry *dentry, const char *name)
 {
-	int ret;
+	int rc;
 
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
-	/*
-	 * SELinux and Smack integrate the cap call,
-	 * so assume that all LSMs supplying this call do so.
-	 */
-	ret = call_int_hook(inode_removexattr, idmap, dentry, name);
-	if (ret == 1)
-		ret = cap_inode_removexattr(idmap, dentry, name);
-	return ret;
+
+	/* enforce the capability checks at the lsm layer, if needed */
+	if (!call_int_hook(inode_xattr_skipcap, name)) {
+		rc = cap_inode_removexattr(idmap, dentry, name);
+		if (rc)
+			return rc;
+	}
+
+	return call_int_hook(inode_removexattr, idmap, dentry, name);
 }
 
 /**
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3448454c82d0..7be385ebf09b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3181,6 +3181,23 @@ static bool has_cap_mac_admin(bool audit)
 	return true;
 }
 
+/**
+ * selinux_inode_xattr_skipcap - Skip the xattr capability checks?
+ * @name: name of the xattr
+ *
+ * Returns 1 to indicate that SELinux "owns" the access control rights to xattrs
+ * named @name; the LSM layer should avoid enforcing any traditional
+ * capability based access controls on this xattr.  Returns 0 to indicate that
+ * SELinux does not "own" the access control rights to xattrs named @name and is
+ * deferring to the LSM layer for further access controls, including capability
+ * based controls.
+ */
+static int selinux_inode_xattr_skipcap(const char *name)
+{
+	/* require capability check if not a selinux xattr */
+	return !strcmp(name, XATTR_NAME_SELINUX);
+}
+
 static int selinux_inode_setxattr(struct mnt_idmap *idmap,
 				  struct dentry *dentry, const char *name,
 				  const void *value, size_t size, int flags)
@@ -3192,15 +3209,9 @@ static int selinux_inode_setxattr(struct mnt_idmap *idmap,
 	u32 newsid, sid = current_sid();
 	int rc = 0;
 
-	if (strcmp(name, XATTR_NAME_SELINUX)) {
-		rc = cap_inode_setxattr(dentry, name, value, size, flags);
-		if (rc)
-			return rc;
-
-		/* Not an attribute we recognize, so just check the
-		   ordinary setattr permission. */
+	/* if not a selinux xattr, only check the ordinary setattr perm */
+	if (strcmp(name, XATTR_NAME_SELINUX))
 		return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
-	}
 
 	if (!selinux_initialized())
 		return (inode_owner_or_capable(idmap, inode) ? 0 : -EPERM);
@@ -7185,6 +7196,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_permission, selinux_inode_permission),
 	LSM_HOOK_INIT(inode_setattr, selinux_inode_setattr),
 	LSM_HOOK_INIT(inode_getattr, selinux_inode_getattr),
+	LSM_HOOK_INIT(inode_xattr_skipcap, selinux_inode_xattr_skipcap),
 	LSM_HOOK_INIT(inode_setxattr, selinux_inode_setxattr),
 	LSM_HOOK_INIT(inode_post_setxattr, selinux_inode_post_setxattr),
 	LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 146667937811..6e37df0465a4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1282,6 +1282,33 @@ static int smack_inode_getattr(const struct path *path)
 	return rc;
 }
 
+/**
+ * smack_inode_xattr_skipcap - Skip the xattr capability checks?
+ * @name: name of the xattr
+ *
+ * Returns 1 to indicate that Smack "owns" the access control rights to xattrs
+ * named @name; the LSM layer should avoid enforcing any traditional
+ * capability based access controls on this xattr.  Returns 0 to indicate that
+ * Smack does not "own" the access control rights to xattrs named @name and is
+ * deferring to the LSM layer for further access controls, including capability
+ * based controls.
+ */
+static int smack_inode_xattr_skipcap(const char *name)
+{
+	if (strncmp(name, XATTR_SMACK_SUFFIX, strlen(XATTR_SMACK_SUFFIX)))
+		return 0;
+
+	if (strcmp(name, XATTR_NAME_SMACK) == 0 ||
+	    strcmp(name, XATTR_NAME_SMACKIPIN) == 0 ||
+	    strcmp(name, XATTR_NAME_SMACKIPOUT) == 0 ||
+	    strcmp(name, XATTR_NAME_SMACKEXEC) == 0 ||
+	    strcmp(name, XATTR_NAME_SMACKMMAP) == 0 ||
+	    strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0)
+		return 1;
+
+	return 0;
+}
+
 /**
  * smack_inode_setxattr - Smack check for setting xattrs
  * @idmap: idmap of the mount
@@ -1325,8 +1352,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
 		    size != TRANS_TRUE_SIZE ||
 		    strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
 			rc = -EINVAL;
-	} else
-		rc = cap_inode_setxattr(dentry, name, value, size, flags);
+	}
 
 	if (check_priv && !smack_privileged(CAP_MAC_ADMIN))
 		rc = -EPERM;
@@ -5050,6 +5076,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_permission, smack_inode_permission),
 	LSM_HOOK_INIT(inode_setattr, smack_inode_setattr),
 	LSM_HOOK_INIT(inode_getattr, smack_inode_getattr),
+	LSM_HOOK_INIT(inode_xattr_skipcap, smack_inode_xattr_skipcap),
 	LSM_HOOK_INIT(inode_setxattr, smack_inode_setxattr),
 	LSM_HOOK_INIT(inode_post_setxattr, smack_inode_post_setxattr),
 	LSM_HOOK_INIT(inode_getxattr, smack_inode_getxattr),
-- 
2.45.0

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Casey Schaufler]

On 5/2/2024 5:58 PM, Paul Moore wrote:
> The current security_inode_setxattr() and security_inode_removexattr()
> hooks rely on individual LSMs to either call into the associated
> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
> return a magic value of 1 to indicate that the LSM layer itself should
> perform the capability checks.  Unfortunately, with the default return
> value for these LSM hooks being 0, an individual LSM hook returning a
> 1 will cause the LSM hook processing to exit early, potentially
> skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
> of the LSMs which currently register inode xattr hooks should end up
> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
> executing last there should be no real harm in stopping processing of
> the LSM hooks.  However, the reliance on the individual LSMs to either
> call the capability hooks themselves, or signal the LSM with a return
> value of 1, is fragile and relies on a specific set of LSMs being
> enabled.  This patch is an effort to resolve, or minimize, these
> issues.
>
> Before we discuss the solution,

https://lore.kernel.org/lkml/20231215221636.105680-1-casey@schaufler-ca.com/T/#mac61625dc1983d13ee5e8015fd22e1165381f079

... or am I missing something?

>  there are a few observations and
> considerations that we need to take into account:
> * BPF LSM registers an implementation for every LSM hook, and that
>   implementation simply returns the hook's default return value, a
>   0 in this case.  We want to ensure that the default BPF LSM behavior
>   results in the capability checks being called.
> * SELinux and Smack do not expect the traditional capability checks
>   to be applied to the xattrs that they "own".
> * SELinux and Smack are currently written in such a way that the
>   xattr capability checks happen before any additional LSM specific
>   access control checks.  SELinux does apply SELinux specific access
>   controls to all xattrs, even those not "owned" by SELinux.
> * IMA and EVM also register xattr hooks but assume that the LSM layer
>   and specific LSMs have already authorized the basic xattr operation.
>
> In order to ensure we perform the capability based access controls
> before the individual LSM access controls, perform only one capability
> access control check for each operation, and clarify the logic around
> applying the capability controls, we need a mechanism to determine if
> any of the enabled LSMs "own" a particular xattr and want to take
> responsibility for controlling access to that xattr.  The solution in
> this patch is to create a new LSM hook, 'inode_xattr_skipcap', that is
> not exported to the rest of the kernel via a security_XXX() function,
> but is used by the LSM layer to determine if a LSM wants to control
> access to a given xattr and avoid the traditional capability controls.
> Registering an inode_xattr_skipcap hook is optional, if a LSM declines
> to register an implementation, or uses an implementation that simply
> returns the default value (0), there is no effect as the LSM continues
> to enforce the capability based controls (unless another LSM takes
> ownership of the xattr).  If none of the LSMs signal that the
> capability checks should be skipped, the capability check is performed
> and if access is granted the individual LSM xattr access control hooks
> are executed, keeping with the DAC-before-LSM convention.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  include/linux/lsm_hook_defs.h |  1 +
>  security/security.c           | 70 ++++++++++++++++++++++++-----------
>  security/selinux/hooks.c      | 28 ++++++++++----
>  security/smack/smack_lsm.c    | 31 +++++++++++++++-
>  4 files changed, 98 insertions(+), 32 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 334e00efbde4..6e54dae3256b 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -144,6 +144,7 @@ LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
>  LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
>  	 struct dentry *dentry, int ia_valid)
>  LSM_HOOK(int, 0, inode_getattr, const struct path *path)
> +LSM_HOOK(int, 0, inode_xattr_skipcap, const char *name)
>  LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
>  	 struct dentry *dentry, const char *name, const void *value,
>  	 size_t size, int flags)
> diff --git a/security/security.c b/security/security.c
> index 7e118858b545..1f5c68e2a62a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2278,7 +2278,20 @@ int security_inode_getattr(const struct path *path)
>   * @size: size of xattr value
>   * @flags: flags
>   *
> - * Check permission before setting the extended attributes.
> + * This hook performs the desired permission checks before setting the extended
> + * attributes (xattrs) on @dentry.  It is important to note that we have some
> + * additional logic before the main LSM implementation calls to detect if we
> + * need to perform an additional capability check at the LSM layer.
> + *
> + * Normally we enforce a capability check prior to executing the various LSM
> + * hook implementations, but if a LSM wants to avoid this capability check,
> + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
> + * xattrs that it wants to avoid the capability check, leaving the LSM fully
> + * responsible for enforcing the access control for the specific xattr.  If all
> + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
> + * or return a 0 (the default return value), the capability check is still
> + * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
> + * check is performed.
>   *
>   * Return: Returns 0 if permission is granted.
>   */
> @@ -2286,20 +2299,20 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
>  			    struct dentry *dentry, const char *name,
>  			    const void *value, size_t size, int flags)
>  {
> -	int ret;
> +	int rc;
>  
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return 0;
> -	/*
> -	 * SELinux and Smack integrate the cap call,
> -	 * so assume that all LSMs supplying this call do so.
> -	 */
> -	ret = call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
> -			    flags);
>  
> -	if (ret == 1)
> -		ret = cap_inode_setxattr(dentry, name, value, size, flags);
> -	return ret;
> +	/* enforce the capability checks at the lsm layer, if needed */
> +	if (!call_int_hook(inode_xattr_skipcap, name)) {
> +		rc = cap_inode_setxattr(dentry, name, value, size, flags);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
> +			     flags);
>  }
>  
>  /**
> @@ -2452,26 +2465,39 @@ int security_inode_listxattr(struct dentry *dentry)
>   * @dentry: file
>   * @name: xattr name
>   *
> - * Check permission before removing the extended attribute identified by @name
> - * for @dentry.
> + * This hook performs the desired permission checks before setting the extended
> + * attributes (xattrs) on @dentry.  It is important to note that we have some
> + * additional logic before the main LSM implementation calls to detect if we
> + * need to perform an additional capability check at the LSM layer.
> + *
> + * Normally we enforce a capability check prior to executing the various LSM
> + * hook implementations, but if a LSM wants to avoid this capability check,
> + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
> + * xattrs that it wants to avoid the capability check, leaving the LSM fully
> + * responsible for enforcing the access control for the specific xattr.  If all
> + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
> + * or return a 0 (the default return value), the capability check is still
> + * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
> + * check is performed.
>   *
>   * Return: Returns 0 if permission is granted.
>   */
>  int security_inode_removexattr(struct mnt_idmap *idmap,
>  			       struct dentry *dentry, const char *name)
>  {
> -	int ret;
> +	int rc;
>  
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return 0;
> -	/*
> -	 * SELinux and Smack integrate the cap call,
> -	 * so assume that all LSMs supplying this call do so.
> -	 */
> -	ret = call_int_hook(inode_removexattr, idmap, dentry, name);
> -	if (ret == 1)
> -		ret = cap_inode_removexattr(idmap, dentry, name);
> -	return ret;
> +
> +	/* enforce the capability checks at the lsm layer, if needed */
> +	if (!call_int_hook(inode_xattr_skipcap, name)) {
> +		rc = cap_inode_removexattr(idmap, dentry, name);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return call_int_hook(inode_removexattr, idmap, dentry, name);
>  }
>  
>  /**
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3448454c82d0..7be385ebf09b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3181,6 +3181,23 @@ static bool has_cap_mac_admin(bool audit)
>  	return true;
>  }
>  
> +/**
> + * selinux_inode_xattr_skipcap - Skip the xattr capability checks?
> + * @name: name of the xattr
> + *
> + * Returns 1 to indicate that SELinux "owns" the access control rights to xattrs
> + * named @name; the LSM layer should avoid enforcing any traditional
> + * capability based access controls on this xattr.  Returns 0 to indicate that
> + * SELinux does not "own" the access control rights to xattrs named @name and is
> + * deferring to the LSM layer for further access controls, including capability
> + * based controls.
> + */
> +static int selinux_inode_xattr_skipcap(const char *name)
> +{
> +	/* require capability check if not a selinux xattr */
> +	return !strcmp(name, XATTR_NAME_SELINUX);
> +}
> +
>  static int selinux_inode_setxattr(struct mnt_idmap *idmap,
>  				  struct dentry *dentry, const char *name,
>  				  const void *value, size_t size, int flags)
> @@ -3192,15 +3209,9 @@ static int selinux_inode_setxattr(struct mnt_idmap *idmap,
>  	u32 newsid, sid = current_sid();
>  	int rc = 0;
>  
> -	if (strcmp(name, XATTR_NAME_SELINUX)) {
> -		rc = cap_inode_setxattr(dentry, name, value, size, flags);
> -		if (rc)
> -			return rc;
> -
> -		/* Not an attribute we recognize, so just check the
> -		   ordinary setattr permission. */
> +	/* if not a selinux xattr, only check the ordinary setattr perm */
> +	if (strcmp(name, XATTR_NAME_SELINUX))
>  		return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> -	}
>  
>  	if (!selinux_initialized())
>  		return (inode_owner_or_capable(idmap, inode) ? 0 : -EPERM);
> @@ -7185,6 +7196,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(inode_permission, selinux_inode_permission),
>  	LSM_HOOK_INIT(inode_setattr, selinux_inode_setattr),
>  	LSM_HOOK_INIT(inode_getattr, selinux_inode_getattr),
> +	LSM_HOOK_INIT(inode_xattr_skipcap, selinux_inode_xattr_skipcap),
>  	LSM_HOOK_INIT(inode_setxattr, selinux_inode_setxattr),
>  	LSM_HOOK_INIT(inode_post_setxattr, selinux_inode_post_setxattr),
>  	LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 146667937811..6e37df0465a4 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1282,6 +1282,33 @@ static int smack_inode_getattr(const struct path *path)
>  	return rc;
>  }
>  
> +/**
> + * smack_inode_xattr_skipcap - Skip the xattr capability checks?
> + * @name: name of the xattr
> + *
> + * Returns 1 to indicate that Smack "owns" the access control rights to xattrs
> + * named @name; the LSM layer should avoid enforcing any traditional
> + * capability based access controls on this xattr.  Returns 0 to indicate that
> + * Smack does not "own" the access control rights to xattrs named @name and is
> + * deferring to the LSM layer for further access controls, including capability
> + * based controls.
> + */
> +static int smack_inode_xattr_skipcap(const char *name)
> +{
> +	if (strncmp(name, XATTR_SMACK_SUFFIX, strlen(XATTR_SMACK_SUFFIX)))
> +		return 0;
> +
> +	if (strcmp(name, XATTR_NAME_SMACK) == 0 ||
> +	    strcmp(name, XATTR_NAME_SMACKIPIN) == 0 ||
> +	    strcmp(name, XATTR_NAME_SMACKIPOUT) == 0 ||
> +	    strcmp(name, XATTR_NAME_SMACKEXEC) == 0 ||
> +	    strcmp(name, XATTR_NAME_SMACKMMAP) == 0 ||
> +	    strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0)
> +		return 1;
> +
> +	return 0;
> +}
> +
>  /**
>   * smack_inode_setxattr - Smack check for setting xattrs
>   * @idmap: idmap of the mount
> @@ -1325,8 +1352,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
>  		    size != TRANS_TRUE_SIZE ||
>  		    strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
>  			rc = -EINVAL;
> -	} else
> -		rc = cap_inode_setxattr(dentry, name, value, size, flags);
> +	}
>  
>  	if (check_priv && !smack_privileged(CAP_MAC_ADMIN))
>  		rc = -EPERM;
> @@ -5050,6 +5076,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(inode_permission, smack_inode_permission),
>  	LSM_HOOK_INIT(inode_setattr, smack_inode_setattr),
>  	LSM_HOOK_INIT(inode_getattr, smack_inode_getattr),
> +	LSM_HOOK_INIT(inode_xattr_skipcap, smack_inode_xattr_skipcap),
>  	LSM_HOOK_INIT(inode_setxattr, smack_inode_setxattr),
>  	LSM_HOOK_INIT(inode_post_setxattr, smack_inode_post_setxattr),
>  	LSM_HOOK_INIT(inode_getxattr, smack_inode_getxattr),

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

On Fri, May 3, 2024 at 11:26 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/2/2024 5:58 PM, Paul Moore wrote:
> > The current security_inode_setxattr() and security_inode_removexattr()
> > hooks rely on individual LSMs to either call into the associated
> > capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
> > return a magic value of 1 to indicate that the LSM layer itself should
> > perform the capability checks.  Unfortunately, with the default return
> > value for these LSM hooks being 0, an individual LSM hook returning a
> > 1 will cause the LSM hook processing to exit early, potentially
> > skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
> > of the LSMs which currently register inode xattr hooks should end up
> > returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
> > executing last there should be no real harm in stopping processing of
> > the LSM hooks.  However, the reliance on the individual LSMs to either
> > call the capability hooks themselves, or signal the LSM with a return
> > value of 1, is fragile and relies on a specific set of LSMs being
> > enabled.  This patch is an effort to resolve, or minimize, these
> > issues.
> >
> > Before we discuss the solution,
>
> https://lore.kernel.org/lkml/20231215221636.105680-1-casey@schaufler-ca.com/T/#mac61625dc1983d13ee5e8015fd22e1165381f079
>
> ... or am I missing something?

Yes, that patch, as well as some of the others that have been posted,
change the ordering of the access control checks, moving the LSM-based
checks ahead of the capability-based checks.  The patch I'm proposing
here not only preserves the current ordering, but it sticks with our
access control convention of DAC-before-LSM.

-- 
paul-moore.com

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Casey Schaufler]

On 5/3/2024 8:36 AM, Paul Moore wrote:
> On Fri, May 3, 2024 at 11:26 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 5/2/2024 5:58 PM, Paul Moore wrote:
>>> The current security_inode_setxattr() and security_inode_removexattr()
>>> hooks rely on individual LSMs to either call into the associated
>>> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
>>> return a magic value of 1 to indicate that the LSM layer itself should
>>> perform the capability checks.  Unfortunately, with the default return
>>> value for these LSM hooks being 0, an individual LSM hook returning a
>>> 1 will cause the LSM hook processing to exit early, potentially
>>> skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
>>> of the LSMs which currently register inode xattr hooks should end up
>>> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
>>> executing last there should be no real harm in stopping processing of
>>> the LSM hooks.  However, the reliance on the individual LSMs to either
>>> call the capability hooks themselves, or signal the LSM with a return
>>> value of 1, is fragile and relies on a specific set of LSMs being
>>> enabled.  This patch is an effort to resolve, or minimize, these
>>> issues.
>>>
>>> Before we discuss the solution,
>> https://lore.kernel.org/lkml/20231215221636.105680-1-casey@schaufler-ca.com/T/#mac61625dc1983d13ee5e8015fd22e1165381f079
>>
>> ... or am I missing something?
> Yes, that patch, as well as some of the others that have been posted,
> change the ordering of the access control checks, moving the LSM-based
> checks ahead of the capability-based checks.  The patch I'm proposing
> here not only preserves the current ordering, but it sticks with our
> access control convention of DAC-before-LSM.

Fair enough.

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

On Fri, May 3, 2024 at 11:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/3/2024 8:36 AM, Paul Moore wrote:
> > On Fri, May 3, 2024 at 11:26 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 5/2/2024 5:58 PM, Paul Moore wrote:
> >>> The current security_inode_setxattr() and security_inode_removexattr()
> >>> hooks rely on individual LSMs to either call into the associated
> >>> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
> >>> return a magic value of 1 to indicate that the LSM layer itself should
> >>> perform the capability checks.  Unfortunately, with the default return
> >>> value for these LSM hooks being 0, an individual LSM hook returning a
> >>> 1 will cause the LSM hook processing to exit early, potentially
> >>> skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
> >>> of the LSMs which currently register inode xattr hooks should end up
> >>> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
> >>> executing last there should be no real harm in stopping processing of
> >>> the LSM hooks.  However, the reliance on the individual LSMs to either
> >>> call the capability hooks themselves, or signal the LSM with a return
> >>> value of 1, is fragile and relies on a specific set of LSMs being
> >>> enabled.  This patch is an effort to resolve, or minimize, these
> >>> issues.
> >>>
> >>> Before we discuss the solution,
> >> https://lore.kernel.org/lkml/20231215221636.105680-1-casey@schaufler-ca.com/T/#mac61625dc1983d13ee5e8015fd22e1165381f079
> >>
> >> ... or am I missing something?
> > Yes, that patch, as well as some of the others that have been posted,
> > change the ordering of the access control checks, moving the LSM-based
> > checks ahead of the capability-based checks.  The patch I'm proposing
> > here not only preserves the current ordering, but it sticks with our
> > access control convention of DAC-before-LSM.
>
> Fair enough.

Are you okay with the patch otherwise?  It's too late for v6.9, but if
everyone is okay with this approach I'd like to merge this after the
v6.10 merge window closes.

I also need to track down the appropriate commits for the 'Fixes:'
tag(s); I'm not entirely convinced that some of the other patches were
targeting the proper commit ...

-- 
paul-moore.com

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Casey Schaufler]

On 5/3/2024 9:26 AM, Paul Moore wrote:
> On Fri, May 3, 2024 at 11:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 5/3/2024 8:36 AM, Paul Moore wrote:
>>> On Fri, May 3, 2024 at 11:26 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 5/2/2024 5:58 PM, Paul Moore wrote:
>>>>> The current security_inode_setxattr() and security_inode_removexattr()
>>>>> hooks rely on individual LSMs to either call into the associated
>>>>> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
>>>>> return a magic value of 1 to indicate that the LSM layer itself should
>>>>> perform the capability checks.  Unfortunately, with the default return
>>>>> value for these LSM hooks being 0, an individual LSM hook returning a
>>>>> 1 will cause the LSM hook processing to exit early, potentially
>>>>> skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
>>>>> of the LSMs which currently register inode xattr hooks should end up
>>>>> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
>>>>> executing last there should be no real harm in stopping processing of
>>>>> the LSM hooks.  However, the reliance on the individual LSMs to either
>>>>> call the capability hooks themselves, or signal the LSM with a return
>>>>> value of 1, is fragile and relies on a specific set of LSMs being
>>>>> enabled.  This patch is an effort to resolve, or minimize, these
>>>>> issues.
>>>>>
>>>>> Before we discuss the solution,
>>>> https://lore.kernel.org/lkml/20231215221636.105680-1-casey@schaufler-ca.com/T/#mac61625dc1983d13ee5e8015fd22e1165381f079
>>>>
>>>> ... or am I missing something?
>>> Yes, that patch, as well as some of the others that have been posted,
>>> change the ordering of the access control checks, moving the LSM-based
>>> checks ahead of the capability-based checks.  The patch I'm proposing
>>> here not only preserves the current ordering, but it sticks with our
>>> access control convention of DAC-before-LSM.
>> Fair enough.
> Are you okay with the patch otherwise?  It's too late for v6.9, but if
> everyone is okay with this approach I'd like to merge this after the
> v6.10 merge window closes.

I'm not real happy with the shear size of the change, but I
don't see a better approach that meets the stated objectives.
You can add my Acked-by.

> I also need to track down the appropriate commits for the 'Fixes:'
> tag(s); I'm not entirely convinced that some of the other patches were
> targeting the proper commit ...
>

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Serge Hallyn]

May 2, 2024 19:59:11 Paul Moore <paul@paul-moore.com>:

> The current security_inode_setxattr() and security_inode_removexattr()
> hooks rely on individual LSMs to either call into the associated
> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
> return a magic value of 1 to indicate that the LSM layer itself should
> perform the capability checks.  Unfortunately, with the default return
> value for these LSM hooks being 0, an individual LSM hook returning a
> 1 will cause the LSM hook processing to exit early, potentially
> skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
> of the LSMs which currently register inode xattr hooks should end up
> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
> executing last there should be no real harm in stopping processing of
> the LSM hooks.  However, the reliance on the individual LSMs to either
> call the capability hooks themselves, or signal the LSM with a return
> value of 1, is fragile and relies on a specific set of LSMs being
> enabled.  This patch is an effort to resolve, or minimize, these
> issues.
>
> Before we discuss the solution, there are a few observations and
> considerations that we need to take into account:
> * BPF LSM registers an implementation for every LSM hook, and that
>   implementation simply returns the hook's default return value, a
>   0 in this case.  We want to ensure that the default BPF LSM behavior
>   results in the capability checks being called.
> * SELinux and Smack do not expect the traditional capability checks
>   to be applied to the xattrs that they "own".
> * SELinux and Smack are currently written in such a way that the
>   xattr capability checks happen before any additional LSM specific
>   access control checks.  SELinux does apply SELinux specific access
>   controls to all xattrs, even those not "owned" by SELinux.
> * IMA and EVM also register xattr hooks but assume that the LSM layer
>   and specific LSMs have already authorized the basic xattr operation.
>
> In order to ensure we perform the capability based access controls
> before the individual LSM access controls, perform only one capability
> access control check for each operation, and clarify the logic around
> applying the capability controls, we need a mechanism to determine if
> any of the enabled LSMs "own" a particular xattr and want to take
> responsibility for controlling access to that xattr.  The solution in
> this patch is to create a new LSM hook, 'inode_xattr_skipcap', that is
> not exported to the rest of the kernel via a security_XXX() function,
> but is used by the LSM layer to determine if a LSM wants to control
> access to a given xattr and avoid the traditional capability controls.
> Registering an inode_xattr_skipcap hook is optional, if a LSM declines
> to register an implementation, or uses an implementation that simply
> returns the default value (0), there is no effect as the LSM continues
> to enforce the capability based controls (unless another LSM takes
> ownership of the xattr).  If none of the LSMs signal that the
> capability checks should be skipped, the capability check is performed
> and if access is granted the individual LSM xattr access control hooks
> are executed, keeping with the DAC-before-LSM convention.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> include/linux/lsm_hook_defs.h |  1 +
> security/security.c           | 70 ++++++++++++++++++++++++-----------
> security/selinux/hooks.c      | 28 ++++++++++----
> security/smack/smack_lsm.c    | 31 +++++++++++++++-
> 4 files changed, 98 insertions(+), 32 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 334e00efbde4..6e54dae3256b 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -144,6 +144,7 @@ LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
> LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
>      struct dentry *dentry, int ia_valid)
> LSM_HOOK(int, 0, inode_getattr, const struct path *path)
> +LSM_HOOK(int, 0, inode_xattr_skipcap, const char *name)
> LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
>      struct dentry *dentry, const char *name, const void *value,
>      size_t size, int flags)
> diff --git a/security/security.c b/security/security.c
> index 7e118858b545..1f5c68e2a62a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2278,7 +2278,20 @@ int security_inode_getattr(const struct path *path)
>   * @size: size of xattr value
>   * @flags: flags
>   *
> - * Check permission before setting the extended attributes.
> + * This hook performs the desired permission checks before setting the extended
> + * attributes (xattrs) on @dentry.  It is important to note that we have some
> + * additional logic before the main LSM implementation calls to detect if we
> + * need to perform an additional capability check at the LSM layer.
> + *
> + * Normally we enforce a capability check prior to executing the various LSM
> + * hook implementations, but if a LSM wants to avoid this capability check,
> + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
> + * xattrs that it wants to avoid the capability check, leaving the LSM fully
> + * responsible for enforcing the access control for the specific xattr.  If all
> + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
> + * or return a 0 (the default return value), the capability check is still
> + * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
> + * check is performed.
>   *
>   * Return: Returns 0 if permission is granted.
>   */
> @@ -2286,20 +2299,20 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
>                 struct dentry *dentry, const char *name,
>                 const void *value, size_t size, int flags)
> {
> -   int ret;
> +   int rc;
>
>     if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>         return 0;
> -   /*
> -    * SELinux and Smack integrate the cap call,
> -    * so assume that all LSMs supplying this call do so.
> -    */
> -   ret = call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
> -               flags);
>
> -   if (ret == 1)
> -       ret = cap_inode_setxattr(dentry, name, value, size, flags);
> -   return ret;
> +   /* enforce the capability checks at the lsm layer, if needed */
> +   if (!call_int_hook(inode_xattr_skipcap, name)) {
> +       rc = cap_inode_setxattr(dentry, name, value, size, flags);
> +       if (rc)
> +           return rc;
> +   }
> +
> +   return call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
> +                flags);
> }
>
> /**
> @@ -2452,26 +2465,39 @@ int security_inode_listxattr(struct dentry *dentry)
>   * @dentry: file
>   * @name: xattr name
>   *
> - * Check permission before removing the extended attribute identified by @name
> - * for @dentry.
> + * This hook performs the desired permission checks before setting the extended
> + * attributes (xattrs) on @dentry.  It is important to note that we have some
> + * additional logic before the main LSM implementation calls to detect if we
> + * need to perform an additional capability check at the LSM layer.
> + *
> + * Normally we enforce a capability check prior to executing the various LSM
> + * hook implementations, but if a LSM wants to avoid this capability check,
> + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
> + * xattrs that it wants to avoid the capability check, leaving the LSM fully
> + * responsible for enforcing the access control for the specific xattr.  If all
> + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
> + * or return a 0 (the default return value), the capability check is still
> + * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
> + * check is performed.
>   *
>   * Return: Returns 0 if permission is granted.
>   */
> int security_inode_removexattr(struct mnt_idmap *idmap,
>                    struct dentry *dentry, const char *name)
> {
> -   int ret;
> +   int rc;
>
>     if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>         return 0;
> -   /*
> -    * SELinux and Smack integrate the cap call,
> -    * so assume that all LSMs supplying this call do so.
> -    */
> -   ret = call_int_hook(inode_removexattr, idmap, dentry, name);
> -   if (ret == 1)
> -       ret = cap_inode_removexattr(idmap, dentry, name);
> -   return ret;
> +
> +   /* enforce the capability checks at the lsm layer, if needed */
> +   if (!call_int_hook(inode_xattr_skipcap, name)) {

Hm, so if it should happen that lsm 2 returns 0 (allow) but lsm 3
has skipcap return 3, and lsm 3 would have returned
1 to deny the remove, we will get an unexpected result.  It feels like
we need a stronger tie between the lsm which allowed and the one
saying skip the capability check.

> +       rc = cap_inode_removexattr(idmap, dentry, name);
> +       if (rc)
> +           return rc;
> +   }
> +
> +   return call_int_hook(inode_removexattr, idmap, dentry, name);
> }
>
> /**
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3448454c82d0..7be385ebf09b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3181,6 +3181,23 @@ static bool has_cap_mac_admin(bool audit)
>     return true;
> }
>
> +/**
> + * selinux_inode_xattr_skipcap - Skip the xattr capability checks?
> + * @name: name of the xattr
> + *
> + * Returns 1 to indicate that SELinux "owns" the access control rights to xattrs
> + * named @name; the LSM layer should avoid enforcing any traditional
> + * capability based access controls on this xattr.  Returns 0 to indicate that
> + * SELinux does not "own" the access control rights to xattrs named @name and is
> + * deferring to the LSM layer for further access controls, including capability
> + * based controls.
> + */
> +static int selinux_inode_xattr_skipcap(const char *name)
> +{
> +   /* require capability check if not a selinux xattr */
> +   return !strcmp(name, XATTR_NAME_SELINUX);
> +}
> +
> static int selinux_inode_setxattr(struct mnt_idmap *idmap,
>                   struct dentry *dentry, const char *name,
>                   const void *value, size_t size, int flags)
> @@ -3192,15 +3209,9 @@ static int selinux_inode_setxattr(struct mnt_idmap *idmap,
>     u32 newsid, sid = current_sid();
>     int rc = 0;
>
> -   if (strcmp(name, XATTR_NAME_SELINUX)) {
> -       rc = cap_inode_setxattr(dentry, name, value, size, flags);
> -       if (rc)
> -           return rc;
> -
> -       /* Not an attribute we recognize, so just check the
> -          ordinary setattr permission. */
> +   /* if not a selinux xattr, only check the ordinary setattr perm */
> +   if (strcmp(name, XATTR_NAME_SELINUX))
>         return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> -   }
>
>     if (!selinux_initialized())
>         return (inode_owner_or_capable(idmap, inode) ? 0 : -EPERM);
> @@ -7185,6 +7196,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>     LSM_HOOK_INIT(inode_permission, selinux_inode_permission),
>     LSM_HOOK_INIT(inode_setattr, selinux_inode_setattr),
>     LSM_HOOK_INIT(inode_getattr, selinux_inode_getattr),
> +   LSM_HOOK_INIT(inode_xattr_skipcap, selinux_inode_xattr_skipcap),
>     LSM_HOOK_INIT(inode_setxattr, selinux_inode_setxattr),
>     LSM_HOOK_INIT(inode_post_setxattr, selinux_inode_post_setxattr),
>     LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 146667937811..6e37df0465a4 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1282,6 +1282,33 @@ static int smack_inode_getattr(const struct path *path)
>     return rc;
> }
>
> +/**
> + * smack_inode_xattr_skipcap - Skip the xattr capability checks?
> + * @name: name of the xattr
> + *
> + * Returns 1 to indicate that Smack "owns" the access control rights to xattrs
> + * named @name; the LSM layer should avoid enforcing any traditional
> + * capability based access controls on this xattr.  Returns 0 to indicate that
> + * Smack does not "own" the access control rights to xattrs named @name and is
> + * deferring to the LSM layer for further access controls, including capability
> + * based controls.
> + */
> +static int smack_inode_xattr_skipcap(const char *name)
> +{
> +   if (strncmp(name, XATTR_SMACK_SUFFIX, strlen(XATTR_SMACK_SUFFIX)))
> +       return 0;
> +
> +   if (strcmp(name, XATTR_NAME_SMACK) == 0 ||
> +       strcmp(name, XATTR_NAME_SMACKIPIN) == 0 ||
> +       strcmp(name, XATTR_NAME_SMACKIPOUT) == 0 ||
> +       strcmp(name, XATTR_NAME_SMACKEXEC) == 0 ||
> +       strcmp(name, XATTR_NAME_SMACKMMAP) == 0 ||
> +       strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0)
> +       return 1;
> +
> +   return 0;
> +}
> +
> /**
>   * smack_inode_setxattr - Smack check for setting xattrs
>   * @idmap: idmap of the mount
> @@ -1325,8 +1352,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
>             size != TRANS_TRUE_SIZE ||
>             strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
>             rc = -EINVAL;
> -   } else
> -       rc = cap_inode_setxattr(dentry, name, value, size, flags);
> +   }
>
>     if (check_priv && !smack_privileged(CAP_MAC_ADMIN))
>         rc = -EPERM;
> @@ -5050,6 +5076,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
>     LSM_HOOK_INIT(inode_permission, smack_inode_permission),
>     LSM_HOOK_INIT(inode_setattr, smack_inode_setattr),
>     LSM_HOOK_INIT(inode_getattr, smack_inode_getattr),
> +   LSM_HOOK_INIT(inode_xattr_skipcap, smack_inode_xattr_skipcap),
>     LSM_HOOK_INIT(inode_setxattr, smack_inode_setxattr),
>     LSM_HOOK_INIT(inode_post_setxattr, smack_inode_post_setxattr),
>     LSM_HOOK_INIT(inode_getxattr, smack_inode_getxattr),
> --
> 2.45.0

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

On May 4, 2024 1:04:57 PM Serge Hallyn <serge@hallyn.com> wrote:
> May 2, 2024 19:59:11 Paul Moore <paul@paul-moore.com>:
>
>> The current security_inode_setxattr() and security_inode_removexattr()
>> hooks rely on individual LSMs to either call into the associated
>> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
>> return a magic value of 1 to indicate that the LSM layer itself should
>> perform the capability checks.  Unfortunately, with the default return
>> value for these LSM hooks being 0, an individual LSM hook returning a
>> 1 will cause the LSM hook processing to exit early, potentially
>> skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
>> of the LSMs which currently register inode xattr hooks should end up
>> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
>> executing last there should be no real harm in stopping processing of
>> the LSM hooks.  However, the reliance on the individual LSMs to either
>> call the capability hooks themselves, or signal the LSM with a return
>> value of 1, is fragile and relies on a specific set of LSMs being
>> enabled.  This patch is an effort to resolve, or minimize, these
>> issues.
>>
>> Before we discuss the solution, there are a few observations and
>> considerations that we need to take into account:
>> * BPF LSM registers an implementation for every LSM hook, and that
>>  implementation simply returns the hook's default return value, a
>>  0 in this case.  We want to ensure that the default BPF LSM behavior
>>  results in the capability checks being called.
>> * SELinux and Smack do not expect the traditional capability checks
>>  to be applied to the xattrs that they "own".
>> * SELinux and Smack are currently written in such a way that the
>>  xattr capability checks happen before any additional LSM specific
>>  access control checks.  SELinux does apply SELinux specific access
>>  controls to all xattrs, even those not "owned" by SELinux.
>> * IMA and EVM also register xattr hooks but assume that the LSM layer
>>  and specific LSMs have already authorized the basic xattr operation.
>>
>> In order to ensure we perform the capability based access controls
>> before the individual LSM access controls, perform only one capability
>> access control check for each operation, and clarify the logic around
>> applying the capability controls, we need a mechanism to determine if
>> any of the enabled LSMs "own" a particular xattr and want to take
>> responsibility for controlling access to that xattr.  The solution in
>> this patch is to create a new LSM hook, 'inode_xattr_skipcap', that is
>> not exported to the rest of the kernel via a security_XXX() function,
>> but is used by the LSM layer to determine if a LSM wants to control
>> access to a given xattr and avoid the traditional capability controls.
>> Registering an inode_xattr_skipcap hook is optional, if a LSM declines
>> to register an implementation, or uses an implementation that simply
>> returns the default value (0), there is no effect as the LSM continues
>> to enforce the capability based controls (unless another LSM takes
>> ownership of the xattr).  If none of the LSMs signal that the
>> capability checks should be skipped, the capability check is performed
>> and if access is granted the individual LSM xattr access control hooks
>> are executed, keeping with the DAC-before-LSM convention.
>>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>> include/linux/lsm_hook_defs.h |  1 +
>> security/security.c           | 70 ++++++++++++++++++++++++-----------
>> security/selinux/hooks.c      | 28 ++++++++++----
>> security/smack/smack_lsm.c    | 31 +++++++++++++++-
>> 4 files changed, 98 insertions(+), 32 deletions(-)
>>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index 334e00efbde4..6e54dae3256b 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -144,6 +144,7 @@ LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap 
>> *idmap, struct dentry *dentry,
>> LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
>>     struct dentry *dentry, int ia_valid)
>> LSM_HOOK(int, 0, inode_getattr, const struct path *path)
>> +LSM_HOOK(int, 0, inode_xattr_skipcap, const char *name)
>> LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
>>     struct dentry *dentry, const char *name, const void *value,
>>     size_t size, int flags)
>> diff --git a/security/security.c b/security/security.c
>> index 7e118858b545..1f5c68e2a62a 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2278,7 +2278,20 @@ int security_inode_getattr(const struct path *path)
>>  * @size: size of xattr value
>>  * @flags: flags
>>  *
>> - * Check permission before setting the extended attributes.
>> + * This hook performs the desired permission checks before setting the 
>> extended
>> + * attributes (xattrs) on @dentry.  It is important to note that we have some
>> + * additional logic before the main LSM implementation calls to detect if we
>> + * need to perform an additional capability check at the LSM layer.
>> + *
>> + * Normally we enforce a capability check prior to executing the various LSM
>> + * hook implementations, but if a LSM wants to avoid this capability check,
>> + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
>> + * xattrs that it wants to avoid the capability check, leaving the LSM fully
>> + * responsible for enforcing the access control for the specific xattr.  
>> If all
>> + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
>> + * or return a 0 (the default return value), the capability check is still
>> + * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
>> + * check is performed.
>>  *
>>  * Return: Returns 0 if permission is granted.
>>  */
>> @@ -2286,20 +2299,20 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
>>                struct dentry *dentry, const char *name,
>>                const void *value, size_t size, int flags)
>> {
>> -   int ret;
>> +   int rc;
>>
>>    if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>>        return 0;
>> -   /*
>> -    * SELinux and Smack integrate the cap call,
>> -    * so assume that all LSMs supplying this call do so.
>> -    */
>> -   ret = call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
>> -               flags);
>>
>> -   if (ret == 1)
>> -       ret = cap_inode_setxattr(dentry, name, value, size, flags);
>> -   return ret;
>> +   /* enforce the capability checks at the lsm layer, if needed */
>> +   if (!call_int_hook(inode_xattr_skipcap, name)) {
>> +       rc = cap_inode_setxattr(dentry, name, value, size, flags);
>> +       if (rc)
>> +           return rc;
>> +   }
>> +
>> +   return call_int_hook(inode_setxattr, idmap, dentry, name, value, size,
>> +                flags);
>> }
>>
>> /**
>> @@ -2452,26 +2465,39 @@ int security_inode_listxattr(struct dentry *dentry)
>>  * @dentry: file
>>  * @name: xattr name
>>  *
>> - * Check permission before removing the extended attribute identified by @name
>> - * for @dentry.
>> + * This hook performs the desired permission checks before setting the 
>> extended
>> + * attributes (xattrs) on @dentry.  It is important to note that we have some
>> + * additional logic before the main LSM implementation calls to detect if we
>> + * need to perform an additional capability check at the LSM layer.
>> + *
>> + * Normally we enforce a capability check prior to executing the various LSM
>> + * hook implementations, but if a LSM wants to avoid this capability check,
>> + * it can register a 'inode_xattr_skipcap' hook and return a value of 1 for
>> + * xattrs that it wants to avoid the capability check, leaving the LSM fully
>> + * responsible for enforcing the access control for the specific xattr.  
>> If all
>> + * of the enabled LSMs refrain from registering a 'inode_xattr_skipcap' hook,
>> + * or return a 0 (the default return value), the capability check is still
>> + * performed.  If no 'inode_xattr_skipcap' hooks are registered the capability
>> + * check is performed.
>>  *
>>  * Return: Returns 0 if permission is granted.
>>  */
>> int security_inode_removexattr(struct mnt_idmap *idmap,
>>                   struct dentry *dentry, const char *name)
>> {
>> -   int ret;
>> +   int rc;
>>
>>    if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>>        return 0;
>> -   /*
>> -    * SELinux and Smack integrate the cap call,
>> -    * so assume that all LSMs supplying this call do so.
>> -    */
>> -   ret = call_int_hook(inode_removexattr, idmap, dentry, name);
>> -   if (ret == 1)
>> -       ret = cap_inode_removexattr(idmap, dentry, name);
>> -   return ret;
>> +
>> +   /* enforce the capability checks at the lsm layer, if needed */
>> +   if (!call_int_hook(inode_xattr_skipcap, name)) {
>
> Hm, so if it should happen that lsm 2 returns 0 (allow) but lsm 3
> has skipcap return 3, and lsm 3 would have returned
> 1 to deny the remove, we will get an unexpected result.  It feels like
> we need a stronger tie between the lsm which allowed and the one
> saying skip the capability check.

That's not an unexpected result, that is a valid outcome in the world of 
LSM stacking. The skipcap check only guarantees that the capability check 
will be skipped if an LSM returns a non-zero value.  The vast majority 
(all?) of the hooks operate as you describe: a LSM towards the back of the 
list can reject an operation that was previous LSM has allowed.  This isn't 
limited to LSMs either, there are plenty of reasons, e.g. transient 
failures, which could cause an operation to fail after being authorized by 
a particular LSM.

A particular LSM can only authorize a requested operation; a successful 
return value from a LSM hook implementation can not guarantee a successful 
operation result.

--
paul-moore.com

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Serge E. Hallyn]

On Sat, May 04, 2024 at 04:26:24PM -0400, Paul Moore wrote:
> On May 4, 2024 1:04:57 PM Serge Hallyn <serge@hallyn.com> wrote:
> > Hm, so if it should happen that lsm 2 returns 0 (allow) but lsm 3
> > has skipcap return 3, and lsm 3 would have returned
> > 1 to deny the remove, we will get an unexpected result.  It feels like
> > we need a stronger tie between the lsm which allowed and the one
> > saying skip the capability check.
> 
> That's not an unexpected result, that is a valid outcome in the world of LSM
> stacking. The skipcap check only guarantees that the capability check will
> be skipped if an LSM returns a non-zero value.  The vast majority (all?) of
> the hooks operate as you describe: a LSM towards the back of the list can
> reject an operation that was previous LSM has allowed.  This isn't limited
> to LSMs either, there are plenty of reasons, e.g. transient failures, which
> could cause an operation to fail after being authorized by a particular LSM.
> 
> A particular LSM can only authorize a requested operation; a successful
> return value from a LSM hook implementation can not guarantee a successful
> operation result.

Ok, thanks.

-serge

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

On Fri, May 3, 2024 at 12:26 PM Paul Moore <paul@paul-moore.com> wrote:
> I also need to track down the appropriate commits for the 'Fixes:'
> tag(s); I'm not entirely convinced that some of the other patches were
> targeting the proper commit ...

Looking at this again, I'm not sure we can easily narrow this down to
one or two commits as the current flawed situation is really the
result of a lot of things.  If I had to pick two may be the addition
of the BPF LSM and the introduction of the LSM hook macros?  I think
this patch may just get a stable tag without an explicit set of
'Fixes', which should be okay.

-- 
paul-moore.com

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

On Mon, May 6, 2024 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, May 3, 2024 at 12:26 PM Paul Moore <paul@paul-moore.com> wrote:
> > I also need to track down the appropriate commits for the 'Fixes:'
> > tag(s); I'm not entirely convinced that some of the other patches were
> > targeting the proper commit ...
>
> Looking at this again, I'm not sure we can easily narrow this down to
> one or two commits as the current flawed situation is really the
> result of a lot of things.  If I had to pick two may be the addition
> of the BPF LSM and the introduction of the LSM hook macros?  I think
> this patch may just get a stable tag without an explicit set of
> 'Fixes', which should be okay.

I merged this patch, with Casey's ACK and a stable tag, into
lsm/dev-staging.  Assuming no issues are uncovered during testing, or
mentioned on-list, I'll plan to merge this into lsm/dev after the
upcoming merge window closes; I'll send another note when that
happens.

-- 
paul-moore.com

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

On Tue, May 7, 2024 at 3:17 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, May 6, 2024 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, May 3, 2024 at 12:26 PM Paul Moore <paul@paul-moore.com> wrote:
> > > I also need to track down the appropriate commits for the 'Fixes:'
> > > tag(s); I'm not entirely convinced that some of the other patches were
> > > targeting the proper commit ...
> >
> > Looking at this again, I'm not sure we can easily narrow this down to
> > one or two commits as the current flawed situation is really the
> > result of a lot of things.  If I had to pick two may be the addition
> > of the BPF LSM and the introduction of the LSM hook macros?  I think
> > this patch may just get a stable tag without an explicit set of
> > 'Fixes', which should be okay.
>
> I merged this patch, with Casey's ACK and a stable tag, into
> lsm/dev-staging.  Assuming no issues are uncovered during testing, or
> mentioned on-list, I'll plan to merge this into lsm/dev after the
> upcoming merge window closes; I'll send another note when that
> happens.

Quick update to let everyone know that I've just merged this into lsm/dev.

-- 
paul-moore.com

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[KP Singh]

> On 3 May 2024, at 02:58, Paul Moore <paul@paul-moore.com> wrote:
> 
> The current security_inode_setxattr() and security_inode_removexattr()
> hooks rely on individual LSMs to either call into the associated
> capability hooks (cap_inode_setxattr() or cap_inode_removexattr()), or
> return a magic value of 1 to indicate that the LSM layer itself should
> perform the capability checks.  Unfortunately, with the default return
> value for these LSM hooks being 0, an individual LSM hook returning a
> 1 will cause the LSM hook processing to exit early, potentially
> skipping a LSM.  Thankfully, with the exception of the BPF LSM, none
> of the LSMs which currently register inode xattr hooks should end up
> returning a value of 1, and in the BPF LSM case, with the BPF LSM hooks
> executing last there should be no real harm in stopping processing of
> the LSM hooks.  However, the reliance on the individual LSMs to either
> call the capability hooks themselves, or signal the LSM with a return
> value of 1, is fragile and relies on a specific set of LSMs being
> enabled.  This patch is an effort to resolve, or minimize, these
> issues.
> 
> Before we discuss the solution, there are a few observations and
> considerations that we need to take into account:
> * BPF LSM registers an implementation for every LSM hook, and that
>  implementation simply returns the hook's default return value, a
>  0 in this case.  We want to ensure that the default BPF LSM behavior
>  results in the capability checks being called.


The BPF LSM never intended to add default callbacks from the very first day:


https://lwn.net/ml/linux-kernel/20200224171305.GA21886@chromium.org/


But, we went ahead with a "compromise" because were were going to make the LSM layer better and tackle this problem with the broader enhancements for the LSM layer. Little did I know this would take 4 years (and counting...) from then.


If you want to go ahead with this change for other reasons, please feel free to. But, I don't want the BPF LSM default callbacks being cited as a reason here.


- KP

> * SELinux and Smack do not expect the traditional capability checks
>  to be applied to the xattrs that they "own".
> * SELinux and Smack are currently written in such a way that the
>  xattr capability checks happen before any additional LSM specific
>  access control checks.  SELinux does apply SELinux specific access
>  controls to all xattrs, even those not "owned" by SELinux.
> * IMA and EVM also register xattr hooks but assume that the LSM layer
>  and specific LSMs have already authorized the basic xattr operation.


[...]

Re: [RFC PATCH] lsm: fixup the inode xattr capability handling

[Paul Moore]

On Fri, Jul 5, 2024 at 4:28 PM KP Singh <kpsingh@chromium.org> wrote:
> > On 3 May 2024, at 02:58, Paul Moore <paul@paul-moore.com> wrote:

...

> > Before we discuss the solution, there are a few observations and
> > considerations that we need to take into account:
> > * BPF LSM registers an implementation for every LSM hook, and that
> >  implementation simply returns the hook's default return value, a
> >  0 in this case.  We want to ensure that the default BPF LSM behavior
> >  results in the capability checks being called.

...

> If you want to go ahead with this change for other reasons, please feel free to. But, I don't want the BPF LSM default callbacks being cited as a reason here.

As mentioned previously in this thread, over a month ago, the patch is
in the lsm/dev branch and is therefore scheduled to go up to Linus
during the next merge window.  It may be worth noting that the current
BPF LSM behavior is cited not as a "reason" but merely as part of the
"observations and considerations" along with the SELinux and Smack
behaviors.  If you look at the full description as well as the patch
itself, you'll notice that the core issue really is more about legacy
SELinux and Smack behaviors, not that of the BPF LSM.  The
considerations section that you highlighted is simply there to provide
some background on how things work to help the reader better
understand the approach taken in the patch.

-- 
paul-moore.com