On Thu, 7 Oct 2021 at 08:54, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 10/6/21 16:05, Ilias Apalodimas wrote:
On Wed, 6 Oct 2021 at 16:15, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
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.
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().
Yea fair enough, I was trying to think if it would make sense to re-use it for other variables.
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
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org