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

[Paul Moore]

On Tue, Jan 30, 2024 at 7:33 PM Paul Moore <paul at paul-moore.com> wrote:
> 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 built a kernel and did a quick test that failed spectacularly :)

Without looking too closely, I'm guessing I forgot to take into
account that SELinux and Smack don't normally apply the capability
checks if it is one of their xattrs, and installing the capability
checks as hooks means they are always checked regardless of the other
LSMs.

Bummer.

I'll come back to this tomorrow with some fresh eyes.

-- 
paul-moore.com