Re: [PATCH v3 3/3] efi: Add API for retrieving the EFI secret for cryptodisk

[Dov Murik]

Hello James,

On 31/12/2020 19:36, James Bottomley wrote:
> This module is designed to provide an efisecret command which
> interrogates the EFI configuration table to find the location of the
> confidential computing secret and tries to register the secret with
> the cryptodisk.
>
> The secret is stored in a boot allocated area, usually a page in size.
> The layout of the secret injection area is a header
>
> |GRUB_EFI_SECRET_TABLE_HEADER_GUID|len|
>
> with entries of the form
>
> |guid|len|data|
>
> the guid corresponding to the disk encryption passphrase is
> GRUB_EFI_DISKPASSWD_GUID and data must be a zero terminated string.
> To get a high entropy string that doesn't need large numbers of
> iterations, use a base64 encoding of 33 bytes of random data.
>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>
> ---
>
> v2: use callback to print failure message and destroy secret
> v3: change to generic naming to use for TDX and SEV and use new mechanism
> ---
>   grub-core/Makefile.core.def    |   8 ++
>   grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
>   include/grub/efi/api.h         |  15 ++++
>   3 files changed, 152 insertions(+)
>   create mode 100644 grub-core/disk/efi/efisecret.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 68b9e9f68..0b7e365cd 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -785,6 +785,14 @@ module = {
>     enable = efi;
>   };
>
> +module = {
> +  name = efisecret;
> +
> +  common = disk/efi/efisecret.c;
> +
> +  enable = efi;
> +};
> +
>   module = {
>     name = lsefimmap;
>
> diff --git a/grub-core/disk/efi/efisecret.c b/grub-core/disk/efi/efisecret.c
> new file mode 100644
> index 000000000..318af0784
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,129 @@
> +#include <grub/err.h>
> +#include <grub/misc.h>
> +#include <grub/cryptodisk.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/api.h>
> +#include <grub/dl.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
> +static grub_efi_packed_guid_t tableheader_guid = 
> GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> +static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
> +
> +/*
> + * EFI places the secret in the lower 4GB, so it uses a UINT32
> + * for the pointer which we have to transform to the correct type
> + */

This comment is no longer accurate now that efi_secret has 64-bit 
address and length fields. Just remove it?

> +struct efi_secret {
> +  grub_uint64_t base;
> +  grub_uint64_t size;
> +};
> +
> +struct secret_header {
> +  grub_efi_packed_guid_t guid;
> +  grub_uint32_t len;
> +};
> +
> +struct secret_entry {
> +  grub_efi_packed_guid_t guid;
> +  grub_uint32_t len;
> +  char data[0];
> +};
> +
> +static grub_err_t
> +grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
> +                    char **ptr)
> +{
> +  struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct 
> secret_entry *)0)->data);
> +
> +  /* destroy the secret */
> +  grub_memset (e, 0, e->len);

In grub-core/lib/libgcrypt/src/g10lib.h there's a wipememory macro which 
should circumvent optimizers which might remove the grub_memset call:


/* To avoid that a compiler optimizes certain memset calls away, these
   macros may be used instead. */
#define wipememory2(_ptr,_set,_len) do { \
              volatile char *_vptr=(volatile char *)(_ptr); \
              size_t _vlen=(_len); \
              while(_vlen) { *_vptr=(_set); _vptr++; _vlen--; } \
                  } while(0)
#define wipememory(_ptr,_len) wipememory2(_ptr,0,_len)



Maybe the same thing should be used here or copied here? This is an 
attempt to wipe the disk decryption password (= part the EFI secret 
area) against future leakage.

(Note that I have no evidence that any optimizer currently removes the 
grub_memset call.)


