* [PATCH 1/4] Cryptomount support LUKS detached header
2015-06-16 9:11 Cryptomount enhancements: detached headers, key-files and plain mode John Lane
@ 2015-06-16 9:11 ` John Lane
2015-06-16 9:11 ` [PATCH 2/4] Cryptomount support key files John Lane
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: John Lane @ 2015-06-16 9:11 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
---
grub-core/disk/cryptodisk.c | 23 +++++++++++++++++++----
grub-core/disk/geli.c | 7 +++++--
grub-core/disk/luks.c | 45 +++++++++++++++++++++++++++++++++++++--------
include/grub/cryptodisk.h | 5 +++--
4 files changed, 64 insertions(+), 16 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 82a3dcb..65b3a01 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -40,6 +40,7 @@ static const struct grub_arg_option options[] =
/* TRANSLATORS: It's still restricted to cryptodisks only. */
{"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},
{0, 0, 0, 0, 0, 0}
};
@@ -803,6 +804,7 @@ 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 void
cryptodisk_close (grub_cryptodisk_t dev)
@@ -827,13 +829,13 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
FOR_CRYPTODISK_DEVS (cr)
{
- dev = cr->scan (source, search_uuid, check_boot);
+ dev = cr->scan (source, search_uuid, check_boot, hdr);
if (grub_errno)
return grub_errno;
if (!dev)
continue;
- err = cr->recover_key (source, dev);
+ err = cr->recover_key (source, dev, hdr);
if (err)
{
cryptodisk_close (dev);
@@ -874,7 +876,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);
if (grub_errno)
return grub_errno;
if (!dev)
@@ -928,6 +930,19 @@ 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 */
+ return GRUB_ERR_BAD_ARGUMENT;
+
+ hdr = grub_file_open (state[3].arg);
+ if (!hdr)
+ return grub_errno;
+ grub_printf ("\nUsing detached header %s\n", state[3].arg);
+ }
+ else
+ hdr = NULL;
+
have_it = 0;
if (state[0].set)
{
@@ -1125,7 +1140,7 @@ 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"),
+ N_("SOURCE|-u UUID|-a|-b|-H file"),
N_("Mount a crypto device."), options);
grub_procfs_register ("luks_script", &luks_script);
}
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index e9d2329..f4394eb 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -52,6 +52,7 @@
#include <grub/dl.h>
#include <grub/err.h>
#include <grub/disk.h>
+#include <grub/file.h>
#include <grub/crypto.h>
#include <grub/partition.h>
#include <grub/i18n.h>
@@ -243,7 +244,8 @@ grub_util_get_geli_uuid (const char *dev)
static grub_cryptodisk_t
configure_ciphers (grub_disk_t disk, const char *check_uuid,
- int boot_only)
+ int boot_only,
+ grub_file_t hdr __attribute__ ((unused)) )
{
grub_cryptodisk_t newdev;
struct grub_geli_phdr header;
@@ -398,7 +400,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
}
static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev)
+recover_key (grub_disk_t source, grub_cryptodisk_t dev,
+ grub_file_t hdr __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 86c50c6..66e64c0 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -23,6 +23,7 @@
#include <grub/dl.h>
#include <grub/err.h>
#include <grub/disk.h>
+#include <grub/file.h>
#include <grub/crypto.h>
#include <grub/partition.h>
#include <grub/i18n.h>
@@ -66,7 +67,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
static grub_cryptodisk_t
configure_ciphers (grub_disk_t disk, const char *check_uuid,
- int check_boot)
+ int check_boot, grub_file_t hdr)
{
grub_cryptodisk_t newdev;
const char *iptr;
@@ -86,11 +87,21 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
int benbi_log = 0;
grub_err_t err;
+ err = GRUB_ERR_NONE;
+
if (check_boot)
return NULL;
/* Read the LUKS header. */
- err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
+ if (hdr)
+ {
+ grub_file_seek (hdr, 0);
+ if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
+ err = GRUB_ERR_READ_ERROR;
+ }
+ else
+ err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
+
if (err)
{
if (err == GRUB_ERR_OUT_OF_RANGE)
@@ -304,12 +315,14 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
newdev->modname = "luks";
COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
+
return newdev;
}
static grub_err_t
luks_recover_key (grub_disk_t source,
- grub_cryptodisk_t dev)
+ grub_cryptodisk_t dev,
+ grub_file_t hdr)
{
struct grub_luks_phdr header;
grub_size_t keysize;
@@ -321,8 +334,19 @@ luks_recover_key (grub_disk_t source,
grub_err_t err;
grub_size_t max_stripes = 1;
char *tmp;
+ grub_uint32_t sector;
+
+ err = GRUB_ERR_NONE;
+
+ if (hdr)
+ {
+ grub_file_seek (hdr, 0);
+ if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
+ err = GRUB_ERR_READ_ERROR;
+ }
+ else
+ err = grub_disk_read (source, 0, 0, sizeof (header), &header);
- err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
return err;
@@ -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);
+ 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);
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index f2ad2a7..16dee3c 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -20,6 +20,7 @@
#define GRUB_CRYPTODISK_HEADER 1
#include <grub/disk.h>
+#include <grub/file.h>
#include <grub/crypto.h>
#include <grub/list.h>
#ifdef GRUB_UTIL
@@ -106,8 +107,8 @@ struct grub_cryptodisk_dev
struct grub_cryptodisk_dev **prev;
grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
- int boot_only);
- grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev);
+ int boot_only, grub_file_t hdr);
+ grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr);
};
typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
--
2.1.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/4] Cryptomount support key files
2015-06-16 9:11 Cryptomount enhancements: detached headers, key-files and plain mode John Lane
2015-06-16 9:11 ` [PATCH 1/4] Cryptomount support LUKS detached header John Lane
@ 2015-06-16 9:11 ` John Lane
2015-06-20 4:54 ` Andrei Borzenkov
2015-06-16 9:11 ` [PATCH 3/4] Cryptomount support plain dm-crypt John Lane
2015-06-16 9:11 ` [PATCH 4/4] Cryptomount support for hyphens in UUID John Lane
3 siblings, 1 reply; 13+ messages in thread
From: John Lane @ 2015-06-16 9:11 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
---
grub-core/disk/cryptodisk.c | 34 +++++++++++++++++++++++++++++++--
grub-core/disk/geli.c | 4 +++-
grub-core/disk/luks.c | 46 +++++++++++++++++++++++++++++++--------------
include/grub/cryptodisk.h | 5 ++++-
4 files changed, 71 insertions(+), 18 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 65b3a01..fbe7db9 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,10 @@ static const struct grub_arg_option options[] =
{"all", 'a', 0, N_("Mount all."), 0, 0},
{"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
{"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STRING},
+ {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
+ {"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
+ {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
+ {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, ARG_TYPE_INT},
{0, 0, 0, 0, 0, 0}
};
@@ -805,6 +809,8 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
static int check_boot, have_it;
static char *search_uuid;
static grub_file_t hdr;
+static grub_uint8_t *key, keyfile_buffer[GRUB_CRYPTODISK_MAX_KEYFILE_SIZE];
+static grub_size_t keyfile_size;
static void
cryptodisk_close (grub_cryptodisk_t dev)
@@ -835,7 +841,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
if (!dev)
continue;
- err = cr->recover_key (source, dev, hdr);
+ err = cr->recover_key (source, dev, hdr, key, keyfile_size);
if (err)
{
cryptodisk_close (dev);
@@ -938,12 +944,36 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
hdr = grub_file_open (state[3].arg);
if (!hdr)
return grub_errno;
- grub_printf ("\nUsing detached header %s\n", state[3].arg);
}
else
hdr = NULL;
have_it = 0;
+
+ if (state[4].set) /* Key file */
+ {
+ grub_file_t keyfile;
+ int keyfile_offset;
+
+ key = keyfile_buffer;
+ keyfile_offset = state[6].set ? grub_strtoul (state[6].arg, 0, 0) : 0;
+ keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0, 0) : \
+ GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
+
+ keyfile = grub_file_open (state[4].arg);
+ if (!keyfile)
+ return grub_errno;
+
+ if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
+ return grub_errno;
+
+ keyfile_size = grub_file_read (keyfile, key, keyfile_size);
+ if (keyfile_size == (grub_size_t)-1)
+ return grub_errno;
+ }
+ else
+ key = NULL;
+
if (state[0].set)
{
grub_cryptodisk_t dev;
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index f4394eb..da6aa6a 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -401,7 +401,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
static grub_err_t
recover_key (grub_disk_t source, grub_cryptodisk_t dev,
- grub_file_t hdr __attribute__ ((unused)) )
+ grub_file_t hdr __attribute__ ((unused)),
+ grub_uint8_t *key __attribute__ ((unused)),
+ grub_size_t keyfile_size __attribute__ ((unused)) )
{
grub_size_t keysize;
grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 66e64c0..0d0db9d 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -136,6 +136,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
ciphermode[sizeof (header.cipherMode)] = 0;
grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
hashspec[sizeof (header.hashSpec)] = 0;
+ grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
+ uuid[sizeof (header.uuid)] = 0;
ciph = grub_crypto_lookup_cipher_by_name (ciphername);
if (!ciph)
@@ -322,12 +324,16 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
static grub_err_t
luks_recover_key (grub_disk_t source,
grub_cryptodisk_t dev,
- grub_file_t hdr)
+ grub_file_t hdr,
+ grub_uint8_t *keyfile_bytes,
+ grub_size_t keyfile_bytes_size)
{
struct grub_luks_phdr header;
grub_size_t keysize;
grub_uint8_t *split_key = NULL;
- char passphrase[MAX_PASSPHRASE] = "";
+ char interactive_passphrase[MAX_PASSPHRASE] = "";
+ grub_uint8_t *passphrase;
+ grub_size_t passphrase_length;
grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
unsigned i;
grub_size_t length;
@@ -364,18 +370,30 @@ luks_recover_key (grub_disk_t source,
if (!split_key)
return grub_errno;
- /* Get the passphrase from the user. */
- tmp = NULL;
- if (source->partition)
- tmp = grub_partition_get_name (source->partition);
- grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
- source->partition ? "," : "", tmp ? : "",
- dev->uuid);
- grub_free (tmp);
- if (!grub_password_get (passphrase, MAX_PASSPHRASE))
+ if (keyfile_bytes)
{
- grub_free (split_key);
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+ /* Use bytestring from key file as passphrase */
+ passphrase = keyfile_bytes;
+ passphrase_length = keyfile_bytes_size;
+ }
+ else
+ {
+ /* Get the passphrase from the user. */
+ tmp = NULL;
+ if (source->partition)
+ tmp = grub_partition_get_name (source->partition);
+ grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+ source->partition ? "," : "", tmp ? : "", dev->uuid);
+ grub_free (tmp);
+ if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
+ {
+ grub_free (split_key);
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+ }
+
+ passphrase = (grub_uint8_t *)interactive_passphrase;
+ passphrase_length = grub_strlen (interactive_passphrase);
+
}
/* Try to recover master key from each active keyslot. */
@@ -393,7 +411,7 @@ luks_recover_key (grub_disk_t source,
/* Calculate the PBKDF2 of the user supplied passphrase. */
gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
- grub_strlen (passphrase),
+ passphrase_length,
header.keyblock[i].passwordSalt,
sizeof (header.keyblock[i].passwordSalt),
grub_be_to_cpu32 (header.keyblock[i].
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 16dee3c..0299625 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -55,6 +55,8 @@ typedef enum
#define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
#define GRUB_CRYPTODISK_MAX_KEYLEN 128
+#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
+
struct grub_cryptodisk;
typedef gcry_err_code_t
@@ -108,7 +110,8 @@ struct grub_cryptodisk_dev
grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
int boot_only, grub_file_t hdr);
- grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr);
+ grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,
+ grub_file_t hdr, grub_uint8_t *key, grub_size_t keyfile_size);
};
typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
--
2.1.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] Cryptomount support key files
2015-06-16 9:11 ` [PATCH 2/4] Cryptomount support key files John Lane
@ 2015-06-20 4:54 ` Andrei Borzenkov
2015-06-23 17:27 ` John Lane
0 siblings, 1 reply; 13+ messages in thread
From: Andrei Borzenkov @ 2015-06-20 4:54 UTC (permalink / raw)
To: John Lane; +Cc: grub-devel
В Tue, 16 Jun 2015 10:11:13 +0100
John Lane <john@lane.uk.net> пишет:
> ---
> grub-core/disk/cryptodisk.c | 34 +++++++++++++++++++++++++++++++--
> grub-core/disk/geli.c | 4 +++-
> grub-core/disk/luks.c | 46 +++++++++++++++++++++++++++++++--------------
> include/grub/cryptodisk.h | 5 ++++-
> 4 files changed, 71 insertions(+), 18 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 65b3a01..fbe7db9 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,10 @@ static const struct grub_arg_option options[] =
> {"all", 'a', 0, N_("Mount all."), 0, 0},
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> {"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STRING},
> + {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
> + {"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
It is unused
> + {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
> + {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, ARG_TYPE_INT},
> {0, 0, 0, 0, 0, 0}
> };
>
> @@ -805,6 +809,8 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> static int check_boot, have_it;
> static char *search_uuid;
> static grub_file_t hdr;
> +static grub_uint8_t *key, keyfile_buffer[GRUB_CRYPTODISK_MAX_KEYFILE_SIZE];
> +static grub_size_t keyfile_size;
>
Someone should really get rid of static variables and pass them as
callback data.
> static void
> cryptodisk_close (grub_cryptodisk_t dev)
> @@ -835,7 +841,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> if (!dev)
> continue;
>
> - err = cr->recover_key (source, dev, hdr);
> + err = cr->recover_key (source, dev, hdr, key, keyfile_size);
You never clear key variable, so after the first call all subsequent
invocations will always use it for any device. Moving to proper
callback data would make such errors less likely.
> if (err)
> {
> cryptodisk_close (dev);
> @@ -938,12 +944,36 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> hdr = grub_file_open (state[3].arg);
> if (!hdr)
> return grub_errno;
> - grub_printf ("\nUsing detached header %s\n", state[3].arg);
You remove line just added in previous patch? Why previous patch added
it then?
> }
> else
> hdr = NULL;
>
> have_it = 0;
> +
> + if (state[4].set) /* Key file */
> + {
> + grub_file_t keyfile;
> + int keyfile_offset;
> +
> + key = keyfile_buffer;
> + keyfile_offset = state[6].set ? grub_strtoul (state[6].arg, 0, 0) : 0;
> + keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0, 0) : \
> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
> +
> + keyfile = grub_file_open (state[4].arg);
> + if (!keyfile)
> + return grub_errno;
> +
> + if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
> + return grub_errno;
> +
> + keyfile_size = grub_file_read (keyfile, key, keyfile_size);
> + if (keyfile_size == (grub_size_t)-1)
> + return grub_errno;
If keyfile size is explicitly given, I expect that short read should be
considered an error. Otherwise what is the point of explicitly giving
size?
> + }
> + else
> + key = NULL;
> +
> if (state[0].set)
> {
> grub_cryptodisk_t dev;
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index f4394eb..da6aa6a 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -401,7 +401,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>
> static grub_err_t
> recover_key (grub_disk_t source, grub_cryptodisk_t dev,
> - grub_file_t hdr __attribute__ ((unused)) )
> + grub_file_t hdr __attribute__ ((unused)),
> + grub_uint8_t *key __attribute__ ((unused)),
> + grub_size_t keyfile_size __attribute__ ((unused)) )
> {
> grub_size_t keysize;
> grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 66e64c0..0d0db9d 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -136,6 +136,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> ciphermode[sizeof (header.cipherMode)] = 0;
> grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
> hashspec[sizeof (header.hashSpec)] = 0;
> + grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
> + uuid[sizeof (header.uuid)] = 0;
>
How exactly this is related to keyfile support?
> ciph = grub_crypto_lookup_cipher_by_name (ciphername);
> if (!ciph)
> @@ -322,12 +324,16 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> static grub_err_t
> luks_recover_key (grub_disk_t source,
> grub_cryptodisk_t dev,
> - grub_file_t hdr)
> + grub_file_t hdr,
> + grub_uint8_t *keyfile_bytes,
> + grub_size_t keyfile_bytes_size)
> {
> struct grub_luks_phdr header;
> grub_size_t keysize;
> grub_uint8_t *split_key = NULL;
> - char passphrase[MAX_PASSPHRASE] = "";
> + char interactive_passphrase[MAX_PASSPHRASE] = "";
> + grub_uint8_t *passphrase;
> + grub_size_t passphrase_length;
> grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
> unsigned i;
> grub_size_t length;
> @@ -364,18 +370,30 @@ luks_recover_key (grub_disk_t source,
> if (!split_key)
> return grub_errno;
>
> - /* Get the passphrase from the user. */
> - tmp = NULL;
> - if (source->partition)
> - tmp = grub_partition_get_name (source->partition);
> - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> - source->partition ? "," : "", tmp ? : "",
> - dev->uuid);
> - grub_free (tmp);
> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> + if (keyfile_bytes)
> {
> - grub_free (split_key);
> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> + /* Use bytestring from key file as passphrase */
> + passphrase = keyfile_bytes;
> + passphrase_length = keyfile_bytes_size;
> + }
> + else
I'm not sure if this should be "else". I think normal behavior of user
space is to ask for password then. If keyfile fails we cannot continue
anyway.
> + {
> + /* Get the passphrase from the user. */
> + tmp = NULL;
> + if (source->partition)
> + tmp = grub_partition_get_name (source->partition);
> + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> + source->partition ? "," : "", tmp ? : "", dev->uuid);
> + grub_free (tmp);
> + if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
> + {
> + grub_free (split_key);
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> + }
> +
> + passphrase = (grub_uint8_t *)interactive_passphrase;
> + passphrase_length = grub_strlen (interactive_passphrase);
> +
> }
>
> /* Try to recover master key from each active keyslot. */
> @@ -393,7 +411,7 @@ luks_recover_key (grub_disk_t source,
>
> /* Calculate the PBKDF2 of the user supplied passphrase. */
> gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
> - grub_strlen (passphrase),
> + passphrase_length,
> header.keyblock[i].passwordSalt,
> sizeof (header.keyblock[i].passwordSalt),
> grub_be_to_cpu32 (header.keyblock[i].
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 16dee3c..0299625 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -55,6 +55,8 @@ typedef enum
> #define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
> #define GRUB_CRYPTODISK_MAX_KEYLEN 128
>
> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
> +
Why it is different from MAX_KEYLEN?
> struct grub_cryptodisk;
>
> typedef gcry_err_code_t
> @@ -108,7 +110,8 @@ struct grub_cryptodisk_dev
>
> grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
> int boot_only, grub_file_t hdr);
> - grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr);
> + grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,
> + grub_file_t hdr, grub_uint8_t *key, grub_size_t keyfile_size);
> };
> typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] Cryptomount support key files
2015-06-20 4:54 ` Andrei Borzenkov
@ 2015-06-23 17:27 ` John Lane
2015-06-24 6:59 ` Andrei Borzenkov
0 siblings, 1 reply; 13+ messages in thread
From: John Lane @ 2015-06-23 17:27 UTC (permalink / raw)
To: Andrei Borzenkov; +Cc: grub-devel
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Comments inline. I'll resubmit the patch set with changes as per comments.
On 20/06/15 05:54, Andrei Borzenkov wrote:
> В Tue, 16 Jun 2015 10:11:13 +0100
> John Lane <john@lane.uk.net> пишет:
>
>> ---
>> grub-core/disk/cryptodisk.c | 34 +++++++++++++++++++++++++++++++--
>> grub-core/disk/geli.c | 4 +++-
>> grub-core/disk/luks.c | 46
+++++++++++++++++++++++++++++++--------------
>> include/grub/cryptodisk.h | 5 ++++-
>> 4 files changed, 71 insertions(+), 18 deletions(-)
>>
>> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
>> index 65b3a01..fbe7db9 100644
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -41,6 +41,10 @@ static const struct grub_arg_option options[] =
>> {"all", 'a', 0, N_("Mount all."), 0, 0},
>> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."),
0, 0},
>> {"header", 'H', 0, N_("Read LUKS header from file"), 0,
ARG_TYPE_STRING},
>> + {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
>> + {"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
> It is unused
That's a mistake from when I split the patch in two. The key size
argument is only used by plain mode.
(in LUKS mode it's obtained from the header). I'll re-do the patches
with that in the plain-mode one.
>
>> + {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0,
ARG_TYPE_INT},
>> + {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0,
ARG_TYPE_INT},
>> {0, 0, 0, 0, 0, 0}
>> };
>>
>> @@ -805,6 +809,8 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>> static int check_boot, have_it;
>> static char *search_uuid;
>> static grub_file_t hdr;
>> +static grub_uint8_t *key,
keyfile_buffer[GRUB_CRYPTODISK_MAX_KEYFILE_SIZE];
>> +static grub_size_t keyfile_size;
>>
> Someone should really get rid of static variables and pass them as
> callback data.
I just followed the conventions used by the existing code. I am not
familiar enough with the code-base
to change how it does things.
>
>> static void
>> cryptodisk_close (grub_cryptodisk_t dev)
>> @@ -835,7 +841,7 @@ grub_cryptodisk_scan_device_real (const char
*name, grub_disk_t source)
>> if (!dev)
>> continue;
>>
>> - err = cr->recover_key (source, dev, hdr);
>> + err = cr->recover_key (source, dev, hdr, key, keyfile_size);
> You never clear key variable, so after the first call all subsequent
> invocations will always use it for any device. Moving to proper
> callback data would make such errors less likely.
It is cleared when the arguments are processed, specifically
"grub_cmd_cryptomount" sets "key = NULL".
I have tested multiple uses and it does work as expected.
>
>> if (err)
>> {
>> cryptodisk_close (dev);
>> @@ -938,12 +944,36 @@ grub_cmd_cryptomount (grub_extcmd_context_t
ctxt, int argc, char **args)
>> hdr = grub_file_open (state[3].arg);
>> if (!hdr)
>> return grub_errno;
>> - grub_printf ("\nUsing detached header %s\n", state[3].arg);
> You remove line just added in previous patch? Why previous patch added
> it then?
I thought I'd removed that. Will re-do.
>
>> }
>> else
>> hdr = NULL;
>>
>> have_it = 0;
>> +
>> + if (state[4].set) /* Key file */
>> + {
>> + grub_file_t keyfile;
>> + int keyfile_offset;
>> +
>> + key = keyfile_buffer;
>> + keyfile_offset = state[6].set ? grub_strtoul (state[6].arg, 0,
0) : 0;
>> + keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0,
0) : \
>> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
>> +
>> + keyfile = grub_file_open (state[4].arg);
>> + if (!keyfile)
>> + return grub_errno;
>> +
>> + if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
>> + return grub_errno;
>> +
>> + keyfile_size = grub_file_read (keyfile, key, keyfile_size);
>> + if (keyfile_size == (grub_size_t)-1)
>> + return grub_errno;
> If keyfile size is explicitly given, I expect that short read should be
> considered an error. Otherwise what is the point of explicitly giving
> size?
A short read is accepted by the upstream "cryptsetup" tool. Its man page
describes its "--keyfile-size" argument as "Read a maximum of value
bytes from the key file. Default is to read the whole file up to the
compiled-in maximum. The cryptomount command follows that rule.
>
>> + }
>> + else
>> + key = NULL;
>> +
>> if (state[0].set)
>> {
>> grub_cryptodisk_t dev;
>> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
>> index f4394eb..da6aa6a 100644
>> --- a/grub-core/disk/geli.c
>> +++ b/grub-core/disk/geli.c
>> @@ -401,7 +401,9 @@ configure_ciphers (grub_disk_t disk, const char
*check_uuid,
>>
>> static grub_err_t
>> recover_key (grub_disk_t source, grub_cryptodisk_t dev,
>> - grub_file_t hdr __attribute__ ((unused)) )
>> + grub_file_t hdr __attribute__ ((unused)),
>> + grub_uint8_t *key __attribute__ ((unused)),
>> + grub_size_t keyfile_size __attribute__ ((unused)) )
>> {
>> grub_size_t keysize;
>> grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
>> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
>> index 66e64c0..0d0db9d 100644
>> --- a/grub-core/disk/luks.c
>> +++ b/grub-core/disk/luks.c
>> @@ -136,6 +136,8 @@ configure_ciphers (grub_disk_t disk, const char
*check_uuid,
>> ciphermode[sizeof (header.cipherMode)] = 0;
>> grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
>> hashspec[sizeof (header.hashSpec)] = 0;
>> + grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
>> + uuid[sizeof (header.uuid)] = 0;
>>
> How exactly this is related to keyfile support?
it isn't. it belongs in one of the other patches. will fix.
>
>
>> ciph = grub_crypto_lookup_cipher_by_name (ciphername);
>> if (!ciph)
>> @@ -322,12 +324,16 @@ configure_ciphers (grub_disk_t disk, const char
*check_uuid,
>> static grub_err_t
>> luks_recover_key (grub_disk_t source,
>> grub_cryptodisk_t dev,
>> - grub_file_t hdr)
>> + grub_file_t hdr,
>> + grub_uint8_t *keyfile_bytes,
>> + grub_size_t keyfile_bytes_size)
>> {
>> struct grub_luks_phdr header;
>> grub_size_t keysize;
>> grub_uint8_t *split_key = NULL;
>> - char passphrase[MAX_PASSPHRASE] = "";
>> + char interactive_passphrase[MAX_PASSPHRASE] = "";
>> + grub_uint8_t *passphrase;
>> + grub_size_t passphrase_length;
>> grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
>> unsigned i;
>> grub_size_t length;
>> @@ -364,18 +370,30 @@ luks_recover_key (grub_disk_t source,
>> if (!split_key)
>> return grub_errno;
>>
>> - /* Get the passphrase from the user. */
>> - tmp = NULL;
>> - if (source->partition)
>> - tmp = grub_partition_get_name (source->partition);
>> - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
>> - source->partition ? "," : "", tmp ? : "",
>> - dev->uuid);
>> - grub_free (tmp);
>> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
>> + if (keyfile_bytes)
>> {
>> - grub_free (split_key);
>> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
supplied");
>> + /* Use bytestring from key file as passphrase */
>> + passphrase = keyfile_bytes;
>> + passphrase_length = keyfile_bytes_size;
>> + }
>> + else
> I'm not sure if this should be "else". I think normal behavior of user
> space is to ask for password then. If keyfile fails we cannot continue
> anyway.
Not sure I follow you. This is saying that the key file data should be
used if given ELSE ask the user for a passphrase.
>
>> + {
>> + /* Get the passphrase from the user. */
>> + tmp = NULL;
>> + if (source->partition)
>> + tmp = grub_partition_get_name (source->partition);
>> + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "),
source->name,
>> + source->partition ? "," : "", tmp ? : "", dev->uuid);
>> + grub_free (tmp);
>> + if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
>> + {
>> + grub_free (split_key);
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
supplied");
>> + }
>> +
>> + passphrase = (grub_uint8_t *)interactive_passphrase;
>> + passphrase_length = grub_strlen (interactive_passphrase);
>> +
>> }
>>
>> /* Try to recover master key from each active keyslot. */
>> @@ -393,7 +411,7 @@ luks_recover_key (grub_disk_t source,
>>
>> /* Calculate the PBKDF2 of the user supplied passphrase. */
>> gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *)
passphrase,
>> - grub_strlen (passphrase),
>> + passphrase_length,
>> header.keyblock[i].passwordSalt,
>> sizeof (header.keyblock[i].passwordSalt),
>> grub_be_to_cpu32 (header.keyblock[i].
>> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
>> index 16dee3c..0299625 100644
>> --- a/include/grub/cryptodisk.h
>> +++ b/include/grub/cryptodisk.h
>> @@ -55,6 +55,8 @@ typedef enum
>> #define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
>> #define GRUB_CRYPTODISK_MAX_KEYLEN 128
>>
>> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
>> +
> Why it is different from MAX_KEYLEN?
A keyfile can be bigger than a key. A offset into the keyfile gives the
start of the key.
The given value is what is implemented by cryptsetup (as reported by
"cryptsetup --help").
The value of MAX_KEYLEN is what existed in Grub prior to patching.
>
>> struct grub_cryptodisk;
>>
>> typedef gcry_err_code_t
>> @@ -108,7 +110,8 @@ struct grub_cryptodisk_dev
>>
>> grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
>> int boot_only, grub_file_t hdr);
>> - grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t
dev, grub_file_t hdr);
>> + grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,
>> + grub_file_t hdr, grub_uint8_t *key, grub_size_t
keyfile_size);
>> };
>> typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
>>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBAgAGBQJViZccAAoJEGnTYCRabxPG3FgP/3v62hWS/5fH9z4KGgRJvaCA
gJSkuy8HcLwBurH8xLqaAwEZe9NVEMeDsbtjS5jeNFiylhYp2kYa1PAgOGb0aAQS
I+i2Ek4soIgPQyC212JzpCot9GI+WUCXObQ/unXVaWrViqL3/DJRoo0Nhes1g0Gh
qvLYJjfBb3Ujl2Ldo9ANWIGATzUSc/wi8oXl/mGWUQ0Gz3hBL3VDKcsjYq3Y8eQD
JpSTr2dJrTKvY+3lnwTlQt4YSbKkOH+PvEdiB/jKGqkw0r7K3BQKGCiIpgeS8bbS
bhLn24HuGIWfCKdZlqvBsmNuB6elUucTUIYkbvLqwD7Q6d/2a/30AzsgOpBgmCR0
dw6EUc0loTBqmpeeChS0Z0+JLMaFzRk8Yxq/FjrASejOXVL3sXLXO/2YW2LyvNTk
PIHSuZ0u8juqwM12xI5eZ94pRq+LNyDvwPcdPqJHsiFyXYTn3JYlPAgD+4R0IHpV
NWWQMIxTR5RdLkVoxGtyAIsKYGcxjd9nY4A0mRvmLZYx8jBBMi0L7XITLpoRy9ZX
ib3a05pfSh6cM1dGHXPXnYjNXga2QBJ7MUrL3HdE48jLSbVd7LtBBpAFczBjrfyJ
Xw5tFvRUbHM5qFBdwCMiFJTIOiQxinBQQfVQ+U0zKH+OPfTC+2svtpNLFr4hGVPS
PblMr2bI0cZAv4zhD96j
=yRSR
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] Cryptomount support key files
2015-06-23 17:27 ` John Lane
@ 2015-06-24 6:59 ` Andrei Borzenkov
2015-06-24 11:26 ` John Lane
0 siblings, 1 reply; 13+ messages in thread
From: Andrei Borzenkov @ 2015-06-24 6:59 UTC (permalink / raw)
To: John Lane; +Cc: The development of GNU GRUB
On Tue, Jun 23, 2015 at 8:27 PM, John Lane <grub@jelmail.com> wrote:
>>> - err = cr->recover_key (source, dev, hdr);
>>> + err = cr->recover_key (source, dev, hdr, key, keyfile_size);
>> You never clear key variable, so after the first call all subsequent
>> invocations will always use it for any device. Moving to proper
>> callback data would make such errors less likely.
> It is cleared when the arguments are processed, specifically
> "grub_cmd_cryptomount" sets "key = NULL".
Right, missed it, sorry.
>>> + keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0,
> 0) : \
>>> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
>>> +
This must check keyfile_size, otherwise it meand buffer overwrite.
>>> + keyfile = grub_file_open (state[4].arg);
>>> + if (!keyfile)
>>> + return grub_errno;
>>> +
>>> + if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
>>> + return grub_errno;
>>> +
>>> + keyfile_size = grub_file_read (keyfile, key, keyfile_size);
>>> + if (keyfile_size == (grub_size_t)-1)
>>> + return grub_errno;
>> If keyfile size is explicitly given, I expect that short read should be
>> considered an error. Otherwise what is the point of explicitly giving
>> size?
> A short read is accepted by the upstream "cryptsetup" tool. Its man page
> describes its "--keyfile-size" argument as "Read a maximum of value
> bytes from the key file. Default is to read the whole file up to the
> compiled-in maximum. The cryptomount command follows that rule.
It is not what my version of cryptsetup says.
From a key file: It will be cropped to the size given by -s. If there
is insufficient key material in the key file, cryptsetup will quit
with an error.
Which is logical. If I state that I want to have N bytes in a key,
getting less is error.
>>> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
>>> + if (keyfile_bytes)
>>> {
>>> - grub_free (split_key);
>>> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
> supplied");
>>> + /* Use bytestring from key file as passphrase */
>>> + passphrase = keyfile_bytes;
>>> + passphrase_length = keyfile_bytes_size;
>>> + }
>>> + else
>> I'm not sure if this should be "else". I think normal behavior of user
>> space is to ask for password then. If keyfile fails we cannot continue
>> anyway.
> Not sure I follow you. This is saying that the key file data should be
> used if given ELSE ask the user for a passphrase.
What I mean - if user requested keyfile but keyfile could not be read,
we probably should fallback to interactive password.
I mostly think about scenario where keyfile is used to decrypt
/boot/grub; in this case we cannot continue if we cannot decrypt it
and we are in pre-normal environment where we cannot easily script. So
asking user for passphrase seems logical here.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] Cryptomount support key files
2015-06-24 6:59 ` Andrei Borzenkov
@ 2015-06-24 11:26 ` John Lane
2015-06-24 12:02 ` Andrei Borzenkov
0 siblings, 1 reply; 13+ messages in thread
From: John Lane @ 2015-06-24 11:26 UTC (permalink / raw)
To: Andrei Borzenkov; +Cc: The development of GNU GRUB
On 24/06/15 07:59, Andrei Borzenkov wrote:
> On Tue, Jun 23, 2015 at 8:27 PM, John Lane <grub@jelmail.com> wrote:
>>>> - err = cr->recover_key (source, dev, hdr);
>>>> + err = cr->recover_key (source, dev, hdr, key, keyfile_size);
>>> You never clear key variable, so after the first call all subsequent
>>> invocations will always use it for any device. Moving to proper
>>> callback data would make such errors less likely.
>> It is cleared when the arguments are processed, specifically
>> "grub_cmd_cryptomount" sets "key = NULL".
> Right, missed it, sorry.
>
>>>> + keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0,
>> 0) : \
>>>> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
>>>> +
> This must check keyfile_size, otherwise it meand buffer overwrite.
I'll add in a check for this.
>
>>>> + keyfile = grub_file_open (state[4].arg);
>>>> + if (!keyfile)
>>>> + return grub_errno;
>>>> +
>>>> + if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
>>>> + return grub_errno;
>>>> +
>>>> + keyfile_size = grub_file_read (keyfile, key, keyfile_size);
>>>> + if (keyfile_size == (grub_size_t)-1)
>>>> + return grub_errno;
>>> If keyfile size is explicitly given, I expect that short read should be
>>> considered an error. Otherwise what is the point of explicitly giving
>>> size?
>> A short read is accepted by the upstream "cryptsetup" tool. Its man page
>> describes its "--keyfile-size" argument as "Read a maximum of value
>> bytes from the key file. Default is to read the whole file up to the
>> compiled-in maximum. The cryptomount command follows that rule.
> It is not what my version of cryptsetup says.
>
> >From a key file: It will be cropped to the size given by -s. If there
> is insufficient key material in the key file, cryptsetup will quit
> with an error.
This is equivalent to the "-l" or "--keyfile-size" option, not the "-s"
or '--key-size' option.
It isn't the number of bytes in the key; it's the maximum number of
bytes that is read from the key file. For LUKS the key file contains a
passphrase which is then used to derive the key (pbkdf2 function). This
argument allows a passphrase length to be given.
My cryptsetup version is 1.6.6 and it says what I wrote above, which is
also reflected in the current upstream head:
https://gitlab.com/cryptsetup/cryptsetup/blob/master/man/cryptsetup.8#L635
> Which is logical. If I state that I want to have N bytes in a key,
> getting less is error.
I can see your logic but I was just following the convention set down by
cryptsetup.
I can make it error if that's preferred but it isn't what cryptsetup does.
>>>> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
>>>> + if (keyfile_bytes)
>>>> {
>>>> - grub_free (split_key);
>>>> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
>> supplied");
>>>> + /* Use bytestring from key file as passphrase */
>>>> + passphrase = keyfile_bytes;
>>>> + passphrase_length = keyfile_bytes_size;
>>>> + }
>>>> + else
>>> I'm not sure if this should be "else". I think normal behavior of user
>>> space is to ask for password then. If keyfile fails we cannot continue
>>> anyway.
>> Not sure I follow you. This is saying that the key file data should be
>> used if given ELSE ask the user for a passphrase.
> What I mean - if user requested keyfile but keyfile could not be read,
> we probably should fallback to interactive password.
>
> I mostly think about scenario where keyfile is used to decrypt
> /boot/grub; in this case we cannot continue if we cannot decrypt it
> and we are in pre-normal environment where we cannot easily script. So
> asking user for passphrase seems logical here.
>
I'll change it so that it falls back to passphrase if it cannot open the
key file. This is in cryptodisk.c.
Should it also fall back to passphrase on other errors (seek and read) ?
The behaviour that I copied from elsewhere in the code was to exit with
an error when these things happen.
Here's the relevant (revised) snippet:
keyfile = grub_file_open (state[4].arg);
if (keyfile)
{
if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
return grub_errno;
key = keyfile_buffer;
keyfile_size = grub_file_read (keyfile, key, keyfile_size);
if (keyfile_size == (grub_size_t)-1)
return grub_errno;
}
else
grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
You can see it errors on seek and read but will continue if the open
fails. "Continue" means return to the grub prompt.
Most commands seem to return to the prompt on error.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] Cryptomount support key files
2015-06-24 11:26 ` John Lane
@ 2015-06-24 12:02 ` Andrei Borzenkov
2015-06-25 20:06 ` John Lane
0 siblings, 1 reply; 13+ messages in thread
From: Andrei Borzenkov @ 2015-06-24 12:02 UTC (permalink / raw)
To: John Lane; +Cc: The development of GNU GRUB
On Wed, Jun 24, 2015 at 2:26 PM, John Lane <grub@jelmail.com> wrote:
>>>>> +
>>>>> + keyfile_size = grub_file_read (keyfile, key, keyfile_size);
>>>>> + if (keyfile_size == (grub_size_t)-1)
>>>>> + return grub_errno;
>>>> If keyfile size is explicitly given, I expect that short read should be
>>>> considered an error. Otherwise what is the point of explicitly giving
>>>> size?
>>> A short read is accepted by the upstream "cryptsetup" tool. Its man page
>>> describes its "--keyfile-size" argument as "Read a maximum of value
>>> bytes from the key file. Default is to read the whole file up to the
>>> compiled-in maximum. The cryptomount command follows that rule.
>> It is not what my version of cryptsetup says.
>>
>> >From a key file: It will be cropped to the size given by -s. If there
>> is insufficient key material in the key file, cryptsetup will quit
>> with an error.
> This is equivalent to the "-l" or "--keyfile-size" option, not the "-s"
> or '--key-size' option.
>
Ah, right. Cryptography must be confusing, otherwise it is not secret :)
> It isn't the number of bytes in the key; it's the maximum number of
> bytes that is read from the key file. For LUKS the key file contains a
> passphrase which is then used to derive the key (pbkdf2 function). This
> argument allows a passphrase length to be given.
>
> My cryptsetup version is 1.6.6 and it says what I wrote above, which is
> also reflected in the current upstream head:
>
> https://gitlab.com/cryptsetup/cryptsetup/blob/master/man/cryptsetup.8#L635
I do not unterpret it your way.
>> Which is logical. If I state that I want to have N bytes in a key,
>> getting less is error.
> I can see your logic but I was just following the convention set down by
> cryptsetup.
> I can make it error if that's preferred but it isn't what cryptsetup does.
Hmm ...
https://gitlab.com/cryptsetup/cryptsetup/blob/master/lib/utils_crypt.c#L446
if (!unlimited_read && i != keyfile_size_max) {
log_err(cd, _("Cannot read requested amount of data.\n"));
goto out_err;
}
So yes, it reads as much as it can - unless it is explicitly requested
to read specific amount of data.
>>>>> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
>>>>> + if (keyfile_bytes)
>>>>> {
>>>>> - grub_free (split_key);
>>>>> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
>>> supplied");
>>>>> + /* Use bytestring from key file as passphrase */
>>>>> + passphrase = keyfile_bytes;
>>>>> + passphrase_length = keyfile_bytes_size;
>>>>> + }
>>>>> + else
>>>> I'm not sure if this should be "else". I think normal behavior of user
>>>> space is to ask for password then. If keyfile fails we cannot continue
>>>> anyway.
>>> Not sure I follow you. This is saying that the key file data should be
>>> used if given ELSE ask the user for a passphrase.
>> What I mean - if user requested keyfile but keyfile could not be read,
>> we probably should fallback to interactive password.
>>
>> I mostly think about scenario where keyfile is used to decrypt
>> /boot/grub; in this case we cannot continue if we cannot decrypt it
>> and we are in pre-normal environment where we cannot easily script. So
>> asking user for passphrase seems logical here.
>>
>
> I'll change it so that it falls back to passphrase if it cannot open the
> key file. This is in cryptodisk.c.
> Should it also fall back to passphrase on other errors (seek and read) ?
I do not right now see reasons to differentiate. Either we could read
key or not. Moreover, it pobably should fallback to pasphrase even if
key file is present but verification failed. We may add --silent or
similar if some use case emerges.
> The behaviour that I copied from elsewhere in the code was to exit with
> an error when these things happen.
>
User using cryptsetup in full blown Unix shell has much better ways to
handle such errors; we do not.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] Cryptomount support key files
2015-06-24 12:02 ` Andrei Borzenkov
@ 2015-06-25 20:06 ` John Lane
0 siblings, 0 replies; 13+ messages in thread
From: John Lane @ 2015-06-25 20:06 UTC (permalink / raw)
To: Andrei Borzenkov; +Cc: The development of GNU GRUB
On 24/06/15 13:02, Andrei Borzenkov wrote:
> On Wed, Jun 24, 2015 at 2:26 PM, John Lane <grub@jelmail.com> wrote:
>>>>>> +
>>>>>> + keyfile_size = grub_file_read (keyfile, key, keyfile_size);
>>>>>> + if (keyfile_size == (grub_size_t)-1)
>>>>>> + return grub_errno;
>>>>> If keyfile size is explicitly given, I expect that short read should be
>>>>> considered an error. Otherwise what is the point of explicitly giving
>>>>> size?
>>>> A short read is accepted by the upstream "cryptsetup" tool. Its man page
>>>> describes its "--keyfile-size" argument as "Read a maximum of value
>>>> bytes from the key file. Default is to read the whole file up to the
>>>> compiled-in maximum. The cryptomount command follows that rule.
>>> It is not what my version of cryptsetup says.
>>>
>>> >From a key file: It will be cropped to the size given by -s. If there
>>> is insufficient key material in the key file, cryptsetup will quit
>>> with an error.
>> This is equivalent to the "-l" or "--keyfile-size" option, not the "-s"
>> or '--key-size' option.
>>
> Ah, right. Cryptography must be confusing, otherwise it is not secret :)
>
>> It isn't the number of bytes in the key; it's the maximum number of
>> bytes that is read from the key file. For LUKS the key file contains a
>> passphrase which is then used to derive the key (pbkdf2 function). This
>> argument allows a passphrase length to be given.
>>
>> My cryptsetup version is 1.6.6 and it says what I wrote above, which is
>> also reflected in the current upstream head:
>>
>> https://gitlab.com/cryptsetup/cryptsetup/blob/master/man/cryptsetup.8#L635
> I do not unterpret it your way.
>
>>> Which is logical. If I state that I want to have N bytes in a key,
>>> getting less is error.
>> I can see your logic but I was just following the convention set down by
>> cryptsetup.
>> I can make it error if that's preferred but it isn't what cryptsetup does.
> Hmm ...
>
> https://gitlab.com/cryptsetup/cryptsetup/blob/master/lib/utils_crypt.c#L446
>
> if (!unlimited_read && i != keyfile_size_max) {
> log_err(cd, _("Cannot read requested amount of data.\n"));
> goto out_err;
> }
>
> So yes, it reads as much as it can - unless it is explicitly requested
> to read specific amount of data.
>
>>>>>> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
>>>>>> + if (keyfile_bytes)
>>>>>> {
>>>>>> - grub_free (split_key);
>>>>>> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
>>>> supplied");
>>>>>> + /* Use bytestring from key file as passphrase */
>>>>>> + passphrase = keyfile_bytes;
>>>>>> + passphrase_length = keyfile_bytes_size;
>>>>>> + }
>>>>>> + else
>>>>> I'm not sure if this should be "else". I think normal behavior of user
>>>>> space is to ask for password then. If keyfile fails we cannot continue
>>>>> anyway.
>>>> Not sure I follow you. This is saying that the key file data should be
>>>> used if given ELSE ask the user for a passphrase.
>>> What I mean - if user requested keyfile but keyfile could not be read,
>>> we probably should fallback to interactive password.
>>>
>>> I mostly think about scenario where keyfile is used to decrypt
>>> /boot/grub; in this case we cannot continue if we cannot decrypt it
>>> and we are in pre-normal environment where we cannot easily script. So
>>> asking user for passphrase seems logical here.
>>>
>> I'll change it so that it falls back to passphrase if it cannot open the
>> key file. This is in cryptodisk.c.
>> Should it also fall back to passphrase on other errors (seek and read) ?
> I do not right now see reasons to differentiate. Either we could read
> key or not.
OK, I've re-coded the keyfile bit to continue to passphrase on error,
and being unable to read a specified keyfile size is considered an error
now in addition to failure to open, seek or read.
revised patch forthcoming...
> Moreover, it pobably should fallback to pasphrase even if
> key file is present but verification failed. We may add --silent or
> similar if some use case emerges.
How about making it so the user gets a number of attempts, so that a bad
passphrase results in the passphrase prompt again (a bit like when you
log in to a getty). The number of tries could be preset, say to 2. If a
keyfile is given then that uses up one try. A bad keyfile would then
result in a passphrase prompt.
The way the existing code is written would fit that model easily without
too much trouble.
>
>> The behaviour that I copied from elsewhere in the code was to exit with
>> an error when these things happen.
>>
> User using cryptsetup in full blown Unix shell has much better ways to
> handle such errors; we do not.
I meant in the Grub code ;)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] Cryptomount support plain dm-crypt
2015-06-16 9:11 Cryptomount enhancements: detached headers, key-files and plain mode John Lane
2015-06-16 9:11 ` [PATCH 1/4] Cryptomount support LUKS detached header John Lane
2015-06-16 9:11 ` [PATCH 2/4] Cryptomount support key files John Lane
@ 2015-06-16 9:11 ` John Lane
2015-06-16 9:45 ` John Lane
2015-06-16 9:11 ` [PATCH 4/4] Cryptomount support for hyphens in UUID John Lane
3 siblings, 1 reply; 13+ messages in thread
From: John Lane @ 2015-06-16 9:11 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
---
grub-core/disk/cryptodisk.c | 298 +++++++++++++++++++++++++++++++++++++++++++-
grub-core/disk/luks.c | 205 +-----------------------------
include/grub/cryptodisk.h | 8 ++
3 files changed, 309 insertions(+), 202 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index fbe7db9..c519c55 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -45,6 +45,11 @@ static const struct grub_arg_option options[] =
{"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
{"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},
{0, 0, 0, 0, 0, 0}
};
@@ -928,6 +933,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)
{
@@ -1033,7 +1080,64 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
- err = grub_cryptodisk_scan_device_real (args[0], disk);
+ if (state[8].set) /* Plain mode */
+ {
+ char *cipher;
+ char *mode;
+ char *digest;
+ int offset, size, key_size;
+
+ cipher = grub_strdup (state[9].set ? state[9].arg : GRUB_CRYPTODISK_PLAIN_CIPHER);
+ digest = grub_strdup (state[10].set ? state[10].arg : GRUB_CRYPTODISK_PLAIN_DIGEST);
+ offset = state[11].set ? grub_strtoul (state[11].arg, 0, 0) : 0;
+ size = state[12].set ? grub_strtoul (state[12].arg, 0, 0) : 0;
+ key_size = ( state[5].set ? grub_strtoul (state[5].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;
+
+ grub_printf ("\nCipher='%s' mode='%s'\n", cipher, mode);
+ 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 (args[0], disk);
grub_disk_close (disk);
@@ -1164,13 +1268,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 0d0db9d..069e72c 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 */
@@ -70,21 +68,11 @@ 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];
- 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;
@@ -114,21 +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;
@@ -139,184 +112,16 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
uuid[sizeof (header.uuid)] = 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
+ if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
{
- grub_crypto_cipher_close (cipher);
- grub_crypto_cipher_close (secondary_cipher);
- grub_error (GRUB_ERR_BAD_ARGUMENT, "Unknown IV mode: %s",
- cipheriv);
+ grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
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;
}
@@ -331,7 +136,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)];
@@ -385,7 +190,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 0299625..4076412 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
@@ -159,4 +164,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.1.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/4] Cryptomount support for hyphens in UUID
2015-06-16 9:11 Cryptomount enhancements: detached headers, key-files and plain mode John Lane
` (2 preceding siblings ...)
2015-06-16 9:11 ` [PATCH 3/4] Cryptomount support plain dm-crypt John Lane
@ 2015-06-16 9:11 ` John Lane
2015-06-20 5:13 ` Andrei Borzenkov
3 siblings, 1 reply; 13+ messages in thread
From: John Lane @ 2015-06-16 9:11 UTC (permalink / raw)
To: grub-devel; +Cc: John Lane
---
grub-core/disk/cryptodisk.c | 9 +++++++++
grub-core/disk/luks.c | 1 +
include/grub/cryptodisk.h | 3 +++
3 files changed, 13 insertions(+)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index c519c55..b800d6f 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -113,6 +113,13 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const grub_uint8_t *b)
}
}
+void
+grub_cryptodisk_uuid_dehyphenate(char *uuid)
+{
+ char *s, *d;
+ for (s=d=uuid;(*d=*s);d+=(*s++!='-'));
+}
+
static gcry_err_code_t
grub_crypto_pcbc_decrypt (grub_crypto_cipher_handle_t cipher,
void *out, void *in, grub_size_t size,
@@ -506,6 +513,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
{
+ grub_cryptodisk_uuid_dehyphenate((char *)name + sizeof ("cryptouuid/"));
for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
break;
@@ -1025,6 +1033,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
{
grub_cryptodisk_t dev;
+ grub_cryptodisk_uuid_dehyphenate(args[0]);
dev = grub_cryptodisk_get_by_uuid (args[0]);
if (dev)
{
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 069e72c..0e570cf 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -111,6 +111,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
hashspec[sizeof (header.hashSpec)] = 0;
grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
uuid[sizeof (header.uuid)] = 0;
+ grub_cryptodisk_uuid_dehyphenate (uuid);
if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
{
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 4076412..cfb0f0d 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -167,4 +167,7 @@ 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);
+void
+grub_cryptodisk_uuid_dehyphenate(char *uuid);
+
#endif
--
2.1.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/4] Cryptomount support for hyphens in UUID
2015-06-16 9:11 ` [PATCH 4/4] Cryptomount support for hyphens in UUID John Lane
@ 2015-06-20 5:13 ` Andrei Borzenkov
0 siblings, 0 replies; 13+ messages in thread
From: Andrei Borzenkov @ 2015-06-20 5:13 UTC (permalink / raw)
To: John Lane; +Cc: grub-devel
В Tue, 16 Jun 2015 10:11:15 +0100
John Lane <john@lane.uk.net> пишет:
> ---
> grub-core/disk/cryptodisk.c | 9 +++++++++
> grub-core/disk/luks.c | 1 +
> include/grub/cryptodisk.h | 3 +++
> 3 files changed, 13 insertions(+)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c519c55..b800d6f 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -113,6 +113,13 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const grub_uint8_t *b)
> }
> }
>
> +void
> +grub_cryptodisk_uuid_dehyphenate(char *uuid)
> +{
> + char *s, *d;
> + for (s=d=uuid;(*d=*s);d+=(*s++!='-'));
Did you try to fit smartphone size? We are not short on spaces yet.
> +}
> +
> static gcry_err_code_t
> grub_crypto_pcbc_decrypt (grub_crypto_cipher_handle_t cipher,
> void *out, void *in, grub_size_t size,
> @@ -506,6 +513,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
>
> if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
> {
> + grub_cryptodisk_uuid_dehyphenate((char *)name + sizeof ("cryptouuid/"));
We do not really know what underlying implementation expects as UUID.
It *may* contain hyphens as valid character.
> for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
> break;
> @@ -1025,6 +1033,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> {
> grub_cryptodisk_t dev;
>
> + grub_cryptodisk_uuid_dehyphenate(args[0]);
Same: cryptomount is generic interface. We do not know what underlying
implementation expects.
> dev = grub_cryptodisk_get_by_uuid (args[0]);
> if (dev)
> {
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 069e72c..0e570cf 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -111,6 +111,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> hashspec[sizeof (header.hashSpec)] = 0;
> grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
> uuid[sizeof (header.uuid)] = 0;
> + grub_cryptodisk_uuid_dehyphenate (uuid);
>
It already removes hyphens when getting UUID from LUKS. But you may
want to remove hyphens from check_uuid string too.
> if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> {
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 4076412..cfb0f0d 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -167,4 +167,7 @@ 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);
>
> +void
> +grub_cryptodisk_uuid_dehyphenate(char *uuid);
> +
> #endif
This function is needed in one place only, so moving it into luks makes
more sense for now.
^ permalink raw reply [flat|nested] 13+ messages in thread