Re: [PATCH v2 3/6] efi_loader: don't load signature database from file

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Fri, Aug 27, 2021 at 01:49:41PM +0900, 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?

Obviously, thread -> threat

> 
> -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@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
> > > > 
> >