Re: [PATCH v3 2/4] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk

[Daniel Kiper]

On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> The crypto device modules should only be setting up the crypto devices and
> not getting user input. This has the added benefit of simplifying the code
> such that three essentially duplicate pieces of code are merged into one.

Mostly nitpicks below...

> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 52 ++++++++++++++++++++++++++++++-------
>  grub-core/disk/geli.c       | 26 ++++---------------
>  grub-core/disk/luks.c       | 27 +++----------------
>  grub-core/disk/luks2.c      | 26 ++++---------------
>  include/grub/cryptodisk.h   |  1 +
>  5 files changed, 57 insertions(+), 75 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 577942088..a5f7b860c 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
>                                 grub_disk_t source,
>                                 grub_cryptomount_args_t cargs)
>  {
> -  grub_err_t err;
> +  grub_err_t ret = GRUB_ERR_NONE;
>    grub_cryptodisk_t dev;
>    grub_cryptodisk_dev_t cr;
> +  int askpass = 0;
> +  char *part = NULL;
>
>    dev = grub_cryptodisk_get_by_source_disk (source);
>
> @@ -1017,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
>        return grub_errno;
>      if (!dev)
>        continue;
> -
> -    err = cr->recover_key (source, dev, cargs);
> -    if (err)
> -    {
> -      cryptodisk_close (dev);
> -      return err;
> -    }
> +
> +    if (cargs->key_len == 0)

I am OK with "if (!cargs->key_len)" for all kinds of numeric values...

> +      {
> +     /* Get the passphrase from the user, if no key data. */
> +     askpass = 1;
> +     if (source->partition)

...but not for the pointers and enums.

s/if (source->partition)/if (source->partition != NULL)/

> +       part = grub_partition_get_name (source->partition);
> +     grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +                  source->partition ? "," : "", part ? : "",

s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/

s/part ? : ""/part != NULL ? part : "NO NAME"/?

> +                  dev->uuid);
> +     grub_free (part);
> +
> +     cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> +     if (!cargs->key_data)

Ditto and below please...

> +       return grub_errno;
> +
> +     if (!grub_password_get ((char *) cargs->key_data, 
> GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +       {
> +         ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");

All errors printed by grub_error() should start with lower case...

> +         goto error;
> +       }
> +     cargs->key_len = grub_strlen ((char *) cargs->key_data);
> +      }
> +
> +    ret = cr->recover_key (source, dev, cargs);
> +    if (ret)

if (ret != GRUB_ERR_NONE)

> +      goto error;
>
>      grub_cryptodisk_insert (dev, name, source);
>
>      have_it = 1;
>
> -    return GRUB_ERR_NONE;
> +    goto cleanup;
>    }
> -  return GRUB_ERR_NONE;
> +  goto cleanup;
> +
> +error:

Please put space before labels.

> +  cryptodisk_close (dev);

I would add empty line here.

> +cleanup:

Please put space before labels.

> +  if (askpass)
> +    {
> +      cargs->key_len = 0;
> +      grub_free (cargs->key_data);
> +    }
> +  return ret;
>  }
>
>  #ifdef GRUB_UTIL
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index 4e8c377e7..32f34d5c3 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -135,8 +135,6 @@ const char *algorithms[] = {
>    [0x16] = "aes"
>  };
>
> -#define MAX_PASSPHRASE 256
> -
>  static gcry_err_code_t
>  geli_rekey (struct grub_cryptodisk *dev, grub_uint64_t zoneno)
>  {
> @@ -406,17 +404,14 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, 
> grub_cryptomount_args_t
>    grub_uint8_t verify_key[GRUB_CRYPTO_MAX_MDLEN];
>    grub_uint8_t zero[GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE];
>    grub_uint8_t geli_cipher_key[64];
> -  char passphrase[MAX_PASSPHRASE] = "";
>    unsigned i;
>    gcry_err_code_t gcry_err;
>    struct grub_geli_phdr header;
> -  char *tmp;
>    grub_disk_addr_t sector;
>    grub_err_t err;
>
> -  /* Keyfiles are not implemented yet */
> -  if (cargs->key_data || cargs->key_len)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +  if (cargs->key_data == NULL || cargs->key_len == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");

All errors printed by grub_error() should start with lower case...

>    if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
>      return grub_error (GRUB_ERR_BUG, "cipher block is too long");
> @@ -438,23 +433,12 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, 
> grub_cryptomount_args_t
>
>    grub_puts_ (N_("Attempting to decrypt master key..."));
>
> -  /* 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))
> -    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> -
>    /* Calculate the PBKDF2 of the user supplied passphrase.  */
>    if (grub_le_to_cpu32 (header.niter) != 0)
>      {
>        grub_uint8_t pbkdf_key[64];
> -      gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
> -                                  grub_strlen (passphrase),
> +      gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data,
> +                                  cargs->key_len,
>                                    header.salt,
>                                    sizeof (header.salt),
>                                    grub_le_to_cpu32 (header.niter),
> @@ -477,7 +461,7 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, 
> grub_cryptomount_args_t
>       return grub_crypto_gcry_error (GPG_ERR_OUT_OF_MEMORY);
>
>        grub_crypto_hmac_write (hnd, header.salt, sizeof (header.salt));
> -      grub_crypto_hmac_write (hnd, passphrase, grub_strlen (passphrase));
> +      grub_crypto_hmac_write (hnd, cargs->key_data, cargs->key_len);
>
>        gcry_err = grub_crypto_hmac_fini (hnd, geomkey);
>        if (gcry_err)
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 0462edc6e..51646cefe 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -29,8 +29,6 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> -#define MAX_PASSPHRASE 256
> -
>  #define LUKS_KEY_ENABLED  0x00AC71F3
>
>  /* On disk LUKS header */
> @@ -158,17 +156,14 @@ luks_recover_key (grub_disk_t source,
>    struct grub_luks_phdr header;
>    grub_size_t keysize;
>    grub_uint8_t *split_key = NULL;
> -  char passphrase[MAX_PASSPHRASE] = "";
>    grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
>    unsigned i;
>    grub_size_t length;
>    grub_err_t err;
>    grub_size_t max_stripes = 1;
> -  char *tmp;
>
> -  /* Keyfiles are not implemented yet */
> -  if (cargs->key_data || cargs->key_len)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> + if (cargs->key_data == NULL || cargs->key_len == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");

Same as above...

>    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>    if (err)
> @@ -188,20 +183,6 @@ 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))
> -    {
> -      grub_free (split_key);
> -      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> -    }
> -
>    /* Try to recover master key from each active keyslot.  */
>    for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
>      {
> @@ -216,8 +197,8 @@ luks_recover_key (grub_disk_t source,
>        grub_dprintf ("luks", "Trying keyslot %d\n", i);
>
>        /* Calculate the PBKDF2 of the user supplied passphrase.  */
> -      gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
> -                                  grub_strlen (passphrase),
> +      gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data,
> +                                  cargs->key_len,
>                                    header.keyblock[i].passwordSalt,
>                                    sizeof (header.keyblock[i].passwordSalt),
>                                    grub_be_to_cpu32 (header.keyblock[i].
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 455a78cb0..c77380cbb 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -35,8 +35,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define LUKS_MAGIC_1ST "LUKS\xBA\xBE"
>  #define LUKS_MAGIC_2ND "SKUL\xBA\xBE"
>
> -#define MAX_PASSPHRASE 256
> -
>  enum grub_luks2_kdf_type
>  {
>    LUKS2_KDF_TYPE_ARGON2I,
> @@ -546,8 +544,8 @@ luks2_recover_key (grub_disk_t source,
>                  grub_cryptomount_args_t cargs)
>  {
>    grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
> -  char passphrase[MAX_PASSPHRASE], cipher[32];
> -  char *json_header = NULL, *part = NULL, *ptr;
> +  char cipher[32];

If you change that could you add a comment why cipher length is equal 32?

> +  char *json_header = NULL, *ptr;
>    grub_size_t candidate_key_len = 0, json_idx, size;
>    grub_luks2_header_t header;
>    grub_luks2_keyslot_t keyslot;
> @@ -557,9 +555,8 @@ luks2_recover_key (grub_disk_t source,
>    grub_json_t *json = NULL, keyslots;
>    grub_err_t ret;
>
> -  /* Keyfiles are not implemented yet */
> -  if (cargs->key_data || cargs->key_len)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +  if (cargs->key_data == NULL || cargs->key_len == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");

Same as above...

Daniel