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

Serge Hallyn serge at hallyn.com
Sat May 4 17:04:43 UTC 2024


May 2, 2024 19:59:11 Paul Moore <paul at 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 at 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




More information about the Linux-security-module-archive mailing list