Re: [PATCH 4/5] cryptodisk: Support key protectors

[Glenn Washburn]

On Mon, 24 Jan 2022 06:12:17 -0800
Hernan Gatta <hegatta@linux.microsoft.com> wrote:

> From: Hernan Gatta <hegatta@microsoft.com>
> 
> Add a new parameter to cryptomount to support the key protectors framework: 
> -k.
> This parameter is accepted whenever the cryptomount command is used to mount a
> specific disk either via a disk specification (e.g., (hd0,gpt1)) or by UUID, 
> but
> not when disks are mounted in bulk (i.e., via -a or -b). The parameter is used
> to automatically retrieve a key from the specified key protector.

Why shouldn't key protectors be used to try to unlock all devices that
a backend identified as being able to unlock? Specifically, can a key
stored via the TPM be used for multiple devices? With respect to a
future unencrypted keyfile protector, it is certainly the case that it
can be applied to multiple devices.

> 
> Signed-off-by: <Hernan Gatta hegatta@linux.microsoft.com>
> ---
>  Makefile.util.def           |  1 +
>  grub-core/disk/cryptodisk.c | 21 ++++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.util.def b/Makefile.util.def
> index f8b356c..39b53b3 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 4970973..176dd56 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>
> @@ -42,6 +43,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},
> +    {"protector", 'k', 0, N_("Unlock disk using the specified key 
> protector."), 0, ARG_TYPE_STRING},

This conflicts with the keyfile and detached header patch series as is.
Ultimately, this is probably the right way to go and there will be an
protector for using unencrypted keyfiles.

Another option is to use -P for protectors and -k for keyfiles and have
two separate methods of using unencrypted keyfiles. I generally don't
like having two methods, the only benefit is that I believe the
--keyfile method is more intuitive. To use the keyfile protector the
user unfamiliar with these features will likely need to consult the
documentation.

>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -1160,6 +1162,7 @@ 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};
> +  grub_err_t err;
>  
>    if (argc < 1 && !state[1].set && !state[2].set)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
> @@ -1167,12 +1170,28 @@ 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[3].set && state[4].set) /* password and key protector */
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                       "a password and a key protector cannot both be set");
> +
> +  if (state[4].set && argc < 1) /* key protector */
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                       "key protectors require a device name or UUID");

Referring to the my first comment, I think we can get rid of this if
block.

> +
>    if (state[3].set) /* password */
>      {
>        cargs.key_data = (grub_uint8_t *) state[3].arg;
>        cargs.key_len = grub_strlen (state[3].arg);
>      }
>  
> +  if (state[4].set) /* key protector */
> +    {
> +      err = grub_key_protector_recover_key (state[4].arg, &cargs.key_data,
> +                                                          &cargs.key_len);
> +      if (err)
> +        grub_printf_ (N_("Could not recover key from key protector.\n"));

Why is this not returning with an error here? Is it believed that
perhaps the device can be unlocked without recovering a key (even
though the user explicitly specified that a key should be used?)?

> +    }
> +
>    if (state[0].set) /* uuid */
>      {
>        int found_uuid;
> @@ -1385,7 +1404,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] [-k protector[:args]] 
> <SOURCE|-u UUID|-a|-b>"),

This looks eerily similiar to what I proposed to James in response to
his SEV patches[1]. As such, I am in favor of this syntax and it looks
to me like a framework that James can use for his SEV series.

Glenn

>                             N_("Mount a crypto device."), options);
>    grub_procfs_register ("luks_script", &luks_script);
>  }

[1] https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00105.html