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

From: Paul Moore <paul@paul-moore.com>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>,
	linux-security-module@vger.kernel.org,  selinux@vger.kernel.org
Subject: Re: [PATCH] security: fix no-op hook logic in security_inode_{set,remove}xattr()
Date: Thu, 1 Feb 2024 18:52:38 -0500	[thread overview]
Message-ID: <CAHC9VhSyNPd4rK+oZE6cDwZTCb3Km_eu-+K8s+X73BJwt8ynuA@mail.gmail.com> (raw)
In-Reply-To: <CAHC9VhRa5q3fvWUD-Dh-d5Udq18XqFwR4AGUzSow6Ur+_TmFrQ@mail.gmail.com>

On Tue, Jan 30, 2024 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
>
> I'll come back to this tomorrow with some fresh eyes.

My apologies, "tomorrow" turned into "the day after tomorrow" (as it
often does) ...

I've been struggling with the idea that there are individual LSMs
still calling into the capability hooks instead of leveraging the LSM
stacking infrastructure, and the "magic" involved to make it all work.
While your patch looks like it should restore proper behavior - that's
good! - I keep thinking that we can, and should, do better.

The only thing that I coming up with is to create two new LSM hooks,
in addition to the existing 'inode_setxattr' hook.  The new LSM hooks
would be 'inode_setxattr_owned' and 'inode_setxattr_cap'.  The _owned
hook would simply check the xattr name and return a positive value if
the LSM "owned" the xattr, e.g. XATTR_NAME_SELINUX for SELinux, and
zero otherwise.  The _cap hook would only be used by the capabilities
code (or something similar), and would match up with
cap_inode_setxattr().  With these two new hooks I think we could do
something like this:

int security_inode_setxattr(...)
{
  owned = false
  hook_loop(inode_setxattr_owned) {
    trc = hook->inode_setxattr_owned(name);
    if (trc > 0) {
      owned = true;
      break;
    }
  }
  if (owned) {
    hook_loop(inode_setxattr) {
      /* run the existing inode_setxattr hooks, e.g. SELinux and Smack */
    }
  } else {
    hook_loop(inode_setxattr_cap) {
      /* run the capability setxattr hooks, e.g. commoncap.c */
    }
  }
}

... with security_inode_removexattr() following a similar pattern.

I will admit that there is some duplication in having to check the
xattr twice (once in _owned, again in inode_setxattr), and the
multiple hook approach is less than ideal, but this seems much less
fragile to me.

Thoughts?

-- 
paul-moore.com