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

[Heinrich Schuchardt]

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