Re: [PATCH] integrity: Load mokx certs from the EFI MOK config table

[Dimitri John Ledkov]

Hi,

On Tue, May 18, 2021 at 3:19 PM Serge E. Hallyn <serge@xxxxxxxxxx> wrote:
>
> On Wed, May 12, 2021 at 04:31:00PM +0100, Dimitri John Ledkov wrote:
> > Refactor load_moklist_certs() to load either MokListRT into db, or
> > MokListXRT into dbx. Call load_moklist_certs() twice - first to load
> > mokx certs into dbx, then mok certs into db.
> >
> > This thus now attempts to load mokx certs via the EFI MOKvar config
> > table first, and if that fails, via the EFI variable. Previously mokx
> > certs were only loaded via the EFI variable. Which fails when
> > MokListXRT is large. Instead of large MokListXRT variable, only
> > MokListXRT{1,2,3} are available which are not loaded. This is the case
> > with Ubuntu's 15.4 based shim. This patch is required to address
> > CVE-2020-26541 when certificates are revoked via MokListXRT.
>
> Hi Dimitri,
>
> I don't see any problems with this technically, and based on
> https://bugs.launchpad.net/bugs/1928679 it looks like it is in fact needed.
>

Yes it is.

Also it has been pointed out to me now that debian is already applying
a variation of this bugfix at

https://salsa.debian.org/kernel-team/linux/-/blob/master/debian/patches/features/all/db-mok-keyring/0002-MODSIGN-load-blacklist-from-MOKx.patch

The path there is similar to mine, but passes the various static
values as args without a struct. Which looks clean enough.

I can do a variation of a rebased patch similar to what Ben is already
applying in Debian. I reached out to them, and they are not interested
in upstreaming this themselves right now.