> +  *ptr = NULL;
> +
> +  if (have_it)
> +    return GRUB_ERR_NONE;
> +
> +  return  grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock any 
> volumes");
> +}
> +
> +static grub_err_t
> +grub_efi_secret_find (struct efi_secret *s, char **secret_ptr)
> +{
> +  int len;
> +  struct secret_header *h;
> +  struct secret_entry *e;
> +  unsigned char *ptr = (unsigned char *)(unsigned long)s->base;

The cast to unsigned long can be removed (s->base is a grub_uint64_t).


> +
> +  /* the area must be big enough for a guid and a u32 length */
> +  if (s->size < sizeof (*h))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small");
> +
> +  h = (struct secret_header *)ptr;
> +  if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not start with 
> correct guid\n");
> +  if (h->len < sizeof (*h))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too 
> small\n");
> +
> +  len = h->len - sizeof (*h);
> +  ptr += sizeof (*h);
> +
> +  while (len >= (int)sizeof (*e)) {
> +    e = (struct secret_entry *)ptr;
> +    if (e->len < sizeof(*e) || e->len > (unsigned int)len)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is 
> corrupt\n");
> +
> +    if (! grub_memcmp (&e->guid, &diskpasswd_guid, sizeof (e->guid))) {
> +      int end = e->len - sizeof(*e);
> +
> +      /*
> +       * the passphrase must be a zero terminated string because the
> +       * password routines call grub_strlen () to find its size
> +       */
> +      if (e->data[end - 1] != '\0')
> +       return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption 
> password is not zero terminated\n");

I think there's a pathological edge case if e->len exactly equals 
sizeof(struct secret_entry) -- that is, it indicates a GUID struct with 
a zero-length data field.  In such a case, 'end' will be 0, the 
zero-termination check below might succeed because e->data[-1] == '\0' 
(last byte of length field in little-endian machine will probably be 
zero).  As a result secret_ptr will point to garbage.

A possible solution is to modify the above condition to:

  if (end <= 0 || e->data[end - 1] != '\0')
    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk 
encryption password is not zero terminated\n");

I assume that a 0-length data field might be OK for other GUIDs but it 
is not valid for the disk password GUID.


> +
> +      *secret_ptr = e->data;
> +      return GRUB_ERR_NONE;
> +    }
> +    ptr += e->len;
> +    len -= e->len;
> +  }
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret aread does not contain disk 
> decryption password\n");

typo: s/aread/area/


> +}
> +
> +static grub_err_t
> +grub_efi_secret_get (const char *arg __attribute__((unused)), char **ptr)
> +{
> +  unsigned int i;
> +
> +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> +    {
> +      grub_efi_packed_guid_t *guid =
> +       &grub_efi_system_table->configuration_table[i].vendor_guid;
> +
> +      if (! grub_memcmp (guid, &secret_guid, sizeof (grub_efi_packed_guid_t))) 
> {
> +       struct efi_secret *s =
> +         grub_efi_system_table->configuration_table[i].vendor_table;
> +
> +       return grub_efi_secret_find(s, ptr);
> +      }
> +    }
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "No secret found in the EFI 
> configuration table");
> +}
> +
> +static struct grub_secret_entry secret = {
> +  .name = "efisecret",
> +  .get = grub_efi_secret_get,
> +  .put = grub_efi_secret_put,
> +};
> +
> +GRUB_MOD_INIT(efisecret)
> +{
> +  grub_cryptodisk_add_secret_provider (&secret);
> +}
> +
> +GRUB_MOD_FINI(efisecret)
> +{
> +  grub_cryptodisk_remove_secret_provider (&secret);
> +}
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index 39733585b..53a47f658 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -299,6 +299,21 @@
>       { 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
>     }
>
> +#define GRUB_EFI_SECRET_TABLE_GUID \
> +  { 0xadf956ad, 0xe98c, 0x484c, \
> +    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47} \
> +  }
> +
> +#define GRUB_EFI_SECRET_TABLE_HEADER_GUID \
> +  { 0x1e74f542, 0x71dd, 0x4d66, \
> +    { 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b } \
> +  }
> +
> +#define GRUB_EFI_DISKPASSWD_GUID \
> +  { 0x736869e5, 0x84f0, 0x4973, \
> +    { 0x92, 0xec, 0x06, 0x87, 0x9c, 0xe3, 0xda, 0x0b } \
> +  }
> +
>   #define GRUB_EFI_ACPI_TABLE_GUID      \
>     { 0xeb9d2d30, 0x2d88, 0x11d3, \
>       { 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \


-Dov