[PATCH] security: fix no-op hook logic in security_inode_{set, remove}xattr()

[Paul Moore]

On Tue, Jan 30, 2024 at 7:09 PM Paul Moore <paul at paul-moore.com> wrote:
> On Mon, Jan 29, 2024 at 8:31 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> >
> > These two hooks currently work like this:
> > 1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
> >    called.
> > 2. If an LSM hook call returns 0, the loop continues to call further
> >    LSMs (and only stops on an error return value).
> > 3. The "default" return value is 0, so e.g. the default BPF LSM hook
> >    just returns 0.
> >
> > This works if BPF LSM is enabled along with SELinux or SMACK (or not
> > enabled at all), but if it's the only LSM implementing the hook, then
> > the cap_inode_{set,remove}xattr() is erroneously skipped.
> >
> > Fix this by using 1 as the default return value and make the loop
> > recognize it as a no-op return value (i.e. if an LSM returns this value
> > it is treated as if it wasn't called at all). The final logic is similar
> > to that of security_fs_context_parse_param().
> >
> > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
> > ---
> >  include/linux/lsm_hook_defs.h |  4 ++--
> >  security/security.c           | 45 +++++++++++++++++++++++++----------
> >  2 files changed, 35 insertions(+), 14 deletions(-)
>
> Thanks for working on this Ondrej, I've got a couple of thoughts on
> the approach taken here, but we definitely need to fix this.
>
> My first thought is that we really should move the
> cap_inode_setxattr() and cap_inode_removexattr() calls in security.c
> over to using the LSM hook infrastructure just as we do with other
> capability hooks in commoncap.c:
>
>   LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr);
>   LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr);
>
> ... of course we will need to adjust cap_inode_setxattr to take (and
> ignore the idmap) parameter, but that is easy enough.  It looks like
> cap_inode_removexattr() can be used as-is.  Modifications to the only
> two LSMs, SELinux and Smack, which explicitly call out to these
> capability hooks looks rather straightforward as well.  Doing this
> should simplify the LSM hooks significantly, and lower the chance of a
> future LSM mistakenly not doing the required capability calls.  There
> should also be a slight performance bump for the few (one? two?)
> people running both SELinux and Smack in a production environment.
>
> My second thought is that we *really* need to add to the function
> header block comment/description for both these hooks.  Of course the
> details here will change depending on the bits above about the
> capability hooks, but if we need any special handling like you're
> proposing here we really should document it in the hook's header
> block.

A completely untested, other than compiling security/, patch is below
demonstrating what I was thinking.  I've also attached the same patch
in case anyone wants to actually try it out as the cut-n-paste version
below is surely whitespace damaged.  I will warn you that this was
hastily thrown together so it is very likely I screwed something up :)

diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..2d3c0af33b65 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -156,7 +156,8 @@ extern int cap_capset(struct cred *new, const
struct cred *old,
        const kernel_cap_t *inheritable,
        const kernel_cap_t *permitted);
 extern int cap_bprm_creds_from_file(struct linux_binprm *bprm, const
struct file *file);
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+        struct dentry *dentry, const char *name,
         const void *value, size_t size, int flags);
 int cap_inode_removexattr(struct mnt_idmap *idmap,
    struct dentry *dentry, const char *name);
@@ -888,7 +889,7 @@ static inline int security_inode_setxattr(struct
mnt_idmap *idmap,
  struct dentry *dentry, const char *name, const void *value,
  size_t size, int flags)
 {
- return cap_inode_setxattr(dentry, name, value, size, flags);
+ return cap_inode_setxattr(idmap, dentry, name, value, size, flags);
 }

 static inline int security_inode_set_acl(struct mnt_idmap *idmap,
diff --git a/security/commoncap.c b/security/commoncap.c
index 162d96b3a676..34caf0e19b2f 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -975,6 +975,7 @@ int cap_bprm_creds_from_file(struct linux_binprm
*bprm, const struct file *file)

 /**
  * cap_inode_setxattr - Determine whether an xattr may be altered
+ * @idmap: idmap of the mount
  * @dentry: The inode/dentry being altered
  * @name: The name of the xattr to be changed
  * @value: The value that the xattr will be changed to
@@ -987,7 +988,8 @@ int cap_bprm_creds_from_file(struct linux_binprm
*bprm, const struct file *file)
  * This is used to make sure security xattrs don't get updated or set by those
  * who aren't privileged to do so.
  */
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+        struct dentry *dentry, const char *name,
         const void *value, size_t size, int flags)
 {
  struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
@@ -1457,6 +1459,8 @@ static struct security_hook_list
capability_hooks[] __ro_after_init = {
  LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
  LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
  LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity),
+ LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr),
+ LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr),
  LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
  LSM_HOOK_INIT(mmap_file, cap_mmap_file),
  LSM_HOOK_INIT(task_fix_setuid, cap_task_fix_setuid),
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..6425d177b301 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2258,15 +2258,9 @@ int security_inode_setxattr(struct mnt_idmap *idmap,

  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, 1, idmap, dentry, name, value,
-     size, flags);

- if (ret == 1)
- ret = cap_inode_setxattr(dentry, name, value, size, flags);
+ ret = call_int_hook(inode_setxattr, 0, idmap, dentry, name, value,
+     size, flags);
  if (ret)
  return ret;
  ret = ima_inode_setxattr(dentry, name, value, size);
@@ -2421,13 +2415,8 @@ int security_inode_removexattr(struct mnt_idmap *idmap,

  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, 1, idmap, dentry, name);
- if (ret == 1)
- ret = cap_inode_removexattr(idmap, dentry, name);
+
+ ret = call_int_hook(inode_removexattr, 0, idmap, dentry, name);
  if (ret)
  return ret;
  ret = ima_inode_removexattr(dentry, name);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..49cb331a0d84 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3193,10 +3193,6 @@ static int selinux_inode_setxattr(struct
mnt_idmap *idmap,
  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. */
  return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
@@ -3350,10 +3346,6 @@ static int selinux_inode_removexattr(struct
mnt_idmap *idmap,
       struct dentry *dentry, const char *name)
 {
  if (strcmp(name, XATTR_NAME_SELINUX)) {
- int rc = cap_inode_removexattr(idmap, dentry, name);
- if (rc)
- return rc;
-
  /* Not an attribute we recognize, so just check the
     ordinary setattr permission. */
  return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..34b74e442412 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1317,8 +1317,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
  if (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;
@@ -1426,12 +1425,8 @@ static int smack_inode_removexattr(struct
mnt_idmap *idmap,
      strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0 ||
      strcmp(name, XATTR_NAME_SMACKMMAP) == 0) {
  if (!smack_privileged(CAP_MAC_ADMIN))
- rc = -EPERM;
- } else
- rc = cap_inode_removexattr(idmap, dentry, name);
-
- if (rc != 0)
- return rc;
+ return -EPERM;
+ }

  smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
  smk_ad_setfield_u_fs_path_dentry(&ad, dentry);

-- 
paul-moore.com