[RFC PATCH 1/3] eficonfig: add UEFI Secure Boot Key enrollment interface
Masahisa Kojima
masahisa.kojima at linaro.org
Mon Jul 11 15:24:37 CEST 2022
Hi Heinrich,
On Sun, 10 Jul 2022 at 18:37, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 7/8/22 11:14, Ilias Apalodimas wrote:
> > 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 at 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>
> >> +
>
> Please, provide function descriptions.
OK.
>
> >> +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.
>
> Assigning NULL would have no effect. The variable is reassigned in the
> next line.
OK.
>
> >
> >> +
> >> + 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?
>
> The UEFI specification says:
>
> "The PK variable contains *the* current Platform Key."
>
> So there should always be only one key in the variable.
Yes, my code always overwrites 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");
>
> *%s/Fail/Failed/
>
> Please, add unit tests for your patches.
What kind of unit tests do you expect to be added?
This patch series mainly adds the UI, do you want to add the unit tests
to manipulate the UI and update the secure boot keys?
Or do you want to enhance the efi_set_variable_int() test cases?
> My expectation is that efi_set_variable_int() will only succeed if the
> variable change request is signed with the old PK or if PK does not exist.
For PK, the current implementation meets this expectation.
Key configuration UI code I'm working on behaves like the wrapper of
efi_set_variable_int(). efi_set_variable_int() checks if the variable
change
request is signed or not.
>
> Under which circumstances shall a board owner be able to remove PK if he
> does not possess the private key?
This is important, but I feel this is out of scope of my patch series.
In current U-Boot implementation, there is no way to remove PK
if the board owner does not possess the private key or signed NULL key.
EDK2 implements the "Custom Mode" to update the PK, KEK, db and dbx
with the non-signed signature list.
To enter the Custom Mode, it requires that board owner is physically present
at the board and it requires the platform specific implementation.
I can't come up with other ideas for now.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >> + goto out;
> >> + }
> >> +
> >> +out:
> >> + free(file_info.current_path);
> >> + free(buf);
> >> +
> >>
> > [...]
> >
> > Thanks
> > /Ilias
>
More information about the U-Boot
mailing list