Re: [PATCH v2 10/11] cryptodisk: Support key protectors

[Gary Lin]

On Fri, Mar 24, 2023 at 03:21:57AM +0000, Glenn Washburn wrote:
> On 3/22/23 08:10, Gary Lin wrote:
> > From: Hernan Gatta <hegatta@linux.microsoft.com>
> > 
> > Add a new parameter to cryptomount to support the key protectors framework: 
> > -P.
> > The parameter is used to automatically retrieve a key from specified key
> > protectors. The parameter may be repeated to specify any number of key
> > protectors. These are tried in order until one provides a usable key for any
> > given disk.
> > 
> > Signed-off-by: Hernan Gatta <hegatta@linux.microsoft.com>
> > Signed-off-by: Michael Chang <mchang@suse.com>
> 
> I'm wondering, why Michael's SOB is needed here? Did I miss somewhere on the
> list where he contributed to this patch?
> 
Michael did the most hard work of rebasing the patch, so I'd like to
give him the credit.

> > Signed-off-by: Gary Lin <glin@suse.com>
> > ---
> >   Makefile.util.def           |   1 +
> >   grub-core/disk/cryptodisk.c | 175 +++++++++++++++++++++++++++++-------
> >   include/grub/cryptodisk.h   |  14 +++
> >   3 files changed, 158 insertions(+), 32 deletions(-)
> > 
> > diff --git a/Makefile.util.def b/Makefile.util.def
> > index c3b7df96d..7d8db722e 100644
> > --- a/Makefile.util.def
> > +++ b/Makefile.util.def
> > @@ -35,6 +35,7 @@ library = {
> >     common = grub-core/kern/list.c;
> >     common = grub-core/kern/misc.c;
> >     common = grub-core/kern/partition.c;
> > +  common = grub-core/kern/protectors.c;
> >     common = grub-core/lib/crypto.c;
> >     common = grub-core/lib/json/json.c;
> >     common = grub-core/disk/luks.c;
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 34b67a705..c5974399e 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -26,6 +26,7 @@
> >   #include <grub/file.h>
> >   #include <grub/procfs.h>
> >   #include <grub/partition.h>
> > +#include <grub/protector.h>
> >   #ifdef GRUB_UTIL
> >   #include <grub/emu/hostdisk.h>
> > @@ -44,7 +45,8 @@ enum
> >       OPTION_KEYFILE,
> >       OPTION_KEYFILE_OFFSET,
> >       OPTION_KEYFILE_SIZE,
> > -    OPTION_HEADER
> > +    OPTION_HEADER,
> > +    OPTION_PROTECTOR
> >     };
> >   static const struct grub_arg_option options[] =
> > @@ -58,6 +60,8 @@ static const struct grub_arg_option options[] =
> >       {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, 
> > ARG_TYPE_INT},
> >       {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, 
> > ARG_TYPE_INT},
> >       {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> > +    {"protector", 'P', GRUB_ARG_OPTION_REPEATABLE,
> > +     N_("Unlock volume(s) using key protector(s)."), 0, ARG_TYPE_STRING},
> >       {0, 0, 0, 0, 0, 0}
> >     };
> > @@ -1060,7 +1064,8 @@ grub_cryptodisk_scan_device_real (const char *name,
> >   {
> >     grub_err_t ret = GRUB_ERR_NONE;
> >     grub_cryptodisk_t dev;
> > -  grub_cryptodisk_dev_t cr;
> > +  grub_cryptodisk_dev_t cr, crd = NULL;
> > +  int i;
> >     struct cryptodisk_read_hook_ctx read_hook_data = {0};
> >     int askpass = 0;
> >     char *part = NULL;
> > @@ -1113,41 +1118,113 @@ grub_cryptodisk_scan_device_real (const char *name,
> >         goto error_no_close;
> >       if (!dev)
> >         continue;
> > +    crd = cr;
> 
> Why can't we just use cr below and not introduce crd?
> 
Good question. 'crd' seems not necessary. Will check the code again.

> > +    break;
> > +  }
> > -    if (!cargs->key_len)
> > -      {
> > -   /* Get the passphrase from the user, if no key data. */
> > -   askpass = 1;
> > -   part = grub_partition_get_name (source->partition);
> > -   grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > -                source->partition != NULL ? "," : "",
> > -                part != NULL ? part : N_("UNKNOWN"),
> > -                dev->uuid);
> > -   grub_free (part);
> > -
> > -   cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> > -   if (cargs->key_data == NULL)
> > -     goto error_no_close;
> > -
> > -   if (!grub_password_get ((char *) cargs->key_data, 
> > GRUB_CRYPTODISK_MAX_PASSPHRASE))
> > -     {
> > -       grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> > -       goto error;
> > -     }
> > -   cargs->key_len = grub_strlen ((char *) cargs->key_data);
> > -      }
> > +  if (!dev)
> > +    {
> > +      grub_error (GRUB_ERR_BAD_MODULE,
> > +                  "no cryptodisk module can handle this device");
> 
> Every 8 spaces should be converted to a tab character. Same for all
> instances below too (and probably in other patches, though not in the ones
> importing foreign libs, like libtasn).
> 
Will fix in v3.

> > +      goto error_no_close;
> > +    }
> > -    ret = cr->recover_key (source, dev, cargs);
> > -    if (ret != GRUB_ERR_NONE)
> > -      goto error;
> > +  if (cargs->protectors)
> > +    {
> > +      for (i = 0; cargs->protectors[i]; i++)
> > +        {
> > +          if (cargs->key_cache[i].invalid)
> > +            continue;
> > +
> > +          if (!cargs->key_cache[i].key)
> > +            {
> > +              ret = grub_key_protector_recover_key (cargs->protectors[i],
> > +                                                    
> > &cargs->key_cache[i].key,
> > +                                                    
> > &cargs->key_cache[i].key_len);
> > +              if (ret)
> 
> Daniel will say this should be "ret != GRUB_ERR_NONE", and same for similar
> uses elsewhere.
> 
Ditto.

> > +                {
> > +                  if (grub_errno)
> > +                    {
> > +                      grub_print_error ();
> > +                      grub_errno = GRUB_ERR_NONE;
> > +                    }
> > +
> > +                  grub_dprintf ("cryptodisk",
> > +                                "failed to recover a key from key 
> > protector "
> > +                                "%s, will not try it again for any other "
> > +                                "disks, if any, during this invocation of "
> > +                                "cryptomount\n",
> > +                                cargs->protectors[i]);
> > +
> > +                  cargs->key_cache[i].invalid = 1;
> > +                  continue;
> > +                }
> > +            }
> > +
> > +          cargs->key_data = cargs->key_cache[i].key;
> > +          cargs->key_len = cargs->key_cache[i].key_len;
> > +
> > +          ret = crd->recover_key (source, dev, cargs);
> > +          if (ret) > +            {
> > +              part = grub_partition_get_name (source->partition);
> > +              grub_dprintf ("cryptodisk",
> > +                            "recovered a key from key protector %s but it "
> > +                            "failed to unlock %s%s%s (%s)\n",
> > +                             cargs->protectors[i], source->name,
> > +                             source->partition != NULL ? "," : "",
> > +                             part != NULL ? part : N_("UNKNOWN"), 
> > dev->uuid);
> > +               grub_free (part);
> > +               continue;
> > +            }
> > +         else
> > +           {
> > +             ret = grub_cryptodisk_insert (dev, name, source);
> > +             if (ret != GRUB_ERR_NONE)
> > +               goto error;
> > +             goto cleanup;
> > +           }
> > +        }
> > -    ret = grub_cryptodisk_insert (dev, name, source);
> > -    if (ret != GRUB_ERR_NONE)
> > +      part = grub_partition_get_name (source->partition);
> > +      grub_error (GRUB_ERR_ACCESS_DENIED,
> > +                  N_("no key protector provided a usable key for %s%s%s 
> > (%s)"),
> > +                  source->name, source->partition != NULL ? "," : "",
> > +                  part != NULL ? part : N_("UNKNOWN"), dev->uuid);
> > +      grub_free (part);
> >         goto error;
> > +    }
> > +
> > +  if (!cargs->key_len)
> > +    {
> > +      /* Get the passphrase from the user, if no key data. */
> > +      askpass = 1;
> > +      part = grub_partition_get_name (source->partition);
> > +      grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > +                    source->partition != NULL ? "," : "",
> > +                    part != NULL ? part : N_("UNKNOWN"), dev->uuid);
> > +      grub_free (part);
> > +
> > +      cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> > +      if (cargs->key_data == NULL)
> > +        goto error;
> > +
> > +      if (!grub_password_get ((char *) cargs->key_data, 
> > GRUB_CRYPTODISK_MAX_PASSPHRASE))
> > +        {
> > +          grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> > +          goto error;
> > +        }
> > +      cargs->key_len = grub_strlen ((char *) cargs->key_data);
> > +    }
> > +
> > +  ret = crd->recover_key (source, dev, cargs);
> > +  if (ret != GRUB_ERR_NONE)
> > +    goto error;
> > +
> > +  ret = grub_cryptodisk_insert (dev, name, source);
> > +  if (ret != GRUB_ERR_NONE)
> > +    goto error;
> > -    goto cleanup;
> > -  }
> > -  grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this 
> > device");
> >     goto cleanup;
> >    error:
> > @@ -1258,6 +1335,20 @@ grub_cryptodisk_scan_device (const char *name,
> >     return ret;
> >   }
> > +static void
> > +grub_cryptodisk_clear_key_cache (struct grub_cryptomount_args *cargs)
> > +{
> > +  int i;
> > +
> > +  if (!cargs->key_cache)
> 
> Daniel also prefers "cargs->key_cache == NULL" for NULL checks on pointers.
> 
Ditto.

> > +    return;
> > +
> > +  for (i = 0; cargs->protectors[i]; i++)
> 
> Maybe good to be defensive and check that cargs->protectors is not NULL.
> 
Agree. Will fix in v3.

> > +    grub_free (cargs->key_cache[i].key);
> > +
> > +  grub_free (cargs->key_cache);
> > +}
> > +
> >   static grub_err_t
> >   grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >   {
> > @@ -1270,6 +1361,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > int argc, char **args)
> >     if (grub_cryptodisk_list == NULL)
> >       return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules 
> > loaded");
> > +  if (state[OPTION_PASSWORD].set && state[OPTION_PROTECTOR].set) /* 
> > password and key protector */
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +                       "a password and a key protector cannot both be 
> > set");
> > +
> >     if (state[OPTION_PASSWORD].set) /* password */
> >       {
> >         cargs.key_data = (grub_uint8_t *) state[OPTION_PASSWORD].arg;
> > @@ -1362,6 +1457,15 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > int argc, char **args)
> >     return grub_errno;
> >       }
> > +  if (state[OPTION_PROTECTOR].set) /* key protector(s) */
> > +    {
> > +      cargs.key_cache = grub_zalloc (state[OPTION_PROTECTOR].set * sizeof 
> > (*cargs.key_cache));
> > +      if (!cargs.key_cache)
> > +        return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> > +                           "no memory for key protector key cache");
> > +      cargs.protectors = state[OPTION_PROTECTOR].args;
> > +    }
> > +
> >     if (state[OPTION_UUID].set) /* uuid */
> >       {
> >         int found_uuid;
> > @@ -1370,6 +1474,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >         dev = grub_cryptodisk_get_by_uuid (args[0]);
> >         if (dev)
> >     {
> > +          grub_cryptodisk_clear_key_cache (&cargs);
> 
> 
> 
> >       grub_dprintf ("cryptodisk",
> >                     "already mounted as crypto%lu\n", dev->id);
> >       return GRUB_ERR_NONE;
> > @@ -1378,6 +1483,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >         cargs.check_boot = state[OPTION_BOOT].set;
> >         cargs.search_uuid = args[0];
> >         found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, 
> > &cargs);
> > +      grub_cryptodisk_clear_key_cache (&cargs);
> >         if (found_uuid)
> >     return GRUB_ERR_NONE;
> > @@ -1397,6 +1503,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >       {
> >         cargs.check_boot = state[OPTION_BOOT].set;
> >         grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > +      grub_cryptodisk_clear_key_cache (&cargs);
> >         return GRUB_ERR_NONE;
> >       }
> >     else
> > @@ -1420,6 +1527,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >         disk = grub_disk_open (diskname);
> >         if (!disk)
> >     {
> > +          grub_cryptodisk_clear_key_cache (&cargs);
> >       if (disklast)
> >         *disklast = ')';
> >       return grub_errno;
> > @@ -1430,12 +1538,14 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > int argc, char **args)
> >     {
> >       grub_dprintf ("cryptodisk", "already mounted as crypto%lu\n", 
> > dev->id);
> >       grub_disk_close (disk);
> > +          grub_cryptodisk_clear_key_cache (&cargs);
> >       if (disklast)
> >         *disklast = ')';
> >       return GRUB_ERR_NONE;
> >     }
> >         dev = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
> > +      grub_cryptodisk_clear_key_cache (&cargs);
> >         grub_disk_close (disk);
> >         if (disklast)
> > @@ -1576,6 +1686,7 @@ GRUB_MOD_INIT (cryptodisk)
> >     cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> >                           N_("[ [-p password] | [-k keyfile"
> >                              " [-O keyoffset] [-S keysize] ] ] [-H file]"
> > +                            " [-P protector [-P protector ...]]"
> >                              " <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 d94df68b6..ce18df883 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -70,6 +70,16 @@ typedef gcry_err_code_t
> >   (*grub_cryptodisk_rekey_func_t) (struct grub_cryptodisk *dev,
> >                              grub_uint64_t zoneno);
> > +struct grub_cryptomount_cached_key
> > +{
> > +  grub_uint8_t *key;
> > +  grub_size_t key_len;
> > +
> > +  /* The key protector associated with this cache entry failed, so avoid it
> > +   * even if the cached entry (an instance of this structure) is empty. */
> 
> Not a properly formatted multiline comment[1].
> 
Will fix in v3.

> Overall this looks okay, even if I'm still not in love with the key
> protectors user interface (but like the idea).
> 

Thanks for reviewing the patch!

Gary Lin

> Glenn
> 
> [1] 
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
> 
> > +  int invalid;
> > +};
> > +
> >   struct grub_cryptomount_args
> >   {
> >     /* scan: Flag to indicate that only bootable volumes should be 
> > decrypted */
> > @@ -81,6 +91,10 @@ struct grub_cryptomount_args
> >     /* recover_key: Length of key_data */
> >     grub_size_t key_len;
> >     grub_file_t hdr_file;
> > +  /* recover_key: Names of the key protectors to use (NULL-terminated) */
> > +  char **protectors;
> > +  /* recover_key: Key cache to avoid invoking the same key protector twice 
> > */
> > +  struct grub_cryptomount_cached_key *key_cache;
> >   };
> >   typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
>