[PATCH v3 1/3] efi_loader: don't load signature database from file

[Heinrich Schuchardt]

On 9/6/21 2:12 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Thu, Sep 02, 2021 at 11:35:29AM +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.
>
> Even with TF-A (or other methods) assumed, I think the behavior here is
> too restrictive.
>
> If you think TF-A provides the base for "chain of trust", all what you
> need to do is to protect PK and all the other auth variables should be
> allowed to be restored even from an unsafe file because the changes of
> values will be verified anyway by UEFI system as long as SetVariable()
> is used.
>
> Please think about why UEFI specification defines both PK and KEK.
> The ability of setting/modifying KEK will add more flexibility of system
> configuration.

If relying on only PK being fixed, one would still have to get the
sequence of import from file right: PK -> KEK -> db,dbx,dbt,dbr.

This would require a change in the file loader (efi_var_restore()).
Could you contribute such a patch?

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>>   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 3d92afe2eb..005c03ea5f 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.32.0
>>