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

[Paul Moore]

On Thu, Feb 1, 2024 at 7:11 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
> On 2/1/2024 3:52 PM, Paul Moore wrote:
> > On Tue, Jan 30, 2024 at 9:19 PM Paul Moore <paul at 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.
>
> Apology for attaching a patch rather than inlining it.
> I've attached patch #38 from the current stacking set.
> It addresses the issue.

Archive link:
https://lore.kernel.org/linux-security-module/20231215221636.105680-39-casey@schaufler-ca.com/

It still relies on checking for special return values, which I'm
starting to sour on as it has been the source of problems in the past.
At some point in the future (likely distant future) I want to spend a
day to see what sort of changes it would take to convert all of the
LSM hooks that return an int value into a 0-on-success,
negative-errno-otherwise format where the negative error codes have no
special meaning to the LSM layer ... and if that would even be
desirable in each case.

> > 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