[PATCH v7 4/5] eficonfig: add UEFI Secure Boot Key enrollment interface
Masahisa Kojima
masahisa.kojima at linaro.org
Thu Nov 10 03:42:14 CET 2022
Hi Ilias,
On Thu, 10 Nov 2022 at 01:04, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Kojima-san
>
>
> On Wed, Nov 09, 2022 at 12:37:27PM +0900, Masahisa Kojima wrote:
> > This commit adds the menu-driven UEFI Secure Boot Key
> > enrollment interface. User can enroll PK, KEK, db
> > and dbx by selecting file.
> > Only the signed EFI Signature List(s) with an authenticated
> > header, typically '.auth' file, is accepted.
> >
> > To clear the PK, KEK, db and dbx, user needs to enroll the null key
> > signed by PK or KEK.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > ---
> > Changes in v7:
> > - only accept .auth file.
> > - remove creating time based authenticated variable
> > - update commit message
> > - use efi_file_size()
> >
> > Changes in v6:
> > - use efi_secure_boot_enabled()
> > - replace with WIN_CERT_REVISION_2_0 from pe.h
> > - call efi_build_signature_store() to check the valid EFI Signature List
> > - update comment
> >
> > Changes in v4:
> > - add CONFIG_EFI_MM_COMM_TEE dependency
> > - fix error handling
> >
> > Changes in v3:
> > - fix error handling
> >
> > Changes in v2:
> > - allow to enroll .esl file
> > - fix typos
> > - add function comments
> >
> > cmd/Makefile | 5 +
> > cmd/eficonfig.c | 3 +
> > cmd/eficonfig_sbkey.c | 242 ++++++++++++++++++++++++++++++++++++++++++
> > include/efi_config.h | 5 +
> > 4 files changed, 255 insertions(+)
> > create mode 100644 cmd/eficonfig_sbkey.c
> >
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 2444d116c0..0b6a96c1d9 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -66,6 +66,11 @@ obj-$(CONFIG_CMD_EEPROM) += eeprom.o
> > obj-$(CONFIG_EFI) += efi.o
> > obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
> > obj-$(CONFIG_CMD_EFICONFIG) += eficonfig.o
> > +ifdef CONFIG_CMD_EFICONFIG
> > +ifdef CONFIG_EFI_MM_COMM_TEE
> > +obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o
> > +endif
> > +endif
> > obj-$(CONFIG_CMD_ELF) += elf.o
> > obj-$(CONFIG_CMD_EROFS) += erofs.o
> > obj-$(CONFIG_HUSH_PARSER) += exit.o
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 12babb76c2..d79194794b 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -2435,6 +2435,9 @@ static const struct eficonfig_item maintenance_menu_items[] = {
> > {"Edit Boot Option", eficonfig_process_edit_boot_option},
> > {"Change Boot Order", eficonfig_process_change_boot_order},
> > {"Delete Boot Option", eficonfig_process_delete_boot_option},
> > +#if (CONFIG_IS_ENABLED(EFI_SECURE_BOOT) && CONFIG_IS_ENABLED(EFI_MM_COMM_TEE))
> > + {"Secure Boot Configuration", eficonfig_process_secure_boot_config},
> > +#endif
> > {"Quit", eficonfig_process_quit},
> > };
> >
> > diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> > new file mode 100644
> > index 0000000000..1e9eb3f51e
> > --- /dev/null
> > +++ b/cmd/eficonfig_sbkey.c
> > @@ -0,0 +1,242 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Menu-driven UEFI Secure Boot Key Maintenance
> > + *
> > + * Copyright (c) 2022 Masahisa Kojima, Linaro Limited
> > + */
> > +
> > +#include <ansi.h>
> > +#include <common.h>
> > +#include <charset.h>
> > +#include <hexdump.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <menu.h>
> > +#include <efi_loader.h>
> > +#include <efi_config.h>
> > +#include <efi_variable.h>
> > +#include <crypto/pkcs7_parser.h>
> > +
> > +enum efi_sbkey_signature_type {
> > + SIG_TYPE_X509 = 0,
> > + SIG_TYPE_HASH,
> > + SIG_TYPE_CRL,
> > + SIG_TYPE_RSA2048,
> > +};
> > +
> > +struct eficonfig_sigtype_to_str {
> > + efi_guid_t sig_type;
> > + char *str;
> > + enum efi_sbkey_signature_type type;
> > +};
> > +
> > +static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
> > + {EFI_CERT_X509_GUID, "X509", SIG_TYPE_X509},
> > + {EFI_CERT_SHA256_GUID, "SHA256", SIG_TYPE_HASH},
> > + {EFI_CERT_X509_SHA256_GUID, "X509_SHA256 CRL", SIG_TYPE_CRL},
> > + {EFI_CERT_X509_SHA384_GUID, "X509_SHA384 CRL", SIG_TYPE_CRL},
> > + {EFI_CERT_X509_SHA512_GUID, "X509_SHA512 CRL", SIG_TYPE_CRL},
> > + /* U-Boot does not support the following signature types */
> > +/* {EFI_CERT_RSA2048_GUID, "RSA2048", SIG_TYPE_RSA2048}, */
> > +/* {EFI_CERT_RSA2048_SHA256_GUID, "RSA2048_SHA256", SIG_TYPE_RSA2048}, */
> > +/* {EFI_CERT_SHA1_GUID, "SHA1", SIG_TYPE_HASH}, */
> > +/* {EFI_CERT_RSA2048_SHA_GUID, "RSA2048_SHA", SIG_TYPE_RSA2048 }, */
> > +/* {EFI_CERT_SHA224_GUID, "SHA224", SIG_TYPE_HASH}, */
> > +/* {EFI_CERT_SHA384_GUID, "SHA384", SIG_TYPE_HASH}, */
> > +/* {EFI_CERT_SHA512_GUID, "SHA512", SIG_TYPE_HASH}, */
> > +};
> > +
> > +/**
> > + * file_have_auth_header() - check file has EFI_VARIABLE_AUTHENTICATION_2 header
> > + * @buf: pointer to file
> > + * @size: file size
> > + * Return: true if file has auth header, false otherwise
> > + */
> > +static bool file_have_auth_header(void *buf, efi_uintn_t size)
> > +{
> > + struct efi_variable_authentication_2 *auth = buf;
> > +
> > + if (auth->auth_info.hdr.wCertificateType != WIN_CERT_TYPE_EFI_GUID)
> > + return false;
> > +
> > + if (guidcmp(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/**
> > + * eficonfig_process_enroll_key() - enroll key into signature database
> > + *
> > + * @data: pointer to the data for each entry
> > + * Return: status code
> > + */
> > +static efi_status_t eficonfig_process_enroll_key(void *data)
> > +{
> > + u32 attr;
> > + char *buf = NULL;
> > + efi_uintn_t size;
> > + efi_status_t ret;
> > + struct efi_file_handle *f;
> > + struct efi_file_handle *root;
> > + struct eficonfig_select_file_info file_info;
> > +
> > + file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
> > + if (!file_info.current_path) {
> > + ret = EFI_OUT_OF_RESOURCES;
> > + goto out;
> > + }
> > +
> > + ret = eficonfig_process_select_file(&file_info);
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > + ret = efi_open_volume_int(file_info.current_volume, &root);
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > + ret = efi_file_open_int(root, &f, file_info.current_path, EFI_FILE_MODE_READ, 0);
> > + if (ret != EFI_SUCCESS)
> > + goto out;
>
> I think it would be better here if we could use efi_file_from_path().
> I think we can't easily do that atm since we can't convert the filename to
> a device path with efi_dp_from_file() since we don't have the block info.
Here we have a device path of volume(file_info.current_volume) and
filename(file_info.current_path), so we can create a full device path to call
efi_file_from_path().
# cmd/eficonfig.c::create_selected_device_path() create the full device path,
we can reuse it.
>
> Since that requires a further clean up, I am fine keeping it as-is for now,
> but add a comment saying we should replace that with efi_file_from_path()
> eventually.
Probably I don't understand what is improved when we replace current code
with efi_file_from_path().
>
> > +
> > + ret = efi_file_size(f, &size);
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > + buf = calloc(1, size);
> > + if (!buf) {
> > + ret = EFI_OUT_OF_RESOURCES;
> > + goto out;
> > + }
> > + if (size == 0) {
> > + eficonfig_print_msg("ERROR! File is empty.");
> > + goto out;
> > + }
> > +
> > + ret = efi_file_read_int(f, &size, buf);
> > + if (ret != EFI_SUCCESS) {
> > + eficonfig_print_msg("ERROR! Failed to read file.");
> > + goto out;
> > + }
> > + if (!file_have_auth_header(buf, size)) {
> > + eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + attr = EFI_VARIABLE_NON_VOLATILE |
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> > +
> > + /* PK can enroll only one certificate */
> > + if (u16_strcmp(data, u"PK")) {
> > + efi_uintn_t db_size = 0;
> > +
> > + /* check the variable exists. If exists, add APPEND_WRITE attribute */
> > + ret = efi_get_variable_int(data, efi_auth_var_get_guid(data), NULL,
> > + &db_size, NULL, NULL);
> > + if (ret == EFI_BUFFER_TOO_SMALL)
> > + attr |= EFI_VARIABLE_APPEND_WRITE;
> > + }
> > +
> > + ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
> > + attr, size, buf, false);
> > + if (ret != EFI_SUCCESS)
> > + eficonfig_print_msg("ERROR! Failed to update signature database");
> > +
> > +out:
> > + free(file_info.current_path);
> > + free(buf);
>
> Shouldn't we close the file handle here as well?
Yes, thank you for pointing this out.
Regards,
Masahisa Kojima
>
> > +
> > + /* return to the parent menu */
> > + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> > +
> > + return ret;
> > +}
> > +
> > +static struct eficonfig_item key_config_menu_items[] = {
> > + {"Enroll New Key", eficonfig_process_enroll_key},
> > + {"Quit", eficonfig_process_quit},
> > +};
> > +
> > +/**
> > + * eficonfig_process_set_secure_boot_key() - display the key configuration menu
> > + *
> > + * @data: pointer to the data for each entry
> > + * Return: status code
> > + */
> > +static efi_status_t eficonfig_process_set_secure_boot_key(void *data)
>
> [...]
>
More information about the U-Boot
mailing list