Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 10/6/21 10:02, Ilias Apalodimas wrote:
> On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 10/6/21 08:29, Ilias Apalodimas wrote:
>>> On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
>>>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v3:
>>>>       Keep error handling in efi_sigstore_parse_sigdb()
>>>> v2:
>>>>       remove a superfluous check
>>>> ---
>>>>    lib/efi_loader/efi_signature.c | 11 ++---------
>>>>    1 file changed, 2 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
>>>> index bdd09881fc..97f6dfacd9 100644
>>>> --- a/lib/efi_loader/efi_signature.c
>>>> +++ b/lib/efi_loader/efi_signature.c
>>>> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>>>>       efi_uintn_t db_size;
>>>>       efi_status_t ret;
>>>>
>>>> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
>>>> -            vendor = &efi_global_variable_guid;
>>>> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
>>>> -            vendor = &efi_guid_image_security_database;
>>>> -    } else {
>>>> -            EFI_PRINT("unknown signature database, %ls\n", name);
>>>> -            return NULL;
>>>> -    }
>>>> +    vendor = efi_auth_var_get_guid(name);
>>>
>>> Should we return NULL if we get back the default guid?
>>
>> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
>> So how should this occur?
> 
> Bugs that slip through maybe?  I generally prefer being more pedantic
> with security related code

PK and KEK use efi_global_variable_guid and will be used as argument for 
efi_sigstore_parse_sigdb(). Your proposed check would break the code.

Best regards

Heinrich

> 
> Regards
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>
>>>>       /* retrieve variable data */
>>>>       db_size = 0;
>>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
>>>> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
>>>>       if (ret == EFI_NOT_FOUND) {
>>>>               EFI_PRINT("variable, %ls, not found\n", name);
>>>>               sigstore = calloc(sizeof(*sigstore), 1);
>>>> --
>>>> 2.32.0
>>>>
>>>
>>> Regards
>>> /Ilias
>>>