| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 3 Jun 2021 16:29:40 +0100
From: Dimitri John Ledkov <dimitri.ledkov@...onical.com>
To: "Serge E. Hallyn" <serge@...lyn.com>
Cc: linux-kernel@...r.kernel.org, keyrings@...r.kernel.org,
Eric Snowberg <eric.snowberg@...cle.com>,
Jarkko Sakkinen <jarkko@...nel.org>,
David Woodhouse <dwmw2@...radead.org>,
David Howells <dhowells@...hat.com>,
Paul Moore <paul@...l-moore.com>
Subject: Re: [PATCH] integrity: Load mokx certs from the EFI MOK config table
Hi,
On Tue, May 18, 2021 at 3:19 PM Serge E. Hallyn <serge@...lyn.com> 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@...onical.com>
> > cc: keyrings@...r.kernel.org
> > cc: Eric Snowberg <eric.snowberg@...cle.com>
> > cc: Jarkko Sakkinen <jarkko@...nel.org>
> > cc: David Woodhouse <dwmw2@...radead.org>
> > cc: David Howells <dhowells@...hat.com>
>
> Acked-by: Serge Hallyn <serge@...lyn.com>
>
> > ---
> > 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.
Powered by blists - more mailing lists