Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support

[Glenn Washburn]

On Mon,  7 Feb 2022 10:29:43 -0500
James Bottomley <jejb@linux.ibm.com> wrote:

> Make use of the new OS provided secrets API so that if the new '-s'
> option is passed in we try to extract the secret from the API rather
> than prompting for it.
> 
> The primary consumer of this is AMD SEV, which has been programmed to
> provide an injectable secret to the encrypted virtual machine.  OVMF
> provides the secret area and passes it into the EFI Configuration
> Tables.  The grub EFI layer pulls the secret out and primes the
> secrets API with it.  The upshot of all of this is that a SEV
> protected VM can do an encrypted boot with a protected boot secret.

I think I prefer the key protector framework proposed in the first
patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It feels
like a more generic mechanism (though admittedly very similar to yours)
because its not tied to cryptodisk. I'm not sure where we'd want to use
the secrets/protectors framework outside of cryptodisk, but it seems
like it could be useful. The advantage of this patch is that it allows
for the clearing of key data from memory.

I don't think we should have both a secrets and a key protectors
framework, as its seems they are aiming to accomplish basically the
same thing.

The name "secrets" seems a bit more generic than "protectors" because,
as in this case, the secret is not protected. There is something I
don't like about the word "secrets", but I don't have a better
suggestion, so this might be what sticks. One thing going for "secrets"
over "protectors", is that the cryptomount option "-s" works better
than "-p" for protectors because that's already taken by the password
option.

> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> 
> ---
> 
> v2: add callback for after secret use
> v3: update to use named module mechanism for secret loading
> v4: update for new secret passing API
> ---
>  grub-core/disk/cryptodisk.c | 56 +++++++++++++++++++++++++++++++++++--
>  include/grub/cryptodisk.h   | 14 ++++++++++
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 497097394..f9c86f958 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,7 @@ static const struct grub_arg_option options[] =
>      {"all", 'a', 0, N_("Mount all."), 0, 0},
>      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
>      {"password", 'p', 0, N_("Password to open volumes."), 0, 
> ARG_TYPE_STRING},
> +    {"secret", 's', 0, N_("Get secret passphrase from named module and 
> optional identifier"), 0, 0},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -984,6 +985,9 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>  
>  #endif
>  
> +/* variable to hold the list of secret providers */
> +static struct grub_secret_entry *secret_providers;
> +
>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
>  {
> @@ -1064,6 +1068,18 @@ grub_cryptodisk_scan_device_real (const char *name,
>    return dev;
>  }
>  
> +void
> +grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e)
> +{
> +  grub_list_push(GRUB_AS_LIST_P (&secret_providers), GRUB_AS_LIST (e));
> +}
> +
> +void
> +grub_cryptodisk_remove_secret_provider  (struct grub_secret_entry *e)
> +{
> +  grub_list_remove (GRUB_AS_LIST (e));
> +}
> +
>  #ifdef GRUB_UTIL
>  #include <grub/util/misc.h>
>  grub_err_t
> @@ -1160,10 +1176,14 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>  {
>    struct grub_arg_list *state = ctxt->state;
>    struct grub_cryptomount_args cargs = {0};
> +  struct grub_secret_entry *se = NULL;
>  
> -  if (argc < 1 && !state[1].set && !state[2].set)
> +  if (argc < 1 && !state[1].set && !state[2].set && !state[3].set)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>  
> +  if (state[3].set && state[4].set)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "-p and -s are exclusive 
> options");
> +
>    if (grub_cryptodisk_list == NULL)
>      return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
>  
> @@ -1172,6 +1192,24 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>        cargs.key_data = (grub_uint8_t *) state[3].arg;
>        cargs.key_len = grub_strlen (state[3].arg);
>      }
> +  else if (state[4].set)     /* secret module */
> +    {
> +      grub_err_t rc;
> +
> +      if (argc < 1)
> +     return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must be 
> specified");
> +#ifndef GRUB_UTIL
> +      grub_dl_load (args[0]);
> +#endif
> +      se = grub_named_list_find (GRUB_AS_NAMED_LIST (secret_providers), 
> args[0]);
> +      if (se == NULL)
> +     return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret provider is 
> found");
> +
> +      rc = se->get (args[1], &cargs.key_data);
> +      if (rc)
> +     return rc;
> +      cargs.key_len = grub_strlen((char *) cargs.key_data);

It seems better to me to send a pointer to cargs.key_len to se->get()
because it already knows the length without having to do a strlen. And
this will allow NULLs in the key data.

> +    }
>  
>    if (state[0].set) /* uuid */
>      {
> @@ -1190,6 +1228,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>        cargs.search_uuid = args[0];
>        found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, 
> &cargs);
>  
> +      if (state[4].set)
> +        {
> +          se->put (args[1], found_uuid, &cargs.key_data);
> +        }
> +
>        if (found_uuid)
>       return GRUB_ERR_NONE;
>        else if (grub_errno == GRUB_ERR_NONE)
> @@ -1208,6 +1251,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>      {
>        cargs.check_boot = state[2].set;
>        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> +      if (state[4].set)
> +        {
> +          se->put (args[1], 1, &cargs.key_data);
> +        }
>        return GRUB_ERR_NONE;
>      }
>    else
> @@ -1252,6 +1299,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>        if (disklast)
>       *disklast = ')';
>  
> +      if (state[4].set)
> +        {
> +          se->put (args[1], dev != NULL, &cargs.key_data);
> +        }
> +
>        return (dev == NULL) ? grub_errno : GRUB_ERR_NONE;
>      }
>  }
> @@ -1385,7 +1437,7 @@ GRUB_MOD_INIT (cryptodisk)
>  {
>    grub_disk_dev_register (&grub_cryptodisk_dev);
>    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> -                           N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
> +                           N_("[-p password] <SOURCE|-u UUID|-a|-b|-s MOD 
> [ID]>"),

Seems like this should be:
 "[-p password|-s MOD [ID]] <SOURCE|-u UUID|-a|-b"

>                             N_("Mount a crypto device."), options);
>    grub_procfs_register ("luks_script", &luks_script);
>  }
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index c6524c9ea..60249f1fc 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -183,4 +183,18 @@ grub_util_get_geli_uuid (const char *dev);
>  grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
>  grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t disk);
>  
> +struct grub_secret_entry {

s/grub_secret_entry/grub_secret_provider/

> +  /* as named list */
> +  struct grub_secret_entry *next;
> +  struct grub_secret_entry **prev;
> +  const char *name;
> +
> +  /* additional entries */
> +  grub_err_t (*get) (const char *arg, grub_uint8_t **secret);

I like having this first arg to pass some data to the secret provider.
I'm guessing this is to accomodate a keyfiles secrets provider. It seems
like it could also be used to differentiate between different elements
using the same secrets provider. Hypotheticaly say one had a USB device
with a bunch of key slots, a numerical value could be passed to choose
which slot to use. In light of that, perhaps the arg should be a void *,
to be more generic.

> +  grub_err_t (*put) (const char *arg, int have_it, grub_uint8_t **secret);

I don't really like the names get and put. Something more descriptive
would be nice. I like "recover_key" or perhaps "recover_secret" for
"get", and "dispose" or "release" for "put". I'm open to other names
also.

> +};
> +
> +void grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e);
> +void grub_cryptodisk_remove_secret_provider (struct grub_secret_entry *e);
> +
>  #endif

Glenn