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

[Andrei Borzenkov]

В Tue, 16 Jun 2015 10:11:13 +0100
John Lane <address@hidden> пишет:

> ---
>  grub-core/disk/cryptodisk.c | 34 +++++++++++++++++++++++++++++++--
>  grub-core/disk/geli.c       |  4 +++-
>  grub-core/disk/luks.c       | 46 
> +++++++++++++++++++++++++++++++--------------
>  include/grub/cryptodisk.h   |  5 ++++-
>  4 files changed, 71 insertions(+), 18 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 65b3a01..fbe7db9 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,10 @@ 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},
> +    {"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
It is unused

> +    {"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}
>    };
>  
> @@ -805,6 +809,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;
>  

Someone should really get rid of static variables and pass them as
callback data.

>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
> @@ -835,7 +841,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);

You never clear key variable, so after the first call all subsequent
invocations will always use it for any device. Moving to proper
callback data would make such errors less likely.

>      if (err)
>      {
>        cryptodisk_close (dev);
> @@ -938,12 +944,36 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>        hdr = grub_file_open (state[3].arg);
>        if (!hdr)
>          return grub_errno;
> -      grub_printf ("\nUsing detached header %s\n", state[3].arg);

You remove line just added in previous patch? Why previous patch added
it then?

>      }
>    else
>      hdr = NULL;
>  
>    have_it = 0;
> +
> +  if (state[4].set) /* Key file */
> +    {
> +      grub_file_t keyfile;
> +      int keyfile_offset;
> +
> +      key = keyfile_buffer;
> +      keyfile_offset = state[6].set ? grub_strtoul (state[6].arg, 0, 0) : 0;
> +      keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0, 0) : \
> +                                          GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
> +
> +      keyfile = grub_file_open (state[4].arg);
> +      if (!keyfile)
> +        return grub_errno;
> +
> +      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
> +        return grub_errno;
> +
> +      keyfile_size = grub_file_read (keyfile, key, keyfile_size);
> +      if (keyfile_size == (grub_size_t)-1)
> +         return grub_errno;

If keyfile size is explicitly given, I expect that short read should be
considered an error. Otherwise what is the point of explicitly giving
size?

> +    }
> +  else
> +    key = NULL;
> +
>    if (state[0].set)
>      {
>        grub_cryptodisk_t dev;
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index f4394eb..da6aa6a 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];
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 66e64c0..0d0db9d 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -136,6 +136,8 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>    ciphermode[sizeof (header.cipherMode)] = 0;
>    grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
>    hashspec[sizeof (header.hashSpec)] = 0;
> +  grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
> +  uuid[sizeof (header.uuid)] = 0;
>  

How exactly this is related to keyfile support?

>    ciph = grub_crypto_lookup_cipher_by_name (ciphername);
>    if (!ciph)
> @@ -322,12 +324,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;
> @@ -364,18 +370,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

I'm not sure if this should be "else". I think normal behavior of user
space is to ask for password then. If keyfile fails we cannot continue
anyway.

> +    {
> +      /* 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);
> +
>      }
>  
>    /* Try to recover master key from each active keyslot.  */
> @@ -393,7 +411,7 @@ luks_recover_key (grub_disk_t source,
>  
>        /* Calculate the PBKDF2 of the user supplied passphrase.  */
>        gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
> -                                  grub_strlen (passphrase),
> +                                  passphrase_length,
>                                    header.keyblock[i].passwordSalt,
>                                    sizeof (header.keyblock[i].passwordSalt),
>                                    grub_be_to_cpu32 (header.keyblock[i].
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 16dee3c..0299625 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -55,6 +55,8 @@ typedef enum
>  #define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
>  #define GRUB_CRYPTODISK_MAX_KEYLEN 128
>  
> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
> +

Why it is different from MAX_KEYLEN? 

>  struct grub_cryptodisk;
>  
>  typedef gcry_err_code_t
> @@ -108,7 +110,8 @@ struct grub_cryptodisk_dev
>  
>    grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
>                            int boot_only, grub_file_t hdr);
> -  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, 
> grub_file_t hdr);
> +  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,
> +                         grub_file_t hdr, grub_uint8_t *key, grub_size_t 
> keyfile_size);
>  };
>  typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
>