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

[Heinrich Schuchardt]

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.

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

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

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

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.

Under which circumstances shall a board owner be able to remove PK if he
does not possess the private key?

Best regards

Heinrich

>> +		goto out;
>> +	}
>> +
>> +out:
>> +	free(file_info.current_path);
>> +	free(buf);
>> +
>>
> [...]
>
> Thanks
> /Ilias