On Thu, Sep 1, 2022 at 8:56 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Wed, Jun 15, 2022 at 11:27 AM Christian Göttsche
> <cgzones@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >
> > ---
> > 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