> I suggest a change below, but I'm not sure you should make the change
> without confirmation from one of the maintainers that it matches what
> they would want.
>
> > Fixes: ebd9c2ae369a ("integrity: Load mokx variables into the blacklist keyring")
> >
> > Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@xxxxxxxxxxxxx>
> > cc: keyrings@xxxxxxxxxxxxxxx
> > cc: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
> > cc: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> > cc: David Howells <dhowells@xxxxxxxxxx>
>
> Acked-by: Serge Hallyn <serge@xxxxxxxxxx>
>
> > ---
> >  security/integrity/platform_certs/load_uefi.c | 74 ++++++++++---------
> >  1 file changed, 40 insertions(+), 34 deletions(-)
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > index f290f78c3f30..f4b913ec76e4 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -66,17 +66,18 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> >  }
> >
> >  /*
> > - * load_moklist_certs() - Load MokList certs
> > + * load_moklist_certs() - Load Mok(X)List certs
> > + * @load_db: Load MokListRT into db when true; MokListXRT into dbx when false
> >   *
> > - * Load the certs contained in the UEFI MokListRT database into the
> > - * platform trusted keyring.
> > + * Load the certs contained in the UEFI MokList(X)RT database into the
> > + * platform trusted/denied keyring.
> >   *
> >   * This routine checks the EFI MOK config table first. If and only if
> > - * that fails, this routine uses the MokListRT ordinary UEFI variable.
> > + * that fails, this routine uses the MokList(X)RT ordinary UEFI variable.
> >   *
> >   * Return:   Status
> >   */
> > -static int __init load_moklist_certs(void)
> > +static int __init load_moklist_certs(const bool load_db)
>
> Rather than passing in a bool here, I would suggest, above this function,
> doing something like:
>
> struct moklist_desc {
>         const char *mokvar_name;
>         efi_char16_t *efivar_name;
>         const char *parse_mokvar_name;
>         const char *parse_efivar_name;
>         efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *);
> };
>
> struct moklist_desc moklist_descriptor = {
>         .mokvar_name = "MokListRT",
>         .efivar_name = L"MokListRT",
>         .parse_mokvar_name =  "UEFI:MokListRT (MOKvar table)",
>         .parse_efivar_name = "UEFI:MokListRT",
>         .get_handler_for_guid = get_handler_for_db,
> };
>
> struct moklist_desc moxklist_descriptor = {
>         .mokvar_name = "MokListXRT",
>         .efivar_name = L"MokListXRT",
>         .parse_mokvar_name =  "UEFI:MokListXRT (MOKvar table)",
>         .parse_efivar_name = "UEFI:MokListXRT",
>         .get_handler_for_guid = get_handler_for_dbx,
> };
>
> And pass each of those into load_moklist_certs() once.
>
> >  {
> >       struct efi_mokvar_table_entry *mokvar_entry;
> >       efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> > @@ -84,41 +85,55 @@ static int __init load_moklist_certs(void)
> >       unsigned long moksize;
> >       efi_status_t status;
> >       int rc;
> > +     const char *mokvar_name = "MokListRT";
> > +     /* Should be const, but get_cert_list() doesn't have it as const yet */
> > +     efi_char16_t *efivar_name = L"MokListRT";
> > +     const char *parse_mokvar_name = "UEFI:MokListRT (MOKvar table)";
> > +     const char *parse_efivar_name = "UEFI:MokListRT";
> > +     efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *) = get_handler_for_db;
> > +
> > +     if (!load_db) {
> > +             mokvar_name = "MokListXRT";
> > +             efivar_name = L"MokListXRT";
> > +             parse_mokvar_name = "UEFI:MokListXRT (MOKvar table)";
> > +             parse_efivar_name = "UEFI:MokListXRT";
> > +             get_handler_for_guid = get_handler_for_dbx;
> > +     }
> >
> >       /* First try to load certs from the EFI MOKvar config table.
> >        * It's not an error if the MOKvar config table doesn't exist
> >        * or the MokListRT entry is not found in it.
> >        */
> > -     mokvar_entry = efi_mokvar_entry_find("MokListRT");
> > +     mokvar_entry = efi_mokvar_entry_find(mokvar_name);
> >       if (mokvar_entry) {
> > -             rc = parse_efi_signature_list("UEFI:MokListRT (MOKvar table)",
> > +             rc = parse_efi_signature_list(parse_mokvar_name,
> >                                             mokvar_entry->data,
> >                                             mokvar_entry->data_size,
> > -                                           get_handler_for_db);
> > +                                           get_handler_for_guid);
> >               /* All done if that worked. */
> >               if (!rc)
> >                       return rc;
> >
> > -             pr_err("Couldn't parse MokListRT signatures from EFI MOKvar config table: %d\n",
> > -                    rc);
> > +             pr_err("Couldn't parse %s signatures from EFI MOKvar config table: %d\n",
> > +                    mokvar_name, rc);
> >       }
> >
> >       /* Get MokListRT. It might not exist, so it isn't an error
> >        * if we can't get it.
> >        */
> > -     mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
> > +     mok = get_cert_list(efivar_name, &mok_var, &moksize, &status);
> >       if (mok) {
> > -             rc = parse_efi_signature_list("UEFI:MokListRT",
> > -                                           mok, moksize, get_handler_for_db);
> > +             rc = parse_efi_signature_list(parse_efivar_name,
> > +                                           mok, moksize, get_handler_for_guid);
> >               kfree(mok);
> >               if (rc)
> > -                     pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
> > +                     pr_err("Couldn't parse %s signatures: %d\n", mokvar_name, rc);
> >               return rc;
> >       }
> >       if (status == EFI_NOT_FOUND)
> > -             pr_debug("MokListRT variable wasn't found\n");
> > +             pr_debug("%s variable wasn't found\n", mokvar_name);
> >       else
> > -             pr_info("Couldn't get UEFI MokListRT\n");
> > +             pr_info("Couldn't get UEFI %s\n", mokvar_name);
> >       return 0;
> >  }
> >
> > @@ -132,9 +147,8 @@ static int __init load_moklist_certs(void)
> >  static int __init load_uefi_certs(void)
> >  {
> >       efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
> > -     efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> > -     void *db = NULL, *dbx = NULL, *mokx = NULL;
> > -     unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
> > +     void *db = NULL, *dbx = NULL;
> > +     unsigned long dbsize = 0, dbxsize = 0;
> >       efi_status_t status;
> >       int rc = 0;
> >
> > @@ -176,23 +190,15 @@ static int __init load_uefi_certs(void)
> >               kfree(dbx);
> >       }
> >
> > -     mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
> > -     if (!mokx) {
> > -             if (status == EFI_NOT_FOUND)
> > -                     pr_debug("mokx variable wasn't found\n");
> > -             else
> > -                     pr_info("Couldn't get mokx list\n");
> > -     } else {
> > -             rc = parse_efi_signature_list("UEFI:MokListXRT",
> > -                                           mokx, mokxsize,
> > -                                           get_handler_for_dbx);
> > -             if (rc)
> > -                     pr_err("Couldn't parse mokx signatures %d\n", rc);
> > -             kfree(mokx);
> > -     }
> > +     /* Load the MokListXRT certs */
> > +     rc = load_moklist_certs(false);
> > +     if (rc)
> > +             pr_err("Couldn't parse mokx signatures: %d\n", rc);
> >
> >       /* Load the MokListRT certs */
> > -     rc = load_moklist_certs();
> > +     rc = load_moklist_certs(true);
> > +     if (rc)
> > +             pr_err("Couldn't parse mok signatures: %d\n", rc);
> >
> >       return rc;
> >  }
> > --
> > 2.27.0
> >



-- 
Regards,

Dimitri.