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

[James Bottomley]

On Sun, 2021-01-03 at 12:46 +0200, Dov Murik wrote:
> Hello James,
> 
> On 31/12/2020 19:36, James Bottomley wrote:
[...]
> > 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?

Yes, an oversight, I'll update the comment.

> > +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.)

Actually, this gets into the whole systems annoyance about who should
see the secret and how long should it live.  The truth is this secret
has to be used twice: once for grub to find the kernel and initrd and
once for the initrd to mount root.  The fact that there's no current
mechanism in place for grub to pass the secret to the initrd is a bit
of a systems failure.  I was thinking the config table could do that,
in which case this code should really be eliminated.  Likely the best
thing to do would be to have an OVMF exitbootservices notifier which
wipes everything.

> > +  *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 style of casting is best practice for converting integers to
pointers ... it's actually what the C manual recommends so it's best to
keep it for the compiler to be aware this is exactly what is intended. 
Just as an illustration think what happens on a 32 bit machine:
grub_uint64_t is too big and we need the unsigned long cast to truncate
it.

> > +
> > +  /* 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.

Sure, although I think end < 2 just in case a zero length string causes
other problems.

> > +
> > +      *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/

fixed.

James