Received-SPF: pass (google.com: domain of linux-kernel+bounces-104783-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240315113828.258005-1-cgzones@googlemail.com>
 <20240315113828.258005-2-cgzones@googlemail.com> <CAEf4BzZF0A9qEzmRigHFLQ4vBQshGUQWZVG5L0q2_--kx4=AXA@mail.gmail.com>
In-Reply-To: <CAEf4BzZF0A9qEzmRigHFLQ4vBQshGUQWZVG5L0q2_--kx4=AXA@mail.gmail.com>
From: =?UTF-8?Q?Christian_G=C3=B6ttsche?= <cgzones@googlemail.com>
Date: Fri, 15 Mar 2024 19:27:22 +0100
Message-ID: <CAJ2a_Dfy3DKnsZZhKUXiMbG-NZqH0APwz3tGF1Fdma+nYcHmOQ@mail.gmail.com>
Subject: Re: [PATCH 02/10] capability: add any wrappers to test for multiple
 caps with exactly one audit message
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: linux-security-module@vger.kernel.org, linux-block@vger.kernel.org, 
	Serge Hallyn <serge@hallyn.com>, linux-kernel@vger.kernel.org, bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, 15 Mar 2024 at 17:46, Andrii Nakryiko <andrii.nakryiko@gmail.com> w=
rote:
>
> On Fri, Mar 15, 2024 at 4:39=E2=80=AFAM Christian G=C3=B6ttsche
> <cgzones@googlemail.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@vger.kernel.org
> > Signed-off-by: Christian G=C3=B6ttsche <cgzones@googlemail.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 adde=
d
> >    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 super=
ior 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 s=
uperior
> > + * capabilities currently available for use, false if not.
> > + *
> > + * In contrast to or'ing capable() this call will create exactly one a=
udit
> > + * message, either for @cap1, if it is granted or both are not permitt=
ed,
> > + * 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 o=
n the
> > + * assumption that it's about to be used.
> > + */
> > +bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2)
> > +{
> > +       if (cap1 =3D=3D 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.

> > +}
> > +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 s=
uperior
> > + * capabilities currently available for use, false if not.
> > + *
> > + * In contrast to or'ing capable() this call will create exactly one a=
udit
> > + * message, either for @cap1, if it is granted or both are not permitt=
ed,
> > + * 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 o=
n 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 i=
n effect
> >   * @cap: The capability to be tested for
> > --
> > 2.43.0
> >
> >