Re: [PATCH v2] ipe: allow secondary and platform keyrings to install/update policies

["Serge E. Hallyn" <serge@hallyn.com>]

On Fri, Sep 20, 2024 at 09:54:06AM +0200, Luca Boccassi wrote:
> On Fri, 20 Sept 2024 at 04:02, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Sun, Sep 15, 2024 at 11:11:19AM +0200, luca.boccassi@gmail.com wrote:
> > > From: Luca Boccassi <bluca@debian.org>
> > >
> > > The current policy management makes it impossible to use IPE
> > > in a general purpose distribution. In such cases the users are not
> > > building the kernel, the distribution is, and access to the private
> > > key included in the trusted keyring is, for obvious reason, not
> > > available.
> > > This means that users have no way to enable IPE, since there will
> > > be no built-in generic policy, and no access to the key to sign
> > > updates validated by the trusted keyring.
> > >
> > > Just as we do for dm-verity, kernel modules and more, allow the
> > > secondary and platform keyrings to also validate policies. This
> > > allows users enrolling their own keys in UEFI db or MOK to also
> > > sign policies, and enroll them. This makes it sensible to enable
> > > IPE in general purpose distributions, as it becomes usable by
> > > any user wishing to do so. Keys in these keyrings can already
> > > load kernels and kernel modules, so there is no security
> > > downgrade.
> > >
> > > Add a kconfig each, like dm-verity does, but default to enabled if
> > > the dependencies are available.
> > >
> > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > ---
> > > v2: add Kconfig entries following the dm-verity model
> > >     update documentation
> > >
> > >  Documentation/admin-guide/LSM/ipe.rst |  5 ++++-
> > >  security/ipe/Kconfig                  | 19 +++++++++++++++++++
> > >  security/ipe/policy.c                 | 14 +++++++++++++-
> > >  3 files changed, 36 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> > > index d8e7db857a2e..bf5aa97911e1 100644
> > > --- a/security/ipe/policy.c
> > > +++ b/security/ipe/policy.c
> > > @@ -169,9 +169,21 @@ struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> > >                       goto err;
> > >               }
> > >
> > > -             rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len, NULL,
> > > +             rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len,
> > > +#ifdef CONFIG_IPE_POLICY_SIG_SECONDARY_KEYRING
> > > +                                         VERIFY_USE_SECONDARY_KEYRING,
> > > +#else
> > > +                                         NULL,
> > > +#endif
> > >                                           VERIFYING_UNSPECIFIED_SIGNATURE,
> > >                                           set_pkcs7_data, new);
> > > +#ifdef CONFIG_IPE_POLICY_SIG_PLATFORM_KEYRING
> > > +             if (rc == -ENOKEY)
> >
> > If the secondary key *is* there, but returns -EKEYREJECTED,
> > do you want to fall back to trying the platform keyring, or not?
> 
> I like the idea in principle, however for ease of use personally I'd
> prefer if the behaviour was the same as dm-verity, given the close
> relationship - maybe we can start with this version, then I can
> propose the same change in a single series for both components later,
> so that we either change it in both or neither. Would that work?

Sounds good, thanks.

Reviewed-by: Serge Hallyn <serge@hallyn.com>

-serge