Date:   Thu, 1 Sep 2022 21:35:05 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Christian Göttsche <cgzones@...glemail.com>
Cc:     selinux@...r.kernel.org, Serge Hallyn <serge@...lyn.com>,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/8] capability: add any wrapper to test for multiple
 caps with exactly one audit message

On Thu, Sep 1, 2022 at 8:56 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Wed, Jun 15, 2022 at 11:27 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 will create an AVC denial.
> > Using this function with the least invasive capability as left most
> > argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers
> > to only allow the least invasive one and SELinux domains pass this check
> > with only capability:sys_nice or capability:sys_admin allowed without
> > any AVC denial message.
> >
> > Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
> >
> > ---
> > 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        | 53 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 63 insertions(+)
>
> ...
>
> > diff --git a/kernel/capability.c b/kernel/capability.c
> > index 765194f5d678..ab9b889c3f4d 100644
> > --- a/kernel/capability.c
> > +++ b/kernel/capability.c
> > @@ -435,6 +435,59 @@ bool ns_capable_setid(struct user_namespace *ns, int cap)
> >  }
> >  EXPORT_SYMBOL(ns_capable_setid);
> >
> > +/**
> > + * 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, i.e. 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 (ns_capable_noaudit(ns, cap1))
> > +               return ns_capable(ns, cap1);
> > +
> > +       if (ns_capable_noaudit(ns, cap2))
> > +               return ns_capable(ns, cap2);
> > +
> > +       return ns_capable(ns, cap1);
>
> I'm slightly concerned that some people are going to be upset about
> making an additional call into the capabilities code with this
> function.  I think we need to be a bit more clever here to take out
> some of the extra work.
>
> I wonder if we create a new capability function, call it
> ns_capable_audittrue(...) or something like that, that only generates
> an audit record if the current task has the requested capability ...

To be clear, when I mean by generating an audit record when true is
that the LSMs implementing the security_capable() hook would only call
their audit related code when the capability requirement was met, in
many cases that will *not* likely generate an audit record, but you
get the basic idea I hope.  For SELinux this would likely mean
modifying cred_has_capability() something like this ...

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 03bca97c8b29..c1b7f0582d16 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1581,6 +1581,7 @@ static int cred_has_capability(const struct cred *cred,
       u16 sclass;
       u32 sid = cred_sid(cred);
       u32 av = CAP_TO_MASK(cap);
+       bool audit;
       int rc;

       ad.type = LSM_AUDIT_DATA_CAP;
@@ -1601,7 +1602,10 @@ static int cred_has_capability(const struct cred *cred,

       rc = avc_has_perm_noaudit(&selinux_state,
                                 sid, sid, sclass, av, 0, &avd);
-       if (!(opts & CAP_OPT_NOAUDIT)) {
+       audit = !(opts & CAP_OPT_NOAUDIT);
+       if (opts & CAP_OPT_AUDITTRUE)
+               audit = !rc;
+       if (audit) {
               int rc2 = avc_audit(&selinux_state,
                                   sid, sid, sclass, av, &avd, rc, &ad);
               if (rc2)

[There is likely a cleaner patch than the above, this was just mean as
a demonstration]

> ... if
> the current task does not have the requested capability no audit
> record is generated.  With this new function I think we could rewrite
> ns_capable_any(...) like this:
>
>   bool ns_capable_any(ns, cap1, cap2)
>   {
>     if (ns_capable_audittrue(ns, cap1))
>       return true;
>     if (ns_capable_audittrue(ns, cap2))
>       return true;
>     return ns_capable(ns, cap1);
>   }
>
> ... we would still have an extra capability check in the failure case,
> but that's an error case anyway and not likely to draw much concern.
>
> Of course this would require some additional work, meaning a new
> CAP_OPT_XXX flag (CAP_OPT_AUDITTRUE?), and updates to the individual
> LSMs.  However, the good news here is that it appears only SELinux and
> AppArmor would need modification (the others don't care about
> capabilities or audit) and in each case the modification to support
> the new CAP_OPT_AUDITTRUE flag look pretty simple.
>
> Thoughts?
>
> > +}
> > +EXPORT_SYMBOL(ns_capable_any);

-- 
paul-moore.com

Powered by blists - more mailing lists