On Sun, 23 Oct 2022 at 18:50, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 10/23/22 10:17, Heinrich Schuchardt wrote:
> > On 10/14/22 08:56, Masahisa Kojima wrote:
> >> This commit adds the menu-driven UEFI Secure Boot Key
> >> enrollment interface. User can enroll the PK, KEK, db
> >> and dbx by selecting EFI Signature Lists file.
> >> After the PK is enrolled, UEFI Secure Boot is enabled and
> >> EFI Signature Lists file must be signed by KEK or PK.
> >>
> >> Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org>
> >> ---
> >> Changes in v3:
> >> - fix error handling
> >>
> >> Changes in v2:
> >> - allow to enroll .esl file
> >> - fix typos
> >> - add function comments
> >>
> >> cmd/Makefile | 3 +
> >> cmd/eficonfig.c | 3 +
> >> cmd/eficonfig_sbkey.c | 357 ++++++++++++++++++++++++++++++++++++++++++
> >> include/efi_config.h | 5 +
> >> 4 files changed, 368 insertions(+)
> >> create mode 100644 cmd/eficonfig_sbkey.c
> >>
> >> diff --git a/cmd/Makefile b/cmd/Makefile
> >> index c95e09d058..f2f2857146 100644
> >> --- a/cmd/Makefile
> >> +++ b/cmd/Makefile
> >> @@ -66,6 +66,9 @@ 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
> >> +obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o
>
> Key enrollment does not make sense if the keys are not persisted across
> boots. The new feature should depend on CONFIG_EFI_MM_COMM_TEE.
Yes, persistent storage is required.
The problem is that not many platforms support CONFIG_EFI_MM_COMM_TEE and
this secure boot key menu feature can not be tested on sandbox.
>
> >> +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 0cb0770ac3..a72f07e671 100644
> >> --- a/cmd/eficonfig.c
> >> +++ b/cmd/eficonfig.c
> >> @@ -2442,6 +2442,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))
> >> + {"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..cc27f78e66
> >> --- /dev/null
> >> +++ b/cmd/eficonfig_sbkey.c
> >> @@ -0,0 +1,357 @@
> >> +// 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}, */
> >> +};
> >> +
> >> +/**
> >> + * is_secureboot_enabled() - check UEFI Secure Boot is enabled
> >> + *
> >> + * Return: true when UEFI Secure Boot is enabled, false otherwise
> >> + */
> >> +static bool is_secureboot_enabled(void)
> >> +{
> >> + efi_status_t ret;
> >> + u8 secure_boot;
> >> + efi_uintn_t size;
> >> +
> >> + size = sizeof(secure_boot);
> >> + ret = efi_get_variable_int(u"SecureBoot", &efi_global_variable_guid,
> >> + NULL, &size, &secure_boot, NULL);
> >> +
> >> + return secure_boot == 1;
> >> +}
> >> +
> >> +/**
> >> + * create_time_based_payload() - create payload for time based
> >> authenticate variable
> >> + *
> >> + * @db: pointer to the original signature database
> >> + * @new_db: pointer to the authenticated variable payload
> >> + * @size: pointer to payload size
> >> + * Return: status code
> >> + */
> >> +static efi_status_t create_time_based_payload(void *db, void
> >> **new_db, efi_uintn_t *size)
> >> +{
> >> + efi_status_t ret;
> >> + struct efi_time time;
> >> + efi_uintn_t total_size;
> >> + struct efi_variable_authentication_2 *auth;
> >> +
> >> + *new_db = NULL;
> >> +
> >> + /*
> >> + * SetVariable() call with
> >> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
> >> + * attribute requires EFI_VARIABLE_AUTHENTICATED_2 descriptor,
> >> prepare it
> >> + * without certificate data in it.
> >> + */
> >> + total_size = sizeof(struct efi_variable_authentication_2) + *size;
> >> +
> >> + auth = calloc(1, total_size);
> >> + if (!auth)
> >> + return EFI_OUT_OF_RESOURCES;
> >> +
> >> + ret = EFI_CALL((*efi_runtime_services.get_time)(&time, NULL));
> >> + if (ret != EFI_SUCCESS) {
> >> + free(auth);
> >> + return EFI_OUT_OF_RESOURCES;
> >> + }
> >> + time.pad1 = 0;
> >> + time.nanosecond = 0;
> >> + time.timezone = 0;
> >> + time.daylight = 0;
> >> + time.pad2 = 0;
> >> + memcpy(&auth->time_stamp, &time, sizeof(time));
> >> + auth->auth_info.hdr.dwLength = sizeof(struct
> >> win_certificate_uefi_guid);
> >> + auth->auth_info.hdr.wRevision = 0x0200;
> >> + auth->auth_info.hdr.wCertificateType = WIN_CERT_TYPE_EFI_GUID;
> >> + guidcpy(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7);
> >> + if (db)
> >> + memcpy((u8 *)auth + sizeof(struct
> >> efi_variable_authentication_2), db, *size);
> >> +
> >> + *new_db = auth;
> >> + *size = total_size;
> >> +
> >> + return EFI_SUCCESS;
> >> +}
> >> +
> >> +/**
> >> + * 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;
> >> +}
> >> +
> >> +/**
> >> + * file_is_efi_signature_list() - check the file is efi signature list
> >> + * @buf: pointer to file
> >> + * Return: true if file is efi signature list, false otherwise
> >> + */
> >> +static bool file_is_efi_signature_list(void *buf)
> >> +{
> >> + u32 i;
> >> + struct efi_signature_list *sig_list = buf;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(sigtype_to_str); i++) {
> >> + if (!guidcmp(&sig_list->signature_type,
> >> &sigtype_to_str[i].sig_type))
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +
> >> +/**
> >> + * 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;
> >> + void *new_db = NULL;
> >> + 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; is missing here.
Yes, this will fix the compilation error in clang.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >> + goto out;
> >> +
> >> + ret = eficonfig_select_file_handler(&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;
> >> +
> >> + size = 0;
> >> + ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, NULL));
> >> + if (ret != EFI_BUFFER_TOO_SMALL)
> >> + goto out;
> >> +
> >> + buf = calloc(1, size);
> >> + if (!buf) {
> >> + ret = EFI_OUT_OF_RESOURCES;
> >> + goto out;
> >> + }
> >> + ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, buf));
> >> + if (ret != EFI_SUCCESS)
> >> + goto out;
> >> +
> >> + size = ((struct efi_file_info *)buf)->file_size;
> >> + free(buf);
> >> +
> >> + buf = calloc(1, size);
> >> + if (!buf) {
> >> + ret = EFI_OUT_OF_RESOURCES;
> >
> > This leaves ret undefined and you get an error when compiling with clang:
> >
> > +cmd/eficonfig_sbkey.c:208:6: error: variable 'ret' is used
> > uninitialized whenever 'if' condition is true
> > [-Werror,-Wsometimes-uninitialized]
> > + if (!file_info.current_path)
> > + ^~~~~~~~~~~~~~~~~~~~~~~
> > +cmd/eficonfig_sbkey.c:294:9: note: uninitialized use occurs here
> > + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> > + ^~~
> > +cmd/eficonfig_sbkey.c:208:2: note: remove the 'if' if its condition is
> > always false
> > + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +cmd/eficonfig_sbkey.c:201:18: note: initialize the variable 'ret' to
> > silence this warning
> > + efi_status_t ret;
> > + ^
> > + = 0
> >
> >> + 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 (size == 0) {
> >> + eficonfig_print_msg("ERROR! File is empty.");
> >> + goto out;
> >> + }
> >> +
> >> + /* We expect that file is EFI Signature Lists or signed EFI
> >> Signature Lists */
> >> + if (!file_have_auth_header(buf, size)) {
> >> + if (!file_is_efi_signature_list(buf)) {
> >> + eficonfig_print_msg("ERROR! Invalid file format.");
> >> + ret = EFI_INVALID_PARAMETER;
> >> + goto out;
> >> + }
> >> +
> >> + ret = create_time_based_payload(buf, &new_db, &size);
> >> + if (ret != EFI_SUCCESS) {
> >> + eficonfig_print_msg("ERROR! Failed to create payload with
> >> timestamp.");
> >> + goto out;
> >> + }
> >> +
> >> + free(buf);
> >> + buf = new_db;
> >> + }
> >> +
> >> + 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");
> >> + goto out;
> >
> > This goto is superfluous.
> >
> > Best regards
> >
> > Heinrich
> >
> >> + }
> >> +
> >> +out:
> >> + free(file_info.current_path);
> >> + free(buf);
> >> +
> >> + /* to stay 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;
> >> + }
> >> +
> >> + /* to stay 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) **",
> >> + (is_secureboot_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;
> >> + }
> >> +
> >> + /* to stay 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 86bc801211..6db8e123f0 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
> >
>
