Message-ID: <CAEf4Bzaw8jV8KxHcO71MCkqA9e2vE9f0XqkGY2CUgbRFiUzjng@mail.gmail.com>
Date: Fri, 15 Mar 2024 11:30:34 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Christian Göttsche <cgzones@...glemail.com>
Cc: linux-security-module@...r.kernel.org, linux-block@...r.kernel.org, 
	Serge Hallyn <serge@...lyn.com>, linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH 02/10] capability: add any wrappers to test for multiple
 caps with exactly one audit message

On Fri, Mar 15, 2024 at 11:27 AM Christian Göttsche
<cgzones@...glemail.com> wrote:
>
> On Fri, 15 Mar 2024 at 17:46, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >
> > On Fri, Mar 15, 2024 at 4:39 AM Christian Göttsche
> > <cgzones@...glemail.com> wrote:
> > >
> > > Add the interfaces `capable_any()` and `ns_capable_any()` as an
> > > alternative to multiple `capable()`/`ns_capable()` calls, like
> > > `capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of
> > > `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`.
> > >
> > > `capable_any()`/`ns_capable_any()` will in particular generate exactly
> > > one audit message, either for the left most capability in effect or, if
> > > the task has none, the first one.
> > >
> > > This is especially helpful with regard to SELinux, where each audit
> > > message about a not allowed capability request will create a denial
> > > message.  Using this new wrapper with the least invasive capability as
> > > left most argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables
> > > policy writers to only grant the least invasive one for the particular
> > > subject instead of both.
> > >
> > > CC: linux-block@...r.kernel.org
> > > Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
> > > ---
> > > v5:
> > >    - add check for identical passed capabilities
> > >    - rename internal helper according to flag rename to
> > >      ns_capable_noauditondeny()
> > > v4:
> > >    Use CAP_OPT_NODENYAUDIT via added ns_capable_nodenyaudit()
> > > v3:
> > >    - rename to capable_any()
> > >    - fix typo in function documentation
> > >    - add ns_capable_any()
> > > v2:
> > >    avoid varargs and fix to two capabilities; capable_or3() can be added
> > >    later if needed
> > > ---
> > >  include/linux/capability.h | 10 ++++++
> > >  kernel/capability.c        | 73 ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 83 insertions(+)
> > >
> >
> > [...]
> >
> > >
> > > +/**
> > > + * ns_capable_any - Determine if the current task has one of two superior capabilities in effect
> > > + * @ns:  The usernamespace we want the capability in
> > > + * @cap1: The capabilities to be tested for first
> > > + * @cap2: The capabilities to be tested for secondly
> > > + *
> > > + * Return true if the current task has at least one of the two given superior
> > > + * capabilities currently available for use, false if not.
> > > + *
> > > + * In contrast to or'ing capable() this call will create exactly one audit
> > > + * message, either for @cap1, if it is granted or both are not permitted,
> > > + * or @cap2, if it is granted while the other one is not.
> > > + *
> > > + * The capabilities should be ordered from least to most invasive, ie. CAP_SYS_ADMIN last.
> > > + *
> > > + * This sets PF_SUPERPRIV on the task if the capability is available on the
> > > + * assumption that it's about to be used.
> > > + */
> > > +bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2)
> > > +{
> > > +       if (cap1 == cap2)
> > > +               return ns_capable(ns, cap1);
> > > +
> > > +       if (ns_capable_noauditondeny(ns, cap1))
> > > +               return true;
> > > +
> > > +       if (ns_capable_noauditondeny(ns, cap2))
> > > +               return true;
> > > +
> > > +       return ns_capable(ns, cap1);
> >
> > this will incur an extra capable() check (with all the LSMs involved,
> > etc), and so for some cases where capability is expected to not be
> > present, this will be a regression. Is there some way to not redo the
> > check, but just audit the failure? At this point we do know that cap1
> > failed before, so might as well just log that.
>
> Logging the failure is quite different in AppArmor and SELinux, so
> just log might not be so easy.
> One option would be to change the entire LSM hook security_capable()
> to take two capability arguments, and let the LSMs handle the any
> logic.

that sounds like an even bigger overkill, probably not worth it

>
> > > +}
> > > +EXPORT_SYMBOL(ns_capable_any);
> > > +
> > > +/**
> > > + * capable_any - Determine if the current task has one of two superior capabilities in effect
> > > + * @cap1: The capabilities to be tested for first
> > > + * @cap2: The capabilities to be tested for secondly
> > > + *
> > > + * Return true if the current task has at least one of the two given superior
> > > + * capabilities currently available for use, false if not.
> > > + *
> > > + * In contrast to or'ing capable() this call will create exactly one audit
> > > + * message, either for @cap1, if it is granted or both are not permitted,
> > > + * or @cap2, if it is granted while the other one is not.
> > > + *
> > > + * The capabilities should be ordered from least to most invasive, ie. CAP_SYS_ADMIN last.
> > > + *
> > > + * This sets PF_SUPERPRIV on the task if the capability is available on the
> > > + * assumption that it's about to be used.
> > > + */
> > > +bool capable_any(int cap1, int cap2)
> > > +{
> > > +       return ns_capable_any(&init_user_ns, cap1, cap2);
> > > +}
> > > +EXPORT_SYMBOL(capable_any);
> > > +
> > >  /**
> > >   * capable - Determine if the current task has a superior capability in effect
> > >   * @cap: The capability to be tested for
> > > --
> > > 2.43.0
> > >
> > >

Powered by blists - more mailing lists