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

[Heinrich Schuchardt]

On 10/6/21 16:05, Ilias Apalodimas wrote:
> On Wed, 6 Oct 2021 at 16:15, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> On 10/6/21 10:02, Ilias Apalodimas wrote:
>>> On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
>>> <heinrich.schuchardt at 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 at 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.
> 
> Yea that's the reason I was asking about efi_auth_var_get_guid().  I
> would prefer if that returned NULL if the variable is not found
> instead of the default GUID

As discussed the functions is only called for hardcoded arguments.

Function efi_auth_var_get_guid() parses a list contains dbx, AuditMode, 
DeployedMode. So the check that you propose cannot safeguard against a 
developer misusing efi_sigstore_parse_sigdb().

Best regards

Heinrich

> 
> Regards
> /Ilias
>>
>> 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
>>>>>
>>