diff mbox series

ipe: allow secondary and platform keyrings to install/update policies

Message ID 20240913234840.1318655-1-luca.boccassi@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series ipe: allow secondary and platform keyrings to install/update policies | expand

Commit Message

Luca Boccassi Sept. 13, 2024, 11:48 p.m. UTC 
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.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 security/ipe/policy.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Fan Wu Sept. 15, 2024, 4:27 a.m. UTC | #1 
On 9/13/2024 4:48 PM, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
> 
...
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>   security/ipe/policy.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> index d8e7db857a2e..cac304dc86c2 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -169,9 +169,15 @@ 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,
> +					    VERIFY_USE_SECONDARY_KEYRING,
>   					    VERIFYING_UNSPECIFIED_SIGNATURE,
>   					    set_pkcs7_data, new);
> +		if (rc == -ENOKEY)
> +			rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len,
> +						    VERIFY_USE_PLATFORM_KEYRING,
> +						    VERIFYING_UNSPECIFIED_SIGNATURE,
> +						    set_pkcs7_data, new);
>   		if (rc)
>   			goto err;
>   	} else {

Hi Luca,

This patch looks good to me. My only thought is, like what we currently 
have for dm-verity, should we also add two kconfigs to enable to use 
these two keyrings respectively?

-Fan
Luca Boccassi Sept. 15, 2024, 9:14 a.m. UTC | #2 
On Sun, 15 Sept 2024 at 06:27, Fan Wu <wufan@linux.microsoft.com> wrote:
>
>
>
> On 9/13/2024 4:48 PM, luca.boccassi@gmail.com wrote:
> > From: Luca Boccassi <bluca@debian.org>
> >
> ...
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> >   security/ipe/policy.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> > index d8e7db857a2e..cac304dc86c2 100644
> > --- a/security/ipe/policy.c
> > +++ b/security/ipe/policy.c
> > @@ -169,9 +169,15 @@ 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,
> > +                                         VERIFY_USE_SECONDARY_KEYRING,
> >                                           VERIFYING_UNSPECIFIED_SIGNATURE,
> >                                           set_pkcs7_data, new);
> > +             if (rc == -ENOKEY)
> > +                     rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len,
> > +                                                 VERIFY_USE_PLATFORM_KEYRING,
> > +                                                 VERIFYING_UNSPECIFIED_SIGNATURE,
> > +                                                 set_pkcs7_data, new);
> >               if (rc)
> >                       goto err;
> >       } else {
>
> Hi Luca,
>
> This patch looks good to me. My only thought is, like what we currently
> have for dm-verity, should we also add two kconfigs to enable to use
> these two keyrings respectively?

Sure no problem, sent v2 with the added kconfigs.

Unlike dm-verity the default is "yes" for both though: it's been quite
a pain to get all distributions to enable the dm-verity kconfigs for
the keyrings, given they are quite hidden and a niche use case and
general distro maintainers don't go and look for these options, but
likewise it made signed dm-verity unusable on general purpose distros
without them. I'd like to avoid repeating the same plight. This way if
one doesn't want them they can disable them, but the default if IPE is
enabled is to use them.
diff mbox series

Patch

diff --git a/security/ipe/policy.c b/security/ipe/policy.c
index d8e7db857a2e..cac304dc86c2 100644
--- a/security/ipe/policy.c
+++ b/security/ipe/policy.c
@@ -169,9 +169,15 @@  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,
+					    VERIFY_USE_SECONDARY_KEYRING,
 					    VERIFYING_UNSPECIFIED_SIGNATURE,
 					    set_pkcs7_data, new);
+		if (rc == -ENOKEY)
+			rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len,
+						    VERIFY_USE_PLATFORM_KEYRING,
+						    VERIFYING_UNSPECIFIED_SIGNATURE,
+						    set_pkcs7_data, new);
 		if (rc)
 			goto err;
 	} else {