[PATCH v2 3/6] efi_loader: don't load signature database from file
Heinrich Schuchardt
xypron.glpk at gmx.de
Fri Aug 27 07:22:39 CEST 2021
On 8/27/21 6:49 AM, AKASHI Takahiro wrote:
> On Fri, Aug 27, 2021 at 06:42:39AM +0200, Heinrich Schuchardt wrote:
>> On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
>>> On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
>>>> The UEFI specification requires that the signature database may only be
>>>> stored in tamper-resistant storage. So these variable may not be read
>>>> from an unsigned file.
>>>
>>> I don't have a strong opinion here, but it seems to be too restrictive.
>>> Nobody expects that file-based variable implementation is *safe*.
>>> Leave it as it is so that people can easily experiment secure boot.
>>
>> If the prior boot stage checks the integrity of the U-Boot binary, the
>> file based implementation becomes 'safe' with this patch.
>
> How safe (or secure) is it? That is a question.
> What is your thread model?
The preseed store is as safe as the capsule updates that Linaro is
working on where the certificate for verifying the capsule is baked into
U-Boot or the StMM based variables.
They all require that an attacker can neither load a manipulated U-Boot
nor that he can alter the memory containing U-Boot at runtime.
Best regards
Heinrich
>
> -Takahiro Akashi
>
>
>> Users can still experiment with secure boot by setting the secure boot
>> variables via the efidebug command.
>>
>> I cannot see a use case for having the secure boot data base on an
>> insecure medium.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>> ---
>>>> v2:
>>>> no change
>>>> ---
>>>> include/efi_variable.h | 5 +++-
>>>> lib/efi_loader/efi_var_common.c | 2 --
>>>> lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++-------------
>>>> lib/efi_loader/efi_variable.c | 2 +-
>>>> 4 files changed, 30 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/include/efi_variable.h b/include/efi_variable.h
>>>> index 4623a64142..2d97655e1f 100644
>>>> --- a/include/efi_variable.h
>>>> +++ b/include/efi_variable.h
>>>> @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>>>> /**
>>>> * efi_var_restore() - restore EFI variables from buffer
>>>> *
>>>> + * Only if @safe is set secure boot related variables will be restored.
>>>> + *
>>>> * @buf: buffer
>>>> + * @safe: restoring from tamper-resistant storage
>>>> * Return: status code
>>>> */
>>>> -efi_status_t efi_var_restore(struct efi_var_file *buf);
>>>> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>>>>
>>>> /**
>>>> * efi_var_from_file() - read variables from file
>>>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
>>>> index cf7afecd60..b0c5b672c5 100644
>>>> --- a/lib/efi_loader/efi_var_common.c
>>>> +++ b/lib/efi_loader/efi_var_common.c
>>>> @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = {
>>>> {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},
>>>> {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB},
>>>> {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
>>>> - /* not used yet
>>>> {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
>>>> {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
>>>> - */
>>>> };
>>>>
>>>> static bool efi_secure_boot;
>>>> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
>>>> index de076b8cbc..c7c6805ed0 100644
>>>> --- a/lib/efi_loader/efi_var_file.c
>>>> +++ b/lib/efi_loader/efi_var_file.c
>>>> @@ -148,9 +148,10 @@ error:
>>>> #endif
>>>> }
>>>>
>>>> -efi_status_t efi_var_restore(struct efi_var_file *buf)
>>>> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>>>> {
>>>> struct efi_var_entry *var, *last_var;
>>>> + u16 *data;
>>>> efi_status_t ret;
>>>>
>>>> if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
>>>> @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf)
>>>> return EFI_INVALID_PARAMETER;
>>>> }
>>>>
>>>> - var = buf->var;
>>>> last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
>>>> - while (var < last_var) {
>>>> - u16 *data = var->name + u16_strlen(var->name) + 1;
>>>> -
>>>> - if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
>>>> - ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
>>>> - var->length, data, 0, NULL,
>>>> - var->time);
>>>> - if (ret != EFI_SUCCESS)
>>>> - log_err("Failed to set EFI variable %ls\n",
>>>> - var->name);
>>>> - }
>>>> - var = (struct efi_var_entry *)
>>>> - ALIGN((uintptr_t)data + var->length, 8);
>>>> + for (var = buf->var; var < last_var;
>>>> + var = (struct efi_var_entry *)
>>>> + ALIGN((uintptr_t)data + var->length, 8)) {
>>>> +
>>>> + data = var->name + u16_strlen(var->name) + 1;
>>>> +
>>>> + /*
>>>> + * Secure boot related and non-volatile variables shall only be
>>>> + * restored from U-Boot's preseed.
>>>> + */
>>>> + if (!safe &&
>>>> + (efi_auth_var_get_type(var->name, &var->guid) !=
>>>> + EFI_AUTH_VAR_NONE ||
>>>> + !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
>>>> + continue;
>>>> + if (!var->length)
>>>> + continue;
>>>> + ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
>>>> + var->length, data, 0, NULL,
>>>> + var->time);
>>>> + if (ret != EFI_SUCCESS)
>>>> + log_err("Failed to set EFI variable %ls\n", var->name);
>>>> }
>>>> return EFI_SUCCESS;
>>>> }
>>>> @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void)
>>>> log_err("Failed to load EFI variables\n");
>>>> goto error;
>>>> }
>>>> - if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
>>>> + if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS)
>>>> log_err("Invalid EFI variables file\n");
>>>> error:
>>>> free(buf);
>>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>>>> index ba0874e9e7..a7d305ffbc 100644
>>>> --- a/lib/efi_loader/efi_variable.c
>>>> +++ b/lib/efi_loader/efi_variable.c
>>>> @@ -426,7 +426,7 @@ efi_status_t efi_init_variables(void)
>>>>
>>>> if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
>>>> ret = efi_var_restore((struct efi_var_file *)
>>>> - __efi_var_file_begin);
>>>> + __efi_var_file_begin, true);
>>>> if (ret != EFI_SUCCESS)
>>>> log_err("Invalid EFI variable seed\n");
>>>> }
>>>> --
>>>> 2.30.2
>>>>
>>
More information about the U-Boot
mailing list