* [PATCH 2/7] Cryptomount support key files
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
@ 2018-03-14 9:44 ` John Lane
2018-03-17 11:10 ` TJ
2018-03-14 9:45 ` [PATCH 3/7] cryptomount luks allow multiple passphrase attempts John Lane
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: John Lane @ 2018-03-14 9:44 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
From: John Lane <john@lane.uk.net>
---
grub-core/disk/cryptodisk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
grub-core/disk/geli.c | 4 +++-
grub-core/disk/luks.c | 44 +++++++++++++++++++++++++++++--------------
include/grub/cryptodisk.h | 5 ++++-
4 files changed, 82 insertions(+), 17 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 5230a5a9a..5261af547 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}
};
@@ -811,6 +814,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)
@@ -841,7 +846,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);
@@ -949,6 +954,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;
+
+ 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);
+ 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;
+
+ keyfile = grub_file_open (state[4].arg);
+ 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;
+ }
+ }
+ }
+
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];
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 66e64c0e0..588236888 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -322,12 +322,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 +368,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);
+
}
/* Try to recover master key from each active keyslot. */
@@ -393,7 +409,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 4e6e89a93..67f6b0b59 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
+
struct grub_cryptodisk;
typedef gcry_err_code_t
@@ -109,7 +111,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;
--
2.16.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/7] Cryptomount support key files
2018-03-14 9:44 ` [PATCH 2/7] Cryptomount support key files John Lane
@ 2018-03-17 11:10 ` TJ
2018-03-18 20:29 ` John Lane
0 siblings, 1 reply; 20+ messages in thread
From: TJ @ 2018-03-17 11:10 UTC (permalink / raw)
To: grub-devel
On 14/03/18 09:44, John Lane wrote:
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -949,6 +954,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;
> +
> + 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);
> + 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;
> +
> + keyfile = grub_file_open (state[4].arg);
> + 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_file_read() returns grub_ssize_t (signed). Is casting to
grub_size_t (unsigned) required or going to work as intended?
Is the only possible error -1? Underlying readwrite functions can return
error codes via grub_error() that are > 0: see include/grub/err.h
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/7] Cryptomount support key files
2018-03-17 11:10 ` TJ
@ 2018-03-18 20:29 ` John Lane
0 siblings, 0 replies; 20+ messages in thread
From: John Lane @ 2018-03-18 20:29 UTC (permalink / raw)
To: grub-devel, TJ
On 17/03/18 11:10, TJ wrote:
> On 14/03/18 09:44, John Lane wrote:
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -949,6 +954,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;
>> +
>> + 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);
>> + 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;
>> +
>> + keyfile = grub_file_open (state[4].arg);
>> + 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_file_read() returns grub_ssize_t (signed). Is casting to
> grub_size_t (unsigned) required or going to work as intended?
>
> Is the only possible error -1? Underlying readwrite functions can return
> error codes via grub_error() that are > 0: see include/grub/err.h
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
I have applied these changes. Builds clean but I need to test it. I
might have time to do that tomorrow, otherwise next weekend will be soonest.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/7] cryptomount luks allow multiple passphrase attempts
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
2018-03-14 9:44 ` [PATCH 2/7] Cryptomount support key files John Lane
@ 2018-03-14 9:45 ` John Lane
2018-03-17 11:10 ` TJ
2018-03-14 9:45 ` [PATCH 4/7] Cryptomount support plain dm-crypt John Lane
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: John Lane @ 2018-03-14 9:45 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
From: John Lane <john@lane.uk.net>
---
grub-core/disk/luks.c | 278 ++++++++++++++++++++++++++------------------------
1 file changed, 143 insertions(+), 135 deletions(-)
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 588236888..11e437edb 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -321,10 +321,10 @@ 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_uint8_t *keyfile_bytes,
- grub_size_t keyfile_bytes_size)
+ grub_cryptodisk_t dev,
+ grub_file_t hdr,
+ grub_uint8_t *keyfile_bytes,
+ grub_size_t keyfile_bytes_size)
{
struct grub_luks_phdr header;
grub_size_t keysize;
@@ -339,6 +339,7 @@ luks_recover_key (grub_disk_t source,
grub_size_t max_stripes = 1;
char *tmp;
grub_uint32_t sector;
+ unsigned attempts = 2;
err = GRUB_ERR_NONE;
@@ -361,151 +362,158 @@ luks_recover_key (grub_disk_t source,
for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED
- && grub_be_to_cpu32 (header.keyblock[i].stripes) > max_stripes)
+ && grub_be_to_cpu32 (header.keyblock[i].stripes) > max_stripes)
max_stripes = grub_be_to_cpu32 (header.keyblock[i].stripes);
split_key = grub_malloc (keysize * max_stripes);
if (!split_key)
return grub_errno;
- if (keyfile_bytes)
+ while (attempts)
{
- /* 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))
+ if (keyfile_bytes)
{
- 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. */
- for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
- {
- gcry_err_code_t gcry_err;
- grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
- grub_uint8_t digest[GRUB_CRYPTODISK_MAX_KEYLEN];
-
- /* Check if keyslot is enabled. */
- if (grub_be_to_cpu32 (header.keyblock[i].active) != LUKS_KEY_ENABLED)
- continue;
-
- 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,
- passphrase_length,
- header.keyblock[i].passwordSalt,
- sizeof (header.keyblock[i].passwordSalt),
- grub_be_to_cpu32 (header.keyblock[i].
- passwordIterations),
- digest, keysize);
-
- if (gcry_err)
- {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
- }
-
- grub_dprintf ("luks", "PBKDF2 done\n");
-
- gcry_err = grub_cryptodisk_setkey (dev, digest, keysize);
- if (gcry_err)
- {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
- }
-
- sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
- length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
-
- /* Read and decrypt the key material from the disk. */
- if (hdr)
- {
- grub_file_seek (hdr, sector * 512);
- if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
- err = GRUB_ERR_READ_ERROR;
+ /* Use bytestring from key file as passphrase */
+ passphrase = keyfile_bytes;
+ passphrase_length = keyfile_bytes_size;
+ keyfile_bytes = NULL; /* use it only once */
}
else
- err = grub_disk_read (source, sector, 0, length, split_key);
- if (err)
- {
- grub_free (split_key);
- return err;
- }
-
- gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
- if (gcry_err)
- {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
- }
-
- /* Merge the decrypted key material to get the candidate master key. */
- gcry_err = AF_merge (dev->hash, split_key, candidate_key, keysize,
- grub_be_to_cpu32 (header.keyblock[i].stripes));
- if (gcry_err)
- {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
- }
-
- grub_dprintf ("luks", "candidate key recovered\n");
-
- /* Calculate the PBKDF2 of the candidate master key. */
- gcry_err = grub_crypto_pbkdf2 (dev->hash, candidate_key,
- grub_be_to_cpu32 (header.keyBytes),
- header.mkDigestSalt,
- sizeof (header.mkDigestSalt),
- grub_be_to_cpu32
- (header.mkDigestIterations),
- candidate_digest,
- sizeof (candidate_digest));
- if (gcry_err)
- {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
- }
-
- /* Compare the calculated PBKDF2 to the digest stored
- in the header to see if it's correct. */
- if (grub_memcmp (candidate_digest, header.mkDigest,
- sizeof (header.mkDigest)) != 0)
- {
- grub_dprintf ("luks", "bad digest\n");
- continue;
- }
+ {
+ /* 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);
- /* TRANSLATORS: It's a cryptographic key slot: one element of an array
- where each element is either empty or holds a key. */
- grub_printf_ (N_("Slot %d opened\n"), i);
+ }
- /* Set the master key. */
- gcry_err = grub_cryptodisk_setkey (dev, candidate_key, keysize);
- if (gcry_err)
- {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
- }
+ /* Try to recover master key from each active keyslot. */
+ for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
+ {
+ gcry_err_code_t gcry_err;
+ grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
+ grub_uint8_t digest[GRUB_CRYPTODISK_MAX_KEYLEN];
+
+ /* Check if keyslot is enabled. */
+ if (grub_be_to_cpu32 (header.keyblock[i].active) != LUKS_KEY_ENABLED)
+ continue;
+
+ 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,
+ passphrase_length,
+ header.keyblock[i].passwordSalt,
+ sizeof (header.keyblock[i].passwordSalt),
+ grub_be_to_cpu32 (header.keyblock[i].
+ passwordIterations),
+ digest, keysize);
+
+ if (gcry_err)
+ {
+ grub_free (split_key);
+ return grub_crypto_gcry_error (gcry_err);
+ }
+
+ grub_dprintf ("luks", "PBKDF2 done\n");
+
+ gcry_err = grub_cryptodisk_setkey (dev, digest, keysize);
+ if (gcry_err)
+ {
+ grub_free (split_key);
+ return grub_crypto_gcry_error (gcry_err);
+ }
+
+ sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
+ length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
+
+ /* Read and decrypt the key material from the disk. */
+ if (hdr)
+ {
+ grub_file_seek (hdr, sector * 512);
+ if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
+ err = GRUB_ERR_READ_ERROR;
+ }
+ else
+ err = grub_disk_read (source, sector, 0, length, split_key);
+ if (err)
+ {
+ grub_free (split_key);
+ return err;
+ }
+
+ gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
+ if (gcry_err)
+ {
+ grub_free (split_key);
+ return grub_crypto_gcry_error (gcry_err);
+ }
+
+ /* Merge the decrypted key material to get the candidate master key. */
+ gcry_err = AF_merge (dev->hash, split_key, candidate_key, keysize,
+ grub_be_to_cpu32 (header.keyblock[i].stripes));
+ if (gcry_err)
+ {
+ grub_free (split_key);
+ return grub_crypto_gcry_error (gcry_err);
+ }
+
+ grub_dprintf ("luks", "candidate key recovered\n");
+
+ /* Calculate the PBKDF2 of the candidate master key. */
+ gcry_err = grub_crypto_pbkdf2 (dev->hash, candidate_key,
+ grub_be_to_cpu32 (header.keyBytes),
+ header.mkDigestSalt,
+ sizeof (header.mkDigestSalt),
+ grub_be_to_cpu32
+ (header.mkDigestIterations),
+ candidate_digest,
+ sizeof (candidate_digest));
+ if (gcry_err)
+ {
+ grub_free (split_key);
+ return grub_crypto_gcry_error (gcry_err);
+ }
+
+ /* Compare the calculated PBKDF2 to the digest stored
+ in the header to see if it's correct. */
+ if (grub_memcmp (candidate_digest, header.mkDigest,
+ sizeof (header.mkDigest)) != 0)
+ {
+ grub_dprintf ("luks", "bad digest\n");
+ continue;
+ }
+
+ /* TRANSLATORS: It's a cryptographic key slot: one element of an array
+ where each element is either empty or holds a key. */
+ grub_printf_ (N_("Slot %d opened\n"), i);
+
+ /* Set the master key. */
+ gcry_err = grub_cryptodisk_setkey (dev, candidate_key, keysize);
+ if (gcry_err)
+ {
+ grub_free (split_key);
+ return grub_crypto_gcry_error (gcry_err);
+ }
- grub_free (split_key);
+ grub_free (split_key);
- return GRUB_ERR_NONE;
+ return GRUB_ERR_NONE;
+ }
+ grub_printf_ (N_("Failed to decrypt master key.\n"));
+ if (--attempts) grub_printf_ (N_("%u attempt%s remaining.\n"), attempts,
+ (attempts==1) ? "" : "s");
}
grub_free (split_key);
--
2.16.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/7] cryptomount luks allow multiple passphrase attempts
2018-03-14 9:45 ` [PATCH 3/7] cryptomount luks allow multiple passphrase attempts John Lane
@ 2018-03-17 11:10 ` TJ
2018-03-18 20:30 ` John Lane
0 siblings, 1 reply; 20+ messages in thread
From: TJ @ 2018-03-17 11:10 UTC (permalink / raw)
To: The development of GNU GRUB, John Lane; +Cc: John Lane
On 14/03/18 09:45, John Lane wrote:
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -321,10 +321,10 @@ 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_uint8_t *keyfile_bytes,
> - grub_size_t keyfile_bytes_size)
> + grub_cryptodisk_t dev,
> + grub_file_t hdr,
> + grub_uint8_t *keyfile_bytes,
> + grub_size_t keyfile_bytes_size)
---8-<--- snip
Much of this patch is moving existing code around, could it be
refactored to avoid that so as to make the new code stand out?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/7] cryptomount luks allow multiple passphrase attempts
2018-03-17 11:10 ` TJ
@ 2018-03-18 20:30 ` John Lane
0 siblings, 0 replies; 20+ messages in thread
From: John Lane @ 2018-03-18 20:30 UTC (permalink / raw)
To: TJ, The development of GNU GRUB
On 17/03/18 11:10, TJ wrote:
> On 14/03/18 09:45, John Lane wrote:
>> --- a/grub-core/disk/luks.c
>> +++ b/grub-core/disk/luks.c
>> @@ -321,10 +321,10 @@ 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_uint8_t *keyfile_bytes,
>> - grub_size_t keyfile_bytes_size)
>> + grub_cryptodisk_t dev,
>> + grub_file_t hdr,
>> + grub_uint8_t *keyfile_bytes,
>> + grub_size_t keyfile_bytes_size)
>
> ---8-<--- snip
>
> Much of this patch is moving existing code around, could it be
> refactored to avoid that so as to make the new code stand out?
>
The code that was moved was just indented into a while loop.
Most of it pre-existed prior to my patches (#357-464), a small part was
added by patch#2. I'm not sure how I would refactor it - any change
would result in 100-ish lines changing position and/or indent and would
lead to a similarly sized patch.
The way the patch presents the changes is confusing but I am not sure
how to control that.
If it helps, all this patch did was add a while loop around the
passphrase reading code to allow the user 3 attempts. i.e
while (attempts)
{
<existing code which returns if successful>
grub_printf_ (N_("Failed to decrypt master key.\n"));
if (--attempts) grub_printf_ (N_("%u attempt%s remaining.\n"),
attempts,
(attempts==1) ? "" : "s");
}
<existing code returns access denied>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/7] Cryptomount support plain dm-crypt
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
2018-03-14 9:44 ` [PATCH 2/7] Cryptomount support key files John Lane
2018-03-14 9:45 ` [PATCH 3/7] cryptomount luks allow multiple passphrase attempts John Lane
@ 2018-03-14 9:45 ` John Lane
2018-03-14 9:45 ` [PATCH 5/7] Cryptomount support for hyphens in UUID John Lane
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: John Lane @ 2018-03-14 9:45 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
From: John Lane <john@lane.uk.net>
Patch modified to take into account a change to context
brought about by c93d3e694713b8230fa2cf88414fabe005b56782
grub-core/disk/cryptodisk.c
142c142
< if (disklast)
---
>
---
grub-core/disk/cryptodisk.c | 298 +++++++++++++++++++++++++++++++++++++++++++-
grub-core/disk/luks.c | 195 +----------------------------
include/grub/cryptodisk.h | 8 ++
3 files changed, 310 insertions(+), 191 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 5261af547..7f656f75c 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -45,6 +45,12 @@ static const struct grub_arg_option options[] =
{"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},
+ {"plain", 'p', 0, N_("Plain (no LUKS header)"), 0, ARG_TYPE_NONE},
+ {"cipher", 'c', 0, N_("Plain mode cipher"), 0, ARG_TYPE_STRING},
+ {"digest", 'd', 0, N_("Plain mode passphrase digest (hash)"), 0, ARG_TYPE_STRING},
+ {"offset", 'o', 0, N_("Plain mode data sector offset"), 0, ARG_TYPE_INT},
+ {"size", 's', 0, N_("Size of raw device (sectors, defaults to whole device)"), 0, ARG_TYPE_INT},
+ {"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
{0, 0, 0, 0, 0, 0}
};
@@ -933,6 +939,48 @@ grub_cryptodisk_scan_device (const char *name,
return have_it && search_uuid ? 1 : 0;
}
+/* Hashes a passphrase into a key and stores it with cipher. */
+static gcry_err_code_t
+set_passphrase (grub_cryptodisk_t dev, grub_size_t keysize, const char *passphrase)
+{
+ grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = derived_hash;
+ char *p;
+ unsigned int round, i;
+ unsigned int len, size;
+
+ /* Need no passphrase if there's no key */
+ if (keysize == 0)
+ return GPG_ERR_INV_KEYLEN;
+
+ /* Hack to support the "none" hash */
+ if (dev->hash)
+ len = dev->hash->mdlen;
+ else
+ len = grub_strlen (passphrase);
+
+ if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN || len > GRUB_CRYPTODISK_MAX_KEYLEN)
+ return GPG_ERR_INV_KEYLEN;
+
+ p = grub_malloc (grub_strlen (passphrase) + 2 + keysize / len);
+ if (!p)
+ return grub_errno;
+
+ for (round = 0, size = keysize; size; round++, dh += len, size -= len)
+ {
+ for (i = 0; i < round; i++)
+ p[i] = 'A';
+
+ grub_strcpy (p + i, passphrase);
+
+ if (len > size)
+ len = size;
+
+ grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
+ }
+
+ return grub_cryptodisk_setkey (dev, derived_hash, keysize);
+}
+
static grub_err_t
grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
{
@@ -1060,7 +1108,63 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
- err = grub_cryptodisk_scan_device_real (diskname, disk);
+ if (state[7].set) /* Plain mode */
+ {
+ char *cipher;
+ char *mode;
+ char *digest;
+ int offset, size, key_size;
+
+ cipher = grub_strdup (state[8].set ? state[8].arg : GRUB_CRYPTODISK_PLAIN_CIPHER);
+ digest = grub_strdup (state[9].set ? state[9].arg : GRUB_CRYPTODISK_PLAIN_DIGEST);
+ offset = state[10].set ? grub_strtoul (state[10].arg, 0, 0) : 0;
+ size = state[11].set ? grub_strtoul (state[11].arg, 0, 0) : 0;
+ key_size = ( state[12].set ? grub_strtoul (state[12].arg, 0, 0) \
+ : GRUB_CRYPTODISK_PLAIN_KEYSIZE ) / 8;
+
+ /* no strtok, do it manually */
+ mode = grub_strchr(cipher,'-');
+ if (!mode)
+ return GRUB_ERR_BAD_ARGUMENT;
+ else
+ *mode++ = 0;
+
+ dev = grub_cryptodisk_create (disk, NULL, cipher, mode, digest);
+
+ dev->offset = offset;
+ if (size) dev->total_length = size;
+
+ if (key)
+ {
+ err = grub_cryptodisk_setkey (dev, key, key_size);
+ if (err)
+ return err;
+ }
+ else
+ {
+ char passphrase[GRUB_CRYPTODISK_MAX_PASSPHRASE] = "";
+
+ grub_printf_ (N_("Enter passphrase for %s: "), diskname);
+ if (!grub_password_get (passphrase, GRUB_CRYPTODISK_MAX_PASSPHRASE))
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+
+ err = set_passphrase (dev, key_size, passphrase);
+ if (err)
+ {
+ grub_crypto_cipher_close (dev->cipher);
+ return err;
+ }
+ }
+
+ grub_cryptodisk_insert (dev, diskname, disk);
+
+ grub_free (cipher);
+ grub_free (digest);
+
+ err = GRUB_ERR_NONE;
+ }
+ else
+ err = grub_cryptodisk_scan_device_real (diskname, disk);
grub_disk_close (disk);
if (disklast)
@@ -1193,13 +1297,203 @@ struct grub_procfs_entry luks_script =
.get_contents = luks_script_get
};
+grub_cryptodisk_t
+grub_cryptodisk_create (grub_disk_t disk, char *uuid,
+ char *ciphername, char *ciphermode, char *hashspec)
+{
+ grub_cryptodisk_t newdev;
+ char *cipheriv = NULL;
+ grub_crypto_cipher_handle_t cipher = NULL, secondary_cipher = NULL;
+ grub_crypto_cipher_handle_t essiv_cipher = NULL;
+ const gcry_md_spec_t *hash = NULL, *essiv_hash = NULL;
+ const struct gcry_cipher_spec *ciph;
+ grub_cryptodisk_mode_t mode;
+ grub_cryptodisk_mode_iv_t mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN64;
+ int benbi_log = 0;
+
+ if (!uuid)
+ uuid = (char*)"00000000000000000000000000000000";
+
+ ciph = grub_crypto_lookup_cipher_by_name (ciphername);
+ if (!ciph)
+ {
+ grub_error (GRUB_ERR_FILE_NOT_FOUND, "Cipher %s isn't available",
+ ciphername);
+ return NULL;
+ }
+
+ /* Configure the cipher used for the bulk data. */
+ cipher = grub_crypto_cipher_open (ciph);
+ if (!cipher)
+ return NULL;
+
+ /* Configure the cipher mode. */
+ if (grub_strcmp (ciphermode, "ecb") == 0)
+ {
+ mode = GRUB_CRYPTODISK_MODE_ECB;
+ mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
+ cipheriv = NULL;
+ }
+ else if (grub_strcmp (ciphermode, "plain") == 0)
+ {
+ mode = GRUB_CRYPTODISK_MODE_CBC;
+ mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
+ cipheriv = NULL;
+ }
+ else if (grub_memcmp (ciphermode, "cbc-", sizeof ("cbc-") - 1) == 0)
+ {
+ mode = GRUB_CRYPTODISK_MODE_CBC;
+ cipheriv = ciphermode + sizeof ("cbc-") - 1;
+ }
+ else if (grub_memcmp (ciphermode, "pcbc-", sizeof ("pcbc-") - 1) == 0)
+ {
+ mode = GRUB_CRYPTODISK_MODE_PCBC;
+ cipheriv = ciphermode + sizeof ("pcbc-") - 1;
+ }
+ else if (grub_memcmp (ciphermode, "xts-", sizeof ("xts-") - 1) == 0)
+ {
+ mode = GRUB_CRYPTODISK_MODE_XTS;
+ cipheriv = ciphermode + sizeof ("xts-") - 1;
+ secondary_cipher = grub_crypto_cipher_open (ciph);
+ if (!secondary_cipher)
+ {
+ grub_crypto_cipher_close (cipher);
+ return NULL;
+ }
+ if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
+ {
+ grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+ cipher->cipher->blocksize);
+ grub_crypto_cipher_close (cipher);
+ grub_crypto_cipher_close (secondary_cipher);
+ return NULL;
+ }
+ if (secondary_cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
+ {
+ grub_crypto_cipher_close (cipher);
+ grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
+ secondary_cipher->cipher->blocksize);
+ grub_crypto_cipher_close (secondary_cipher);
+ return NULL;
+ }
+ }
+ else if (grub_memcmp (ciphermode, "lrw-", sizeof ("lrw-") - 1) == 0)
+ {
+ mode = GRUB_CRYPTODISK_MODE_LRW;
+ cipheriv = ciphermode + sizeof ("lrw-") - 1;
+ if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
+ {
+ grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported LRW block size: %d",
+ cipher->cipher->blocksize);
+ grub_crypto_cipher_close (cipher);
+ return NULL;
+ }
+ }
+ else
+ {
+ grub_crypto_cipher_close (cipher);
+ grub_error (GRUB_ERR_BAD_ARGUMENT, "Unknown cipher mode: %s",
+ ciphermode);
+ return NULL;
+ }
+
+ if (cipheriv == NULL);
+ else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
+ mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
+ else if (grub_memcmp (cipheriv, "plain64", sizeof ("plain64") - 1) == 0)
+ mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN64;
+ else if (grub_memcmp (cipheriv, "benbi", sizeof ("benbi") - 1) == 0)
+ {
+ if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
+ || cipher->cipher->blocksize == 0)
+ grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported benbi blocksize: %d",
+ cipher->cipher->blocksize);
+ /* FIXME should we return an error here? */
+ for (benbi_log = 0;
+ (cipher->cipher->blocksize << benbi_log) < GRUB_DISK_SECTOR_SIZE;
+ benbi_log++);
+ mode_iv = GRUB_CRYPTODISK_MODE_IV_BENBI;
+ }
+ else if (grub_memcmp (cipheriv, "null", sizeof ("null") - 1) == 0)
+ mode_iv = GRUB_CRYPTODISK_MODE_IV_NULL;
+ else if (grub_memcmp (cipheriv, "essiv:", sizeof ("essiv:") - 1) == 0)
+ {
+ char *hash_str = cipheriv + 6;
+
+ mode_iv = GRUB_CRYPTODISK_MODE_IV_ESSIV;
+
+ /* Configure the hash and cipher used for ESSIV. */
+ essiv_hash = grub_crypto_lookup_md_by_name (hash_str);
+ if (!essiv_hash)
+ {
+ grub_crypto_cipher_close (cipher);
+ grub_crypto_cipher_close (secondary_cipher);
+ grub_error (GRUB_ERR_FILE_NOT_FOUND,
+ "Couldn't load %s hash", hash_str);
+ return NULL;
+ }
+ essiv_cipher = grub_crypto_cipher_open (ciph);
+ if (!essiv_cipher)
+ {
+ grub_crypto_cipher_close (cipher);
+ grub_crypto_cipher_close (secondary_cipher);
+ return NULL;
+ }
+ }
+ else
+ {
+ grub_crypto_cipher_close (cipher);
+ grub_crypto_cipher_close (secondary_cipher);
+ grub_error (GRUB_ERR_BAD_ARGUMENT, "Unknown IV mode: %s",
+ cipheriv);
+ return NULL;
+ }
+
+ /* Configure the passphrase hash (LUKS also uses AF splitter and HMAC). */
+ hash = grub_crypto_lookup_md_by_name (hashspec);
+ if (!hash)
+ {
+ grub_crypto_cipher_close (cipher);
+ grub_crypto_cipher_close (essiv_cipher);
+ grub_crypto_cipher_close (secondary_cipher);
+ grub_error (GRUB_ERR_FILE_NOT_FOUND, "Couldn't load %s hash",
+ hashspec);
+ return NULL;
+ }
+
+ newdev = grub_zalloc (sizeof (struct grub_cryptodisk));
+ if (!newdev)
+ {
+ grub_crypto_cipher_close (cipher);
+ grub_crypto_cipher_close (essiv_cipher);
+ grub_crypto_cipher_close (secondary_cipher);
+ return NULL;
+ }
+ newdev->cipher = cipher;
+ newdev->offset = 0;
+ newdev->source_disk = NULL;
+ newdev->benbi_log = benbi_log;
+ newdev->mode = mode;
+ newdev->mode_iv = mode_iv;
+ newdev->secondary_cipher = secondary_cipher;
+ newdev->essiv_cipher = essiv_cipher;
+ newdev->essiv_hash = essiv_hash;
+ newdev->hash = hash;
+ newdev->log_sector_size = 9;
+ newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
+ grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
+ COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
+
+ return newdev;
+}
+
static grub_extcmd_t cmd;
GRUB_MOD_INIT (cryptodisk)
{
grub_disk_dev_register (&grub_cryptodisk_dev);
cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
- N_("SOURCE|-u UUID|-a|-b|-H file"),
+ N_("SOURCE|-u UUID|-a|-b|-H file|-p -c cipher -d digest"),
N_("Mount a crypto device."), options);
grub_procfs_register ("luks_script", &luks_script);
}
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 11e437edb..4ebe21b4e 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -30,8 +30,6 @@
GRUB_MOD_LICENSE ("GPLv3+");
-#define MAX_PASSPHRASE 256
-
#define LUKS_KEY_ENABLED 0x00AC71F3
/* On disk LUKS header */
@@ -76,15 +74,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
char uuid[sizeof (header.uuid) + 1];
char ciphername[sizeof (header.cipherName) + 1];
char ciphermode[sizeof (header.cipherMode) + 1];
- char *cipheriv = NULL;
char hashspec[sizeof (header.hashSpec) + 1];
- grub_crypto_cipher_handle_t cipher = NULL, secondary_cipher = NULL;
- grub_crypto_cipher_handle_t essiv_cipher = NULL;
- const gcry_md_spec_t *hash = NULL, *essiv_hash = NULL;
- const struct gcry_cipher_spec *ciph;
- grub_cryptodisk_mode_t mode;
- grub_cryptodisk_mode_iv_t mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN64;
- int benbi_log = 0;
grub_err_t err;
err = GRUB_ERR_NONE;
@@ -119,7 +109,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
iptr++)
{
if (*iptr != '-')
- *optr++ = *iptr;
+ *optr++ = *iptr;
}
*optr = 0;
@@ -129,6 +119,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
return NULL;
}
+
/* Make sure that strings are null terminated. */
grub_memcpy (ciphername, header.cipherName, sizeof (header.cipherName));
ciphername[sizeof (header.cipherName)] = 0;
@@ -137,184 +128,10 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
hashspec[sizeof (header.hashSpec)] = 0;
- ciph = grub_crypto_lookup_cipher_by_name (ciphername);
- if (!ciph)
- {
- grub_error (GRUB_ERR_FILE_NOT_FOUND, "Cipher %s isn't available",
- ciphername);
- return NULL;
- }
-
- /* Configure the cipher used for the bulk data. */
- cipher = grub_crypto_cipher_open (ciph);
- if (!cipher)
- return NULL;
-
- if (grub_be_to_cpu32 (header.keyBytes) > 1024)
- {
- grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid keysize %d",
- grub_be_to_cpu32 (header.keyBytes));
- grub_crypto_cipher_close (cipher);
- return NULL;
- }
-
- /* Configure the cipher mode. */
- if (grub_strcmp (ciphermode, "ecb") == 0)
- {
- mode = GRUB_CRYPTODISK_MODE_ECB;
- mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
- cipheriv = NULL;
- }
- else if (grub_strcmp (ciphermode, "plain") == 0)
- {
- mode = GRUB_CRYPTODISK_MODE_CBC;
- mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
- cipheriv = NULL;
- }
- else if (grub_memcmp (ciphermode, "cbc-", sizeof ("cbc-") - 1) == 0)
- {
- mode = GRUB_CRYPTODISK_MODE_CBC;
- cipheriv = ciphermode + sizeof ("cbc-") - 1;
- }
- else if (grub_memcmp (ciphermode, "pcbc-", sizeof ("pcbc-") - 1) == 0)
- {
- mode = GRUB_CRYPTODISK_MODE_PCBC;
- cipheriv = ciphermode + sizeof ("pcbc-") - 1;
- }
- else if (grub_memcmp (ciphermode, "xts-", sizeof ("xts-") - 1) == 0)
- {
- mode = GRUB_CRYPTODISK_MODE_XTS;
- cipheriv = ciphermode + sizeof ("xts-") - 1;
- secondary_cipher = grub_crypto_cipher_open (ciph);
- if (!secondary_cipher)
- {
- grub_crypto_cipher_close (cipher);
- return NULL;
- }
- if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
- {
- grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
- cipher->cipher->blocksize);
- grub_crypto_cipher_close (cipher);
- grub_crypto_cipher_close (secondary_cipher);
- return NULL;
- }
- if (secondary_cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
- {
- grub_crypto_cipher_close (cipher);
- grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported XTS block size: %d",
- secondary_cipher->cipher->blocksize);
- grub_crypto_cipher_close (secondary_cipher);
- return NULL;
- }
- }
- else if (grub_memcmp (ciphermode, "lrw-", sizeof ("lrw-") - 1) == 0)
- {
- mode = GRUB_CRYPTODISK_MODE_LRW;
- cipheriv = ciphermode + sizeof ("lrw-") - 1;
- if (cipher->cipher->blocksize != GRUB_CRYPTODISK_GF_BYTES)
- {
- grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported LRW block size: %d",
- cipher->cipher->blocksize);
- grub_crypto_cipher_close (cipher);
- return NULL;
- }
- }
- else
- {
- grub_crypto_cipher_close (cipher);
- grub_error (GRUB_ERR_BAD_ARGUMENT, "Unknown cipher mode: %s",
- ciphermode);
- return NULL;
- }
-
- if (cipheriv == NULL);
- else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
- mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
- else if (grub_memcmp (cipheriv, "plain64", sizeof ("plain64") - 1) == 0)
- mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN64;
- else if (grub_memcmp (cipheriv, "benbi", sizeof ("benbi") - 1) == 0)
- {
- if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
- || cipher->cipher->blocksize == 0)
- grub_error (GRUB_ERR_BAD_ARGUMENT, "Unsupported benbi blocksize: %d",
- cipher->cipher->blocksize);
- /* FIXME should we return an error here? */
- for (benbi_log = 0;
- (cipher->cipher->blocksize << benbi_log) < GRUB_DISK_SECTOR_SIZE;
- benbi_log++);
- mode_iv = GRUB_CRYPTODISK_MODE_IV_BENBI;
- }
- else if (grub_memcmp (cipheriv, "null", sizeof ("null") - 1) == 0)
- mode_iv = GRUB_CRYPTODISK_MODE_IV_NULL;
- else if (grub_memcmp (cipheriv, "essiv:", sizeof ("essiv:") - 1) == 0)
- {
- char *hash_str = cipheriv + 6;
-
- mode_iv = GRUB_CRYPTODISK_MODE_IV_ESSIV;
-
- /* Configure the hash and cipher used for ESSIV. */
- essiv_hash = grub_crypto_lookup_md_by_name (hash_str);
- if (!essiv_hash)
- {
- grub_crypto_cipher_close (cipher);
- grub_crypto_cipher_close (secondary_cipher);
- grub_error (GRUB_ERR_FILE_NOT_FOUND,
- "Couldn't load %s hash", hash_str);
- return NULL;
- }
- essiv_cipher = grub_crypto_cipher_open (ciph);
- if (!essiv_cipher)
- {
- grub_crypto_cipher_close (cipher);
- grub_crypto_cipher_close (secondary_cipher);
- return NULL;
- }
- }
- else
- {
- grub_crypto_cipher_close (cipher);
- grub_crypto_cipher_close (secondary_cipher);
- grub_error (GRUB_ERR_BAD_ARGUMENT, "Unknown IV mode: %s",
- cipheriv);
- return NULL;
- }
-
- /* Configure the hash used for the AF splitter and HMAC. */
- hash = grub_crypto_lookup_md_by_name (hashspec);
- if (!hash)
- {
- grub_crypto_cipher_close (cipher);
- grub_crypto_cipher_close (essiv_cipher);
- grub_crypto_cipher_close (secondary_cipher);
- grub_error (GRUB_ERR_FILE_NOT_FOUND, "Couldn't load %s hash",
- hashspec);
- return NULL;
- }
+ newdev = grub_cryptodisk_create (disk, uuid, ciphername, ciphermode, hashspec);
- newdev = grub_zalloc (sizeof (struct grub_cryptodisk));
- if (!newdev)
- {
- grub_crypto_cipher_close (cipher);
- grub_crypto_cipher_close (essiv_cipher);
- grub_crypto_cipher_close (secondary_cipher);
- return NULL;
- }
- newdev->cipher = cipher;
newdev->offset = grub_be_to_cpu32 (header.payloadOffset);
- newdev->source_disk = NULL;
- newdev->benbi_log = benbi_log;
- newdev->mode = mode;
- newdev->mode_iv = mode_iv;
- newdev->secondary_cipher = secondary_cipher;
- newdev->essiv_cipher = essiv_cipher;
- newdev->essiv_hash = essiv_hash;
- newdev->hash = hash;
- newdev->log_sector_size = 9;
- newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
- grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
newdev->modname = "luks";
- COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
return newdev;
}
@@ -329,7 +146,7 @@ luks_recover_key (grub_disk_t source,
struct grub_luks_phdr header;
grub_size_t keysize;
grub_uint8_t *split_key = NULL;
- char interactive_passphrase[MAX_PASSPHRASE] = "";
+ char interactive_passphrase[GRUB_CRYPTODISK_MAX_PASSPHRASE] = "";
grub_uint8_t *passphrase;
grub_size_t passphrase_length;
grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
@@ -376,7 +193,7 @@ luks_recover_key (grub_disk_t source,
/* Use bytestring from key file as passphrase */
passphrase = keyfile_bytes;
passphrase_length = keyfile_bytes_size;
- keyfile_bytes = NULL; /* use it only once */
+ keyfile_bytes = NULL; /* use it only once */
}
else
{
@@ -387,7 +204,7 @@ luks_recover_key (grub_disk_t source,
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))
+ if (!grub_password_get (interactive_passphrase, GRUB_CRYPTODISK_MAX_PASSPHRASE))
{
grub_free (split_key);
return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 67f6b0b59..bb25ab730 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -54,9 +54,14 @@ typedef enum
#define GRUB_CRYPTODISK_GF_LOG_BYTES (GRUB_CRYPTODISK_GF_LOG_SIZE - 3)
#define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
#define GRUB_CRYPTODISK_MAX_KEYLEN 128
+#define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
+#define GRUB_CRYPTODISK_PLAIN_CIPHER "aes-cbc-essiv:sha256"
+#define GRUB_CRYPTODISK_PLAIN_DIGEST "ripemd160"
+#define GRUB_CRYPTODISK_PLAIN_KEYSIZE 256
+
struct grub_cryptodisk;
typedef gcry_err_code_t
@@ -160,4 +165,7 @@ grub_util_get_geli_uuid (const char *dev);
grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t disk);
+grub_cryptodisk_t grub_cryptodisk_create (grub_disk_t disk, char *uuid,
+ char *ciphername, char *ciphermode, char *digest);
+
#endif
--
2.16.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 5/7] Cryptomount support for hyphens in UUID
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
` (2 preceding siblings ...)
2018-03-14 9:45 ` [PATCH 4/7] Cryptomount support plain dm-crypt John Lane
@ 2018-03-14 9:45 ` John Lane
2018-03-14 9:45 ` [PATCH 6/7] Retain constness of parameters John Lane
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: John Lane @ 2018-03-14 9:45 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
From: John Lane <john@lane.uk.net>
---
grub-core/disk/cryptodisk.c | 20 +++++++++++++++++---
grub-core/disk/luks.c | 26 ++++++++------------------
include/grub/cryptodisk.h | 2 ++
3 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 7f656f75c..c442d3a34 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -114,6 +114,20 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const grub_uint8_t *b)
}
}
+int
+grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b)
+{
+ while ((*uuid_a != '\0') && (*uuid_b != '\0'))
+ {
+ while (*uuid_a == '-') uuid_a++;
+ while (*uuid_b == '-') uuid_b++;
+ if (grub_toupper(*uuid_a) != grub_toupper(*uuid_b)) break;
+ uuid_a++;
+ uuid_b++;
+ }
+ return (*uuid_a == '\0') && (*uuid_b == '\0');
+}
+
static gcry_err_code_t
grub_crypto_pcbc_decrypt (grub_crypto_cipher_handle_t cipher,
void *out, void *in, grub_size_t size,
@@ -509,8 +523,8 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
{
for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
- if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
- break;
+ if (grub_cryptodisk_uuidcmp(name + sizeof ("cryptouuid/") - 1, dev->uuid))
+ break;
}
else
{
@@ -742,7 +756,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
{
grub_cryptodisk_t dev;
for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
- if (grub_strcasecmp (dev->uuid, uuid) == 0)
+ if (grub_cryptodisk_uuidcmp(dev->uuid, uuid))
return dev;
return NULL;
}
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 4ebe21b4e..80a760670 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -68,9 +68,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
int check_boot, grub_file_t hdr)
{
grub_cryptodisk_t newdev;
- const char *iptr;
struct grub_luks_phdr header;
- char *optr;
char uuid[sizeof (header.uuid) + 1];
char ciphername[sizeof (header.cipherName) + 1];
char ciphermode[sizeof (header.cipherMode) + 1];
@@ -104,22 +102,6 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
|| grub_be_to_cpu16 (header.version) != 1)
return NULL;
- optr = uuid;
- for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
- iptr++)
- {
- if (*iptr != '-')
- *optr++ = *iptr;
- }
- *optr = 0;
-
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
- {
- grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
- return NULL;
- }
-
-
/* Make sure that strings are null terminated. */
grub_memcpy (ciphername, header.cipherName, sizeof (header.cipherName));
ciphername[sizeof (header.cipherName)] = 0;
@@ -127,6 +109,14 @@ 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;
+
+ if ( check_uuid && ! grub_cryptodisk_uuidcmp(check_uuid, uuid))
+ {
+ grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
+ return NULL;
+ }
newdev = grub_cryptodisk_create (disk, uuid, ciphername, ciphermode, hashspec);
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index bb25ab730..01c02696e 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -168,4 +168,6 @@ grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t disk);
grub_cryptodisk_t grub_cryptodisk_create (grub_disk_t disk, char *uuid,
char *ciphername, char *ciphermode, char *digest);
+int
+grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b);
#endif
--
2.16.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 6/7] Retain constness of parameters.
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
` (3 preceding siblings ...)
2018-03-14 9:45 ` [PATCH 5/7] Cryptomount support for hyphens in UUID John Lane
@ 2018-03-14 9:45 ` John Lane
2018-03-14 9:45 ` [PATCH 7/7] Add support for using a whole device as a keyfile John Lane
` (2 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: John Lane @ 2018-03-14 9:45 UTC (permalink / raw)
To: grub-devel; +Cc: Denis Kasak
From: Denis Kasak <dkasak@termina.org.uk>
---
grub-core/disk/cryptodisk.c | 2 +-
include/grub/cryptodisk.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index c442d3a34..6fc2c23aa 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -115,7 +115,7 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const grub_uint8_t *b)
}
int
-grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b)
+grub_cryptodisk_uuidcmp(const char *uuid_a, const char *uuid_b)
{
while ((*uuid_a != '\0') && (*uuid_b != '\0'))
{
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 01c02696e..cd6a54582 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -169,5 +169,5 @@ grub_cryptodisk_t grub_cryptodisk_create (grub_disk_t disk, char *uuid,
char *ciphername, char *ciphermode, char *digest);
int
-grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b);
+grub_cryptodisk_uuidcmp(const char *uuid_a, const char *uuid_b);
#endif
--
2.16.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 7/7] Add support for using a whole device as a keyfile
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
` (4 preceding siblings ...)
2018-03-14 9:45 ` [PATCH 6/7] Retain constness of parameters John Lane
@ 2018-03-14 9:45 ` John Lane
2018-03-14 13:05 ` [PATCH 1/7] Cryptomount support LUKS detached header Daniel Kiper
2018-03-17 11:09 ` TJ
7 siblings, 0 replies; 20+ messages in thread
From: John Lane @ 2018-03-14 9:45 UTC (permalink / raw)
To: grub-devel; +Cc: Paul Gideon Dann
From: Paul Gideon Dann <pdgiddie@gmail.com>
---
grub-core/disk/cryptodisk.c | 86 +++++++++++++++++++++++++++++++++++----------
1 file changed, 68 insertions(+), 18 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 6fc2c23aa..a8937e5e3 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1032,26 +1032,76 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
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;
-
- keyfile = grub_file_open (state[4].arg);
- 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
+
+ if (grub_strchr (state[4].arg, '/'))
{
- 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);
+ keyfile_size = requested_keyfile_size ? requested_keyfile_size : \
+ GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
+ keyfile = grub_file_open (state[4].arg);
+ 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
- key = keyfile_buffer;
- }
+ {
+ 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;
+ }
+ }
+ else
+ {
+ grub_disk_t keydisk;
+ char* keydisk_name;
+ grub_err_t err;
+ grub_uint64_t total_sectors;
+
+ keydisk_name = grub_file_get_device_name(state[4].arg);
+ keydisk = grub_disk_open (keydisk_name);
+ if (!keydisk)
+ {
+ grub_printf (N_("Unable to open disk %s\n"), keydisk_name);
+ goto cleanup_keydisk_name;
+ }
+
+ total_sectors = grub_disk_get_size (keydisk);
+ if (total_sectors == GRUB_DISK_SIZE_UNKNOWN)
+ {
+ grub_printf (N_("Unable to determine size of disk %s\n"), keydisk_name);
+ goto cleanup_keydisk;
+ }
+
+ keyfile_size = (total_sectors << GRUB_DISK_SECTOR_BITS);
+ if (requested_keyfile_size > 0 && requested_keyfile_size < keyfile_size)
+ keyfile_size = requested_keyfile_size;
+ if (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);
+ goto cleanup_keydisk;
+ }
+
+ err = grub_disk_read (keydisk, 0, keyfile_offset, keyfile_size, keyfile_buffer);
+ if (err != GRUB_ERR_NONE)
+ {
+ grub_printf (N_("Failed to read from disk %s\n"), keydisk_name);
+ keyfile_size = 0;
+ goto cleanup_keydisk;
+ }
+
+ key = keyfile_buffer;
+
+ cleanup_keydisk:
+ grub_disk_close (keydisk);
+ cleanup_keydisk_name:
+ grub_free (keydisk_name);
+ }
}
}
--
2.16.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
` (5 preceding siblings ...)
2018-03-14 9:45 ` [PATCH 7/7] Add support for using a whole device as a keyfile John Lane
@ 2018-03-14 13:05 ` Daniel Kiper
2018-03-14 19:00 ` John Lane
2018-03-17 11:09 ` TJ
7 siblings, 1 reply; 20+ messages in thread
From: Daniel Kiper @ 2018-03-14 13:05 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: John Lane
On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
> From: John Lane <john@lane.uk.net>
I have just skimmed through the series. First of all, most if not
all patches beg for full blown commit messages. Just vague statements
in the subject are insufficient for me. And please add patch 0 which
introduces the series. git send-email --compose is your friend.
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-14 13:05 ` [PATCH 1/7] Cryptomount support LUKS detached header Daniel Kiper
@ 2018-03-14 19:00 ` John Lane
2018-03-21 7:23 ` Paul Menzel
2018-03-22 12:38 ` Daniel Kiper
0 siblings, 2 replies; 20+ messages in thread
From: John Lane @ 2018-03-14 19:00 UTC (permalink / raw)
To: Daniel Kiper, The development of GNU GRUB
On 14/03/18 13:05, Daniel Kiper wrote:
> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
>> From: John Lane <john@lane.uk.net>
>
> I have just skimmed through the series. First of all, most if not
> all patches beg for full blown commit messages. Just vague statements
> in the subject are insufficient for me. And please add patch 0 which
> introduces the series. git send-email --compose is your friend.
>
> Daniel
>
Sorry Daniel, this isn't something I do that often - submitting patches
to ML. Do you want me to resubmit them again, or is the below ok for you ?
These patches provide extensions to the "cryptomount" command. There are
my original five patches, plus an additional two patches from other
contributors who sent pull requests.
[PATCH 1/7] Cryptomount support LUKS detached header
Support LUKS detached headers so that the header can be separated from
the data payload, e.g. by storing on external removable media such as a
USB key.
[PATCH 2/7] Cryptomount support key files
Support key files so that passphrase entry can be suppressed. The
passphrase can be stored in a "key file" that can be stored, for
example, on external removable media such as a USB key.
[PATCH 3/7] cryptomount luks allow multiple passphrase attempts
Allow a second attempt to enter a passphrase. If unlocking fails on the
first attempt then the user is presented with the passphrase entry
prompt again. If a key file is given that does not unlock the device
then the user is given the opportunity to enter a passphrase.
This feature was added following feedback from Andrei Borzenkov back in
2015 when I first submitted these patches.
[PATCH 4/7] Cryptomount support plain dm-crypt
Support plain dm-crypt mode. Allow plain volumes to be opened. This is
largely a re-factoring of exisitng code to allow the crypto routines be
used independently of LUKS.
An important thing to recognise with this patch is that it largely
moves code from luks to cryptodisk so that it can be used outside of
luks (i.e. as plain dm-crypt). The crypto implementation was not changed
- most of the code in this patch already existed.
A little explanation of what the patch does:
I extracted the in-line code from "luks.c" that creates the crypto disk
into a new cryptomount function called "grub_cryptodisk_create" that is
then used by the luks module and is also avilable to the cryptomount
module.
I extracted the "set_passphrase" function from the "devmapper.c"
committed (e7f405ab) in the "peter/devmapper" branch so that I could use
it in cryptomount to hash a manually entered passphrase.
I then wrote some additional options and a small block of code to
implement plain mode using the above.
[PATCH 5/7] Cryptomount support for hyphens in UUID
Support for hyphens in UUID. The "-u" option of cryptomount accepts a
UUID. This option allows that to be delimited with hyphens so that the
same format can be given to Grub as is passed to the Linux kernel boot
options.
Contributor patches
[PATCH 6/7] Retain constness of parameters.
Don't drop constness on the parameters since they are being only read
anyway. Without this patch, compilation may fail if the compiler
complains that the constness of passed in parameters is being dropped.
[PATCH 7/7] Add support for using a whole device as a keyfile
This fixes the situation where a device is passed as a parameter to
--keyfile instead of the path to a file.
It is probably worth reviewing the email threads beginning with [1] and
[2], and especially my reply [3] in which I answer FAQs about this work.
discusses the self-generation (grub-mkconfig) aspects of this,
especially with respect to plain mode (there isn't any - but I had some
ideas in that mail about how it could possibly be achieved).
[1] http://lists.gnu.org/archive/html/grub-devel/2015-06/msg00109.html
[2] http://lists.gnu.org/archive/html/help-grub/2017-02/msg00017.html
[3] http://lists.gnu.org/archive/html/help-grub/2017-02/msg00029.html
end
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-14 19:00 ` John Lane
@ 2018-03-21 7:23 ` Paul Menzel
2018-03-22 12:38 ` Daniel Kiper
1 sibling, 0 replies; 20+ messages in thread
From: Paul Menzel @ 2018-03-21 7:23 UTC (permalink / raw)
To: John Lane, Daniel Kiper; +Cc: grub-devel
[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]
Dear John,
Am Mittwoch, den 14.03.2018, 19:00 +0000 schrieb John Lane:
> On 14/03/18 13:05, Daniel Kiper wrote:
> > On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
> > > From: John Lane <john@lane.uk.net>
> >
> > I have just skimmed through the series. First of all, most if not
> > all patches beg for full blown commit messages. Just vague statements
> > in the subject are insufficient for me. And please add patch 0 which
> > introduces the series. git send-email --compose is your friend.
>
> Sorry Daniel, this isn't something I do that often - submitting patches
> to ML. Do you want me to resubmit them again, or is the below ok for you ?
[…]
Thank you very much for submitting these patches for review. Judging
from the other comments, it looks like, you are going to send an
updated series anyway, so I guess, you can wait until then, though
expect another round then.
I do not know, how well you are versed with git, but it’d be great, if
you added your messages below as “real” commit messages. `git rebase -i
origin/master` should do that for you. Just replace *pick* by *r*, for
*r*eword, and add the commit messages there.
So, as testing the review changes will take some time, I suggest you
try to resend the patch series as soon as possible to get as much
reviews as possible, so you can test everything, when you get to it.
Thanks,
Paul
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-14 19:00 ` John Lane
2018-03-21 7:23 ` Paul Menzel
@ 2018-03-22 12:38 ` Daniel Kiper
2018-03-22 14:22 ` TJ
1 sibling, 1 reply; 20+ messages in thread
From: Daniel Kiper @ 2018-03-22 12:38 UTC (permalink / raw)
To: John Lane; +Cc: Daniel Kiper, The development of GNU GRUB
Hi John,
On Wed, Mar 14, 2018 at 07:00:11PM +0000, John Lane wrote:
> On 14/03/18 13:05, Daniel Kiper wrote:
> > On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
> >> From: John Lane <john@lane.uk.net>
> >
> > I have just skimmed through the series. First of all, most if not
> > all patches beg for full blown commit messages. Just vague statements
> > in the subject are insufficient for me. And please add patch 0 which
> > introduces the series. git send-email --compose is your friend.
> >
> > Daniel
> >
>
> Sorry Daniel, this isn't something I do that often - submitting patches
Not a problem.
> to ML. Do you want me to resubmit them again, or is the below ok for you ?
Please resubmit whole patch series and do not forget to take into
account comments posted by others.
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-22 12:38 ` Daniel Kiper
@ 2018-03-22 14:22 ` TJ
2018-03-26 13:10 ` John Lane
0 siblings, 1 reply; 20+ messages in thread
From: TJ @ 2018-03-22 14:22 UTC (permalink / raw)
To: grub-devel, John Lane
On 22/03/18 12:38, Daniel Kiper wrote:
> Hi John,
>
> On Wed, Mar 14, 2018 at 07:00:11PM +0000, John Lane wrote:
>> On 14/03/18 13:05, Daniel Kiper wrote:
>>> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
>>>> From: John Lane <john@lane.uk.net>
>>>
>>> I have just skimmed through the series. First of all, most if not
>>> all patches beg for full blown commit messages. Just vague statements
>>> in the subject are insufficient for me. And please add patch 0 which
>>> introduces the series. git send-email --compose is your friend.
>>>
>>> Daniel
>>>
>>
>> Sorry Daniel, this isn't something I do that often - submitting patches
>
> Not a problem.
>
>> to ML. Do you want me to resubmit them again, or is the below ok for you ?
>
> Please resubmit whole patch series and do not forget to take into
> account comments posted by others.
>
> Daniel
I've spent a couple of days doing a thorough review of this patch series.
Firstly I want to say a big thanks to John for bringing this along -
it's long been a large missing piece of the LUKS support.
My observations:
Break the series up. There are five distinct sets of functionality
change here:
a) LUKS detached headers
b) LUKS key files
c) allow multiple unlock attempts
d) plain dm-crypt
e) hyphens in UUIDs
(a) and (b) are in reasonable shape but there's some work to do; mostly
in preparing the way for the new functionality by cleaning up error exit
paths in luks.c::luks_recover_key() first - which I've done and will attach.
With that clean-up John's changes are easier to insert and verify.
(c) creates a lot of churn just due to indenting code that is being
wrapped in a while() loop. I've refactored that so the loop is in a
separate function which reduces the patch from 323 to 41 lines. It's in
my branch detailed below.
I'd suggest submitting (a) (b) and (c) as a series on their own. Get
them accepted and then introduce (e) and (d). I'd say (e) first since
it's relatively small.
(d) is a major new tranch of functionality dealing with core
cryptographic constructs so will need careful review by cryptographers
as well as GRUB developers and therefore could take some time. It'd be a
shame to have this hold up the excellent improvements to LUKS which
don't touch the cryptographic operations.
All in all an excellent contribution which I look forward to being
available.
My "cryptomount_luks_v5" branch for the LUKS changes can be got using:
git remote add iam.tj git://iam.tj/grub.git
git fetch iam.tj
and seen at:
http://iam.tj/gitweb/?p=grub.git;a=shortlog;h=refs/heads/cryptomount_luks_v5
---
commit 19896820737344fb820ab6487d16719e31cae763
Author: TJ <grub-devel@iam.tj>
Date: Wed Mar 21 14:07:22 2018 +0000
LUKS: refactor multiple return paths
Convert multiple return statements to goto with a single return so there
is only one place where memory needs to be free-d in error conditions.
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 86c50c6..a7c5b39 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -318,18 +318,23 @@ luks_recover_key (grub_disk_t source,
grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
unsigned i;
grub_size_t length;
- grub_err_t err;
+ grub_err_t err = GRUB_ERR_NONE;
+ char *err_msg = NULL;
grub_size_t max_stripes = 1;
char *tmp;
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
- return err;
+ goto fail;
grub_puts_ (N_("Attempting to decrypt master key..."));
keysize = grub_be_to_cpu32 (header.keyBytes);
if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN)
- return grub_error (GRUB_ERR_BAD_FS, "key is too long");
+ {
+ err = GRUB_ERR_BAD_FS;
+ err_msg = "key is too long";
+ goto fail;
+ }
for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED
@@ -338,7 +343,10 @@ luks_recover_key (grub_disk_t source,
split_key = grub_malloc (keysize * max_stripes);
if (!split_key)
- return grub_errno;
+ {
+ err = grub_errno;
+ goto fail;
+ }
/* Get the passphrase from the user. */
tmp = NULL;
@@ -350,8 +358,9 @@ luks_recover_key (grub_disk_t source,
grub_free (tmp);
if (!grub_password_get (passphrase, MAX_PASSPHRASE))
{
- grub_free (split_key);
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+ err = GRUB_ERR_BAD_ARGUMENT;
+ err_msg = "Passphrase not supplied";
+ goto fail;
}
/* Try to recover master key from each active keyslot. */
@@ -378,8 +387,8 @@ luks_recover_key (grub_disk_t source,
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
grub_dprintf ("luks", "PBKDF2 done\n");
@@ -387,8 +396,8 @@ luks_recover_key (grub_disk_t source,
gcry_err = grub_cryptodisk_setkey (dev, digest, keysize);
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
@@ -399,16 +408,13 @@ luks_recover_key (grub_disk_t source,
[i].keyMaterialOffset), 0,
length, split_key);
if (err)
- {
- grub_free (split_key);
- return err;
- }
+ goto fail;
gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
/* Merge the decrypted key material to get the candidate master
key. */
@@ -416,8 +422,8 @@ luks_recover_key (grub_disk_t source,
grub_be_to_cpu32 (header.keyblock[i].stripes));
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
grub_dprintf ("luks", "candidate key recovered\n");
@@ -433,8 +439,8 @@ luks_recover_key (grub_disk_t source,
sizeof (candidate_digest));
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
/* Compare the calculated PBKDF2 to the digest stored
@@ -454,17 +460,19 @@ luks_recover_key (grub_disk_t source,
gcry_err = grub_cryptodisk_setkey (dev, candidate_key, keysize);
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
- grub_free (split_key);
-
- return GRUB_ERR_NONE;
+ err = GRUB_ERR_NONE;
}
+fail:
grub_free (split_key);
- return GRUB_ACCESS_DENIED;
+ if(err && err_msg)
+ grub_error (err, errmsg);
+
+ return err;
}
struct grub_cryptodisk_dev luks_crypto = {
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-22 14:22 ` TJ
@ 2018-03-26 13:10 ` John Lane
2018-03-26 14:42 ` Daniel Kiper
0 siblings, 1 reply; 20+ messages in thread
From: John Lane @ 2018-03-26 13:10 UTC (permalink / raw)
To: TJ, grub-devel
On 22/03/18 14:22, TJ wrote:
> On 22/03/18 12:38, Daniel Kiper wrote:
>> Hi John,
>>
>> On Wed, Mar 14, 2018 at 07:00:11PM +0000, John Lane wrote:
>>> On 14/03/18 13:05, Daniel Kiper wrote:
>>>> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
>>>>> From: John Lane <john@lane.uk.net>
>>>>
>>>> I have just skimmed through the series. First of all, most if not
>>>> all patches beg for full blown commit messages. Just vague statements
>>>> in the subject are insufficient for me. And please add patch 0 which
>>>> introduces the series. git send-email --compose is your friend.
>>>>
>>>> Daniel
>>>>
>>>
>>> Sorry Daniel, this isn't something I do that often - submitting patches
>>
>> Not a problem.
>>
>>> to ML. Do you want me to resubmit them again, or is the below ok for you ?
>>
>> Please resubmit whole patch series and do not forget to take into
>> account comments posted by others.
>>
>> Daniel
>
> I've spent a couple of days doing a thorough review of this patch series.
>
> Firstly I want to say a big thanks to John for bringing this along -
> it's long been a large missing piece of the LUKS support.
Thanks for that. I will take a look at the below and try and work a
patch series as you suggest. But it might take a few days to find some
time to dedicate to it, due to other commitments I have right now.
I'll report back when I have something to show.
>
> My observations:
>
> Break the series up. There are five distinct sets of functionality
> change here:
> a) LUKS detached headers
> b) LUKS key files
> c) allow multiple unlock attempts
> d) plain dm-crypt
> e) hyphens in UUIDs
>
> (a) and (b) are in reasonable shape but there's some work to do; mostly
> in preparing the way for the new functionality by cleaning up error exit
> paths in luks.c::luks_recover_key() first - which I've done and will attach.
>
> With that clean-up John's changes are easier to insert and verify.
>
> (c) creates a lot of churn just due to indenting code that is being
> wrapped in a while() loop. I've refactored that so the loop is in a
> separate function which reduces the patch from 323 to 41 lines. It's in
> my branch detailed below.
>
> I'd suggest submitting (a) (b) and (c) as a series on their own. Get
> them accepted and then introduce (e) and (d). I'd say (e) first since
> it's relatively small.
>
> (d) is a major new tranch of functionality dealing with core
> cryptographic constructs so will need careful review by cryptographers
> as well as GRUB developers and therefore could take some time. It'd be a
> shame to have this hold up the excellent improvements to LUKS which
> don't touch the cryptographic operations.
>
> All in all an excellent contribution which I look forward to being
> available.
>
> My "cryptomount_luks_v5" branch for the LUKS changes can be got using:
>
> git remote add iam.tj git://iam.tj/grub.git
> git fetch iam.tj
>
> and seen at:
>
> http://iam.tj/gitweb/?p=grub.git;a=shortlog;h=refs/heads/cryptomount_luks_v5
>
>
> ---
> commit 19896820737344fb820ab6487d16719e31cae763
> Author: TJ <grub-devel@iam.tj>
> Date: Wed Mar 21 14:07:22 2018 +0000
>
> LUKS: refactor multiple return paths
>
> Convert multiple return statements to goto with a single return so there
> is only one place where memory needs to be free-d in error conditions.
>
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 86c50c6..a7c5b39 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -318,18 +318,23 @@ luks_recover_key (grub_disk_t source,
> grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
> unsigned i;
> grub_size_t length;
> - grub_err_t err;
> + grub_err_t err = GRUB_ERR_NONE;
> + char *err_msg = NULL;
> grub_size_t max_stripes = 1;
> char *tmp;
>
> err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> if (err)
> - return err;
> + goto fail;
>
> grub_puts_ (N_("Attempting to decrypt master key..."));
> keysize = grub_be_to_cpu32 (header.keyBytes);
> if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN)
> - return grub_error (GRUB_ERR_BAD_FS, "key is too long");
> + {
> + err = GRUB_ERR_BAD_FS;
> + err_msg = "key is too long";
> + goto fail;
> + }
>
> for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
> if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED
> @@ -338,7 +343,10 @@ luks_recover_key (grub_disk_t source,
>
> split_key = grub_malloc (keysize * max_stripes);
> if (!split_key)
> - return grub_errno;
> + {
> + err = grub_errno;
> + goto fail;
> + }
>
> /* Get the passphrase from the user. */
> tmp = NULL;
> @@ -350,8 +358,9 @@ luks_recover_key (grub_disk_t source,
> grub_free (tmp);
> if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> {
> - grub_free (split_key);
> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> + err = GRUB_ERR_BAD_ARGUMENT;
> + err_msg = "Passphrase not supplied";
> + goto fail;
> }
>
> /* Try to recover master key from each active keyslot. */
> @@ -378,8 +387,8 @@ luks_recover_key (grub_disk_t source,
>
> if (gcry_err)
> {
> - grub_free (split_key);
> - return grub_crypto_gcry_error (gcry_err);
> + err = grub_crypto_gcry_error (gcry_err);
> + goto fail;
> }
>
> grub_dprintf ("luks", "PBKDF2 done\n");
> @@ -387,8 +396,8 @@ luks_recover_key (grub_disk_t source,
> gcry_err = grub_cryptodisk_setkey (dev, digest, keysize);
> if (gcry_err)
> {
> - grub_free (split_key);
> - return grub_crypto_gcry_error (gcry_err);
> + err = grub_crypto_gcry_error (gcry_err);
> + goto fail;
> }
>
> length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
> @@ -399,16 +408,13 @@ luks_recover_key (grub_disk_t source,
> [i].keyMaterialOffset), 0,
> length, split_key);
> if (err)
> - {
> - grub_free (split_key);
> - return err;
> - }
> + goto fail;
>
> gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
> if (gcry_err)
> {
> - grub_free (split_key);
> - return grub_crypto_gcry_error (gcry_err);
> + err = grub_crypto_gcry_error (gcry_err);
> + goto fail;
> }
>
> /* Merge the decrypted key material to get the candidate master
> key. */
> @@ -416,8 +422,8 @@ luks_recover_key (grub_disk_t source,
> grub_be_to_cpu32 (header.keyblock[i].stripes));
> if (gcry_err)
> {
> - grub_free (split_key);
> - return grub_crypto_gcry_error (gcry_err);
> + err = grub_crypto_gcry_error (gcry_err);
> + goto fail;
> }
>
> grub_dprintf ("luks", "candidate key recovered\n");
> @@ -433,8 +439,8 @@ luks_recover_key (grub_disk_t source,
> sizeof (candidate_digest));
> if (gcry_err)
> {
> - grub_free (split_key);
> - return grub_crypto_gcry_error (gcry_err);
> + err = grub_crypto_gcry_error (gcry_err);
> + goto fail;
> }
>
> /* Compare the calculated PBKDF2 to the digest stored
> @@ -454,17 +460,19 @@ luks_recover_key (grub_disk_t source,
> gcry_err = grub_cryptodisk_setkey (dev, candidate_key, keysize);
> if (gcry_err)
> {
> - grub_free (split_key);
> - return grub_crypto_gcry_error (gcry_err);
> + err = grub_crypto_gcry_error (gcry_err);
> + goto fail;
> }
>
> - grub_free (split_key);
> -
> - return GRUB_ERR_NONE;
> + err = GRUB_ERR_NONE;
> }
>
> +fail:
> grub_free (split_key);
> - return GRUB_ACCESS_DENIED;
> + if(err && err_msg)
> + grub_error (err, errmsg);
> +
> + return err;
> }
>
> struct grub_cryptodisk_dev luks_crypto = {
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-26 13:10 ` John Lane
@ 2018-03-26 14:42 ` Daniel Kiper
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Kiper @ 2018-03-26 14:42 UTC (permalink / raw)
To: grub, grub-devel; +Cc: TJ
On Mon, Mar 26, 2018 at 02:10:48PM +0100, John Lane wrote:
> On 22/03/18 14:22, TJ wrote:
> > On 22/03/18 12:38, Daniel Kiper wrote:
> >> Hi John,
> >>
> >> On Wed, Mar 14, 2018 at 07:00:11PM +0000, John Lane wrote:
> >>> On 14/03/18 13:05, Daniel Kiper wrote:
> >>>> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
> >>>>> From: John Lane <john@lane.uk.net>
> >>>>
> >>>> I have just skimmed through the series. First of all, most if not
> >>>> all patches beg for full blown commit messages. Just vague statements
> >>>> in the subject are insufficient for me. And please add patch 0 which
> >>>> introduces the series. git send-email --compose is your friend.
> >>>>
> >>>> Daniel
> >>>>
> >>>
> >>> Sorry Daniel, this isn't something I do that often - submitting patches
> >>
> >> Not a problem.
> >>
> >>> to ML. Do you want me to resubmit them again, or is the below ok for you ?
> >>
> >> Please resubmit whole patch series and do not forget to take into
> >> account comments posted by others.
> >>
> >> Daniel
> >
> > I've spent a couple of days doing a thorough review of this patch series.
> >
> > Firstly I want to say a big thanks to John for bringing this along -
> > it's long been a large missing piece of the LUKS support.
>
> Thanks for that. I will take a look at the below and try and work a
> patch series as you suggest. But it might take a few days to find some
> time to dedicate to it, due to other commitments I have right now.
>
> I'll report back when I have something to show.
Not a problem, we how it works. We are patiently looking forward for the patches.
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-14 9:44 [PATCH 1/7] Cryptomount support LUKS detached header John Lane
` (6 preceding siblings ...)
2018-03-14 13:05 ` [PATCH 1/7] Cryptomount support LUKS detached header Daniel Kiper
@ 2018-03-17 11:09 ` TJ
2018-03-18 20:29 ` John Lane
7 siblings, 1 reply; 20+ messages in thread
From: TJ @ 2018-03-17 11:09 UTC (permalink / raw)
To: grub-devel
On 14/03/18 09:44, John Lane wrote:
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -880,7 +882,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
>
> FOR_CRYPTODISK_DEVS (cr)
> {
> - dev = cr->scan (source, search_uuid, check_boot);
> + dev = cr->scan (source, search_uuid, check_boot,0);
Minor nit; "boot,0" > "boot, 0"
> @@ -934,6 +936,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> if (argc < 1 && !state[1].set && !state[2].set)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>
> + if (state[3].set) /* LUKS detached header */
> + {
> + if (state[0].set) /* Cannot use UUID lookup with detached header */
Should there be specific warning for the user here, at least for #ifdef
GRUB_UTIL
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -391,13 +415,18 @@ luks_recover_key (grub_disk_t source,
> return grub_crypto_gcry_error (gcry_err);
> }
>
> + sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
> length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
>
> /* Read and decrypt the key material from the disk. */
> - err = grub_disk_read (source,
> - grub_be_to_cpu32 (header.keyblock
> - [i].keyMaterialOffset), 0,
> - length, split_key);
> + if (hdr)
> + {
> + grub_file_seek (hdr, sector * 512);
Shouldn't 512 be GRUB_DISK_SECTOR_SIZE ?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/7] Cryptomount support LUKS detached header
2018-03-17 11:09 ` TJ
@ 2018-03-18 20:29 ` John Lane
0 siblings, 0 replies; 20+ messages in thread
From: John Lane @ 2018-03-18 20:29 UTC (permalink / raw)
To: grub-devel, TJ
On 17/03/18 11:09, TJ wrote:
> On 14/03/18 09:44, John Lane wrote:
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -880,7 +882,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
>>
>> FOR_CRYPTODISK_DEVS (cr)
>> {
>> - dev = cr->scan (source, search_uuid, check_boot);
>> + dev = cr->scan (source, search_uuid, check_boot,0);
>
> Minor nit; "boot,0" > "boot, 0"
>
>> @@ -934,6 +936,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>> if (argc < 1 && !state[1].set && !state[2].set)
>> return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>>
>> + if (state[3].set) /* LUKS detached header */
>> + {
>> + if (state[0].set) /* Cannot use UUID lookup with detached header */
>
> Should there be specific warning for the user here, at least for #ifdef
> GRUB_UTIL
>
>> --- a/grub-core/disk/luks.c
>> +++ b/grub-core/disk/luks.c
>> @@ -391,13 +415,18 @@ luks_recover_key (grub_disk_t source,
>> return grub_crypto_gcry_error (gcry_err);
>> }
>>
>> + sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
>> length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
>>
>> /* Read and decrypt the key material from the disk. */
>> - err = grub_disk_read (source,
>> - grub_be_to_cpu32 (header.keyblock
>> - [i].keyMaterialOffset), 0,
>> - length, split_key);
>> + if (hdr)
>> + {
>> + grub_file_seek (hdr, sector * 512);
>
> Shouldn't 512 be GRUB_DISK_SECTOR_SIZE ?
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
I have applied these changes. Builds clean but I need to test it. I
might have time to do that tomorrow, otherwise next weekend will be soonest.
^ permalink raw reply [flat|nested] 20+ messages in thread