[PATCH v9 4/5] eficonfig: add UEFI Secure Boot Key enrollment interface

Heinrich Schuchardt xypron.glpk at gmx.de
Fri Nov 18 18:30:06 CET 2022


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

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