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

On 8/27/21 6:49 AM, 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?

The preseed store is as safe as the capsule updates that Linaro is working on where the certificate for verifying the capsule is baked into U-Boot or the StMM based variables.

They all require that an attacker can neither load a manipulated U-Boot nor that he can alter the memory containing U-Boot at runtime.

Best regards

Heinrich

-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