Re: [RFC PATCH 1/3] eficonfig: add UEFI Secure Boot Key enrollment interface

On Sun, Jun 19, 2022 at 02:20:20PM +0900, 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.kojima@linaro.org

cmd/Makefile | 3 + cmd/eficonfig.c | 3 + cmd/eficonfig_sbkey.c | 202 ++++++++++++++++++++++++++++++++++++++++++ include/efi_config.h | 3 + 4 files changed, 211 insertions(+) create mode 100644 cmd/eficonfig_sbkey.c

diff --git a/cmd/Makefile b/cmd/Makefile index 0afa687e94..9d87b639fc 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -64,6 +64,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 +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 e62f5e41a4..e6d2cba9c5 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1832,6 +1832,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..a5c0dbe9b3 --- /dev/null +++ b/cmd/eficonfig_sbkey.c @@ -0,0 +1,202 @@ +// 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>

+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;

+}

+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)

• 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);

You should set buf to NULL here.

• buf = calloc(1, size);
• if (!buf)

• goto out;
  

• ret = efi_file_read_int(f, &size, buf);
• if (ret != EFI_SUCCESS || size == 0)

• 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;
  

• }

Why are we appending? Shouldn't we always overwrite the platform key?

• 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! Fail to update signature database");
  

• goto out;
  

• }

+out:

• free(file_info.current_path);
• free(buf);

[...]

Thanks /Ilias