Re: [PATCH 2/2] Cryptomount support key files

[Patrick Steinhardt]

On Fri, Feb 21, 2020 at 10:03:49PM +0100, Denis 'GNUtoo' Carikli wrote:
> From: John Lane <address@hidden>
> 
> Signed-off-by: John Lane <address@hidden>
> address@hidden: rebase
> Signed-off-by: Denis 'GNUtoo' Carikli <address@hidden>

Same remarks with regards to commit subject and message as for the other
patch.

> ---
>  grub-core/disk/cryptodisk.c | 46 ++++++++++++++++++++++++++++++++++++-
>  grub-core/disk/geli.c       |  4 +++-
>  grub-core/disk/luks.c       | 44 ++++++++++++++++++++++++-----------
>  include/grub/cryptodisk.h   |  5 +++-
>  include/grub/file.h         |  2 ++
>  5 files changed, 84 insertions(+), 17 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 6d4befc6f..ee2f300dd 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,9 @@ 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},
>      {"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STRING},
> +    {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
> +    {"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},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -972,6 +975,8 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>  static int check_boot, have_it;
>  static char *search_uuid;
>  static grub_file_t hdr;
> +static grub_uint8_t *key, keyfile_buffer[GRUB_CRYPTODISK_MAX_KEYFILE_SIZE];
> +static grub_size_t keyfile_size;
>  
>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
> @@ -1002,7 +1007,7 @@ grub_cryptodisk_scan_device_real (const char *name, 
> grub_disk_t source)
>      if (!dev)
>        continue;
>      
> -    err = cr->recover_key (source, dev, hdr);
> +    err = cr->recover_key (source, dev, hdr, key, keyfile_size);
>      if (err)
>      {
>        cryptodisk_close (dev);
> @@ -1110,6 +1115,45 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>      hdr = NULL;
>  
>    have_it = 0;
> +  key = NULL;
> +
> +  if (state[4].set) /* Key file; fails back to passphrase entry */
> +    {
> +      grub_file_t keyfile;
> +      int keyfile_offset;
> +      grub_size_t requested_keyfile_size;
> +
> +      requested_keyfile_size = state[6].set ? grub_strtoul(state[6].arg, 0, 
> 0) : 0;

I think we should check for conversion errors here when calling
`grub_strtoul()`. Defaulting to `GRUB_CRYPTODISK_MAX_KEYFILE_SIZE` in
case the user has typoed seems surprising to me.

> +      if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> +        grub_printf (N_("Key file size exceeds maximum (%llu)\n"), \
> +                          (unsigned long long) 
> GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);

Shouldn't we error out in this case? Same for the below error cases. If
we do so then we could also get rid of the cascading if statements by
returning early in case any error occurs.

> +      else
> +        {
> +          keyfile_offset = state[5].set ? grub_strtoul (state[5].arg, 0, 0) 
> : 0;
> +          keyfile_size = requested_keyfile_size ? requested_keyfile_size : \
> +                                          GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;

Hum. So if we're given a keyfile size, then we obviously should read
exactly that many bytes. I'd argue in the case where the user didn't
pass a size that we should not use GRUB_CRYPTODISK_MAX_KEYFILE_SIZE, but
instead stat the file and read it in full. The benefit would be that we
know how many bytes to expect when calling `grub_file_read()`.

> +          keyfile = grub_file_open (state[4].arg, 
> GRUB_FILE_TYPE_LUKS_KEY_FILE);
> +          if (!keyfile)
> +            grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
> +          else if (grub_file_seek (keyfile, keyfile_offset) == 
> (grub_off_t)-1)
> +            grub_printf (N_("Unable to seek to offset %d in key file\n"), 
> keyfile_offset);
> +          else
> +            {
> +              keyfile_size = grub_file_read (keyfile, keyfile_buffer, 
> keyfile_size);
> +              if (keyfile_size == (grub_size_t)-1)
> +                 grub_printf (N_("Error reading key file\n"));
> +           else if (requested_keyfile_size && (keyfile_size != 
> requested_keyfile_size))
> +                 grub_printf (N_("Cannot read %llu bytes for key file (read 
> %llu bytes)\n"),
> +                                                (unsigned long long) 
> requested_keyfile_size,
> +                                             (unsigned long long) 
> keyfile_size);
> +              else
> +                key = keyfile_buffer;
> +         }
> +        }
> +    }
> +

Indentation in the above block is wrong, eight consecutive spaces should
be converted to a tab.

>    if (state[0].set)
>      {
>        grub_cryptodisk_t dev;
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index f4394eb42..da6aa6a63 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -401,7 +401,9 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>  
>  static grub_err_t
>  recover_key (grub_disk_t source, grub_cryptodisk_t dev,
> -          grub_file_t hdr __attribute__ ((unused)) )
> +          grub_file_t hdr __attribute__ ((unused)),
> +          grub_uint8_t *key __attribute__ ((unused)),
> +          grub_size_t keyfile_size __attribute__ ((unused)) )
>  {
>    grub_size_t keysize;
>    grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];

Same as with the other patch, you'll also have to adjust
"grub-core/disk/luks2.c".

> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 950e89237..54b1cfe70 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -164,12 +164,16 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>  static grub_err_t
>  luks_recover_key (grub_disk_t source,
>                 grub_cryptodisk_t dev,
> -               grub_file_t hdr)
> +               grub_file_t hdr,
> +               grub_uint8_t *keyfile_bytes,
> +               grub_size_t keyfile_bytes_size)
>  {
>    struct grub_luks_phdr header;
>    grub_size_t keysize;
>    grub_uint8_t *split_key = NULL;
> -  char passphrase[MAX_PASSPHRASE] = "";
> +  char interactive_passphrase[MAX_PASSPHRASE] = "";
> +  grub_uint8_t *passphrase;
> +  grub_size_t passphrase_length;
>    grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
>    unsigned i;
>    grub_size_t length;
> @@ -206,18 +210,30 @@ luks_recover_key (grub_disk_t source,
>    if (!split_key)
>      return grub_errno;
>  
> -  /* Get the passphrase from the user.  */
> -  tmp = NULL;
> -  if (source->partition)
> -    tmp = grub_partition_get_name (source->partition);
> -  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> -            source->partition ? "," : "", tmp ? : "",
> -            dev->uuid);
> -  grub_free (tmp);
> -  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> +  if (keyfile_bytes)
>      {
> -      grub_free (split_key);
> -      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> +      /* Use bytestring from key file as passphrase */
> +      passphrase = keyfile_bytes;
> +      passphrase_length = keyfile_bytes_size;
> +    }
> +  else
> +    {
> +      /* Get the passphrase from the user.  */
> +      tmp = NULL;
> +      if (source->partition)
> +        tmp = grub_partition_get_name (source->partition);
> +      grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +                 source->partition ? "," : "", tmp ? : "", dev->uuid);
> +      grub_free (tmp);
> +      if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
> +        {
> +          grub_free (split_key);
> +          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not 
> supplied");
> +        }
> +
> +      passphrase = (grub_uint8_t *)interactive_passphrase;
> +      passphrase_length = grub_strlen (interactive_passphrase);
> +
>      }

Please remove the newline before the brace.

The rest looks fine to me.

Patrick

signature.asc
Description: PGP signature