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