On Wed, 17 Nov 2021 20:10:21 +0100
Daniel Kiper <dki...@net-space.pl> wrote:
> 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 <developm...@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"/?
Ok, when moving code, I generally don't like to change it unless
necesary. I'll add these changes.
> > + 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...
Ok, I'll try to keep that in mind. There's quite a few grub_error()
calls in cryptodisk that do not conform to that (and I expect else
where in the source).
> > + 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.
Are you saying you want the line to be " error:"? There are labels in
the source which are preceeded by whitespace, but they seem to be in
the minority. What's the rationale for this? or is it just personal
preference? I don't mind making this change, but I don't understand it.
>
> > + 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?
I'm not sure why. I think that's a question for Patrick. I'd guess he
figured it was a resonable upper bound on the length of the cipher
string.
> > + 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
Glenn
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
