[PATCH v9 4/5] eficonfig: add UEFI Secure Boot Key enrollment interface
Masahisa Kojima
masahisa.kojima at linaro.org
Sun Nov 20 00:54:55 CET 2022
On Sat, 19 Nov 2022 at 02:30, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/18/22 10:37, Masahisa Kojima wrote:
> > Hi Heinrhch,
> >
> > On Fri, 18 Nov 2022 at 08:07, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 11/16/22 11:28, 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>
> >>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> >>> ---
> >>> Changes in v9:
> >>> - move file size check, set ret = EFI_INVALID_PARAMETER
> >>>
> >>> Changes in v8:
> >>> - fix missing efi_file_close_int() call
> >>>
> >>> 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 | 246 ++++++++++++++++++++++++++++++++++++++++++
> >>> include/efi_config.h | 5 +
> >>> 4 files changed, 259 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..e9aaf76bf8
> >>> --- /dev/null
> >>> +++ b/cmd/eficonfig_sbkey.c
> >>> @@ -0,0 +1,246 @@
> >>> +// 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)
> >>
> >> The parameter should be u16 *data. You can be more specific than the
> >> eficonfig_entry_func typedef here.
> >
> > I think we can't use u16* data here.
> > This is the callback function called from eficonfig_process_common()
> > when the entry is selected, and it is a generic eficonfig_entry_func
> > function pointer.
> >
> >>
> >>> +{
> >>> + u32 attr;
> >>> + char *buf = NULL;
> >>> + efi_uintn_t size;
> >>> + efi_status_t ret;
> >>> + struct efi_file_handle *root;
> >>> + struct efi_file_handle *f = NULL;
> >>> + struct eficonfig_select_file_info file_info;
> >>> +
> >>> + file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
> >>
> >> The FAT specification has no maximum file path length. Windows 10 made
> >> the 260 character limit for file paths a group policy choice. Why should
> >> we add such a constraint?
> >
> > eficonfig_process_select_file() is commonly used when user selects the
> > file as UEFI load option.
> > In UEFI load option, U-Boot efi_convert_device_path_to_text()
> > implementation has a file path limit by
> > MAX_NODE_LEN(512).
> > The file path size limit in eficonfig is derived from it.
> >
> >>
> >>> + if (!file_info.current_path) {
> >>> + ret = EFI_OUT_OF_RESOURCES;
> >>> + goto out;
> >>> + }
> >>> +
> >>> + ret = eficonfig_process_select_file(&file_info);
> >>
> >> Please, fix the signature of eficonfig_process_select_file().
> >>
> >> If a function expects struct *eficonfig_select_file_info, it should not
> >> have a void * parameter for it.
> >
> > Same as eficonfig_process_enroll_key() above, I think we can only use
> > void* here.
> >
> >>
> >>> + 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;
> >>> +
> >>> + ret = efi_file_size(f, &size);
> >>> + if (ret != EFI_SUCCESS)
> >>> + goto out;
> >>> +
> >>> + if (!size) {
> >>> + eficonfig_print_msg("ERROR! File is empty.");
> >>> + ret = EFI_INVALID_PARAMETER;
> >>> + goto out;
> >>> + }
> >>
> >> Move the non-zero file size check to the file chooser. Choosing a file
> >> and afterwards being informed that it should never have been listed as a
> >> choice is just frustrating.
> >
> > OK, I will move size check ineficonfig_process_select_file().
I'm not sure whether eliminating the empty file from the file list is
good or not,
and adding size check in eficonfig_process_select_file() does not change the
user experience(choose file and afterwards being informed that file is empty).
Let me keep an empty file check here.
> >
> >>
> >>> +
> >>> + buf = calloc(1, size);
> >>
> >> Why should we zero out the buffer?
> >>
> >>> + if (!buf) {
> >>> + ret = EFI_OUT_OF_RESOURCES;
> >>> + goto out;
> >>> + }
> >>> +
> >>> + ret = efi_file_read_int(f, &size, buf);
> >> Don't assume that the driver for the simple file protocol is provided by
> >> U-Boot. It could be provided by a boot service driver that you loaded
> >> via the bootefi command before invoking the eficonfig command.
> >
> > OK, I will call EFI_CALL(f->read()) instead.
>
> This is not the only place to change. You have to avoid
> efi_open_volume_int, efi_file_open_int, efi_file_size. Not only here but
> also inside eficonfig_select_file_handler.
Yes, I will also update.
Thanks,
Masahisa kojima
>
> Best regards
>
> Heinrich
>
>
> > Together with this modification, it is better that the opening file is
> > replaced with
> > efi_file_from_path() as Ilias suggested before.
> >
> > Thanks,
> > Masahisa Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> + 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);
> >>> + if (f)
> >>> + efi_file_close_int(f);
> >>> +
> >>> + /* 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)
> >>> +{
> >>> + u32 i;
> >>> + efi_status_t ret;
> >>> + char header_str[32];
> >>> + struct efimenu *efi_menu;
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(key_config_menu_items); i++)
> >>> + key_config_menu_items[i].data = data;
> >>> +
> >>> + snprintf(header_str, sizeof(header_str), " ** Configure %ls **", (u16 *)data);
> >>> +
> >>> + while (1) {
> >>> + efi_menu = eficonfig_create_fixed_menu(key_config_menu_items,
> >>> + ARRAY_SIZE(key_config_menu_items));
> >>> +
> >>> + ret = eficonfig_process_common(efi_menu, header_str);
> >>> + eficonfig_destroy(efi_menu);
> >>> +
> >>> + if (ret == EFI_ABORTED)
> >>> + break;
> >>> + }
> >>> +
> >>> + /* return to the parent menu */
> >>> + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static const struct eficonfig_item secure_boot_menu_items[] = {
> >>> + {"PK", eficonfig_process_set_secure_boot_key, u"PK"},
> >>> + {"KEK", eficonfig_process_set_secure_boot_key, u"KEK"},
> >>> + {"db", eficonfig_process_set_secure_boot_key, u"db"},
> >>> + {"dbx", eficonfig_process_set_secure_boot_key, u"dbx"},
> >>> + {"Quit", eficonfig_process_quit},
> >>> +};
> >>> +
> >>> +/**
> >>> + * eficonfig_process_secure_boot_config() - display the key list menu
> >>> + *
> >>> + * @data: pointer to the data for each entry
> >>> + * Return: status code
> >>> + */
> >>> +efi_status_t eficonfig_process_secure_boot_config(void *data)
> >>> +{
> >>> + efi_status_t ret;
> >>> + struct efimenu *efi_menu;
> >>> +
> >>> + while (1) {
> >>> + char header_str[64];
> >>> +
> >>> + snprintf(header_str, sizeof(header_str),
> >>> + " ** UEFI Secure Boot Key Configuration (SecureBoot : %s) **",
> >>> + (efi_secure_boot_enabled() ? "ON" : "OFF"));
> >>> +
> >>> + efi_menu = eficonfig_create_fixed_menu(secure_boot_menu_items,
> >>> + ARRAY_SIZE(secure_boot_menu_items));
> >>> + if (!efi_menu) {
> >>> + ret = EFI_OUT_OF_RESOURCES;
> >>> + break;
> >>> + }
> >>> +
> >>> + ret = eficonfig_process_common(efi_menu, header_str);
> >>> + eficonfig_destroy(efi_menu);
> >>> +
> >>> + if (ret == EFI_ABORTED)
> >>> + break;
> >>> + }
> >>> +
> >>> + /* return to the parent menu */
> >>> + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> >>> +
> >>> + return ret;
> >>> +}
> >>> diff --git a/include/efi_config.h b/include/efi_config.h
> >>> index 064f2efe3f..7a02fc68ac 100644
> >>> --- a/include/efi_config.h
> >>> +++ b/include/efi_config.h
> >>> @@ -99,5 +99,10 @@ efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
> >>> char *title, eficonfig_entry_func func,
> >>> void *data);
> >>> efi_status_t eficonfig_append_quit_entry(struct efimenu *efi_menu);
> >>> +void *eficonfig_create_fixed_menu(const struct eficonfig_item *items, int count);
> >>> +
> >>> +#ifdef CONFIG_EFI_SECURE_BOOT
> >>> +efi_status_t eficonfig_process_secure_boot_config(void *data);
> >>> +#endif
> >>>
> >>> #endif
> >>
>
More information about the U-Boot
mailing list