[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Cryptomount support for key files and detached header
|
From: |
Glenn Washburn |
|
Subject: |
Re: [PATCH] Cryptomount support for key files and detached header |
|
Date: |
Sun, 8 Nov 2020 21:16:51 -0600 |
I've read through the patch but not applied or tested it. However, it
looks like it does the job. In fact, its fairly similar in parts to a
patch, which adds LUKS2 keyfile and detached header support, I've been
waiting to send to the list until the previous LUKS1 keyfile and
detached header support gets included (which my patches rely on).
One implementation difference in this patch from the other one is that
this one passes the key file to recover_key as a grub_file_t instead of
passing the key contents. I prefer the previous method because it
consolidates key file reading in grub_cmd_cryptomount, whereas this
patch requires each module to do its own reading. The disadvantage is
this causes extra code in the modules that could be consolidated and I
don't see much advantage to this. By having grub_cmd_cryptomount do
the work we also get keyfile-offset and keyfile-size options in the
previous patch, for not much extra work.
A minor nit, there is not error checking on grub_file_seek calls and
grub_file_read errors are occluded. However, there are some changes in
this patch which I think make the code a bit more aesthetically
pleasing.
While, I like that this patch allows for passing a master key file, I
think this feature should be build on the previous patch series. So I
don't recommend this patch be accepted.
Glenn
On Sat, 7 Nov 2020 19:36:35 +0300
reagentoo <reagentoo@gmail.com> wrote:
> From: Dmitry Baranov <reagentoo@gmail.com>
>
> Hello.
> Could someone make initial review?
> The main difference between this patch and the previous ones is
> the ability to use a master key file with a detached header.
> This patch does not provide luks1 and geli support yet (luks2 only).
>
> Best Regards,
> Dmitry
>
> ---
> grub-core/disk/cryptodisk.c | 99 +++++++++++++++-----
> grub-core/disk/luks2.c | 178
> ++++++++++++++++++++++++++++-------- include/grub/cryptodisk.h |
> 9 +- 3 files changed, 222 insertions(+), 64 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 13af84dd1..987a39b16 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,9 @@ 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 header from file"), 0,
> ARG_TYPE_STRING},
> + {"key-file", 'k', 0, N_("Read key from file"), 0,
> ARG_TYPE_STRING},
> + {"master-key-file", 'K', 0, N_("Use a master key stored in a
> file"), 0, ARG_TYPE_STRING}, {0, 0, 0, 0, 0, 0}
> };
>
> @@ -967,6 +970,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_file, key_file, mkey_file;
>
> static void
> cryptodisk_close (grub_cryptodisk_t dev)
> @@ -991,13 +995,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, hdr_file, search_uuid, check_boot);
> if (grub_errno)
> return grub_errno;
> if (!dev)
> continue;
>
> - err = cr->recover_key (source, dev);
> + err = cr->recover_key (source, dev, hdr_file, key_file,
> mkey_file); if (err)
> {
> cryptodisk_close (dev);
> @@ -1038,7 +1042,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, 0, search_uuid, check_boot);
> if (grub_errno)
> return grub_errno;
> if (!dev)
> @@ -1087,11 +1091,59 @@ grub_cryptodisk_scan_device (const char *name,
> static grub_err_t
> grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char
> **args) {
> + grub_err_t ret = GRUB_ERR_NONE;
> +
> struct grub_arg_list *state = ctxt->state;
>
> if (argc < 1 && !state[1].set && !state[2].set)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name
> required");
> + hdr_file = NULL;
> + key_file = NULL;
> + mkey_file = NULL;
> +
> + if (state[3].set)
> + {
> + if (state[0].set)
> + {
> + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "cannot use UUID
> lookup with detached header");
> + goto err;
> + }
> +
> + hdr_file = grub_file_open (state[3].arg, GRUB_FILE_TYPE_NONE);
> + if (!hdr_file)
> + {
> + ret = grub_errno;
> + goto err;
> + }
> + }
> +
> + if (state[4].set)
> + {
> + if (state[5].set)
> + {
> + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "cannot use key
> with master key");
> + goto err;
> + }
> +
> + key_file = grub_file_open (state[4].arg, GRUB_FILE_TYPE_NONE);
> + if (!key_file)
> + {
> + ret = grub_errno;
> + goto err;
> + }
> + }
> +
> + if (state[5].set)
> + {
> + mkey_file = grub_file_open (state[5].arg, GRUB_FILE_TYPE_NONE);
> + if (!mkey_file)
> + {
> + ret = grub_errno;
> + goto err;
> + }
> + }
> +
> have_it = 0;
> if (state[0].set)
> {
> @@ -1102,7 +1154,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) {
> grub_dprintf ("cryptodisk",
> "already mounted as crypto%lu\n", dev->id);
> - return GRUB_ERR_NONE;
> + goto err;
> }
>
> check_boot = state[2].set;
> @@ -1111,8 +1163,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) search_uuid = NULL;
>
> if (!have_it)
> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> cryptodisk found");
> - return GRUB_ERR_NONE;
> + {
> + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> cryptodisk found");
> + goto err;
> + }
> + goto err;
> }
> else if (state[1].set || (argc == 0 && state[2].set))
> {
> @@ -1120,11 +1175,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) check_boot = state[2].set;
> grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> search_uuid = NULL;
> - return GRUB_ERR_NONE;
> + goto err;
> }
> else
> {
> - grub_err_t err;
> grub_disk_t disk;
> grub_cryptodisk_t dev;
> char *diskname;
> @@ -1147,27 +1201,30 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) {
> if (disklast)
> *disklast = ')';
> - return grub_errno;
> + ret = grub_errno;
> + goto err;
> }
>
> dev = grub_cryptodisk_get_by_source_disk (disk);
> if (dev)
> - {
> - grub_dprintf ("cryptodisk", "already mounted as
> crypto%lu\n", dev->id);
> - grub_disk_close (disk);
> - if (disklast)
> - *disklast = ')';
> - return GRUB_ERR_NONE;
> - }
> -
> - err = grub_cryptodisk_scan_device_real (diskname, disk);
> + grub_dprintf ("cryptodisk", "already mounted as
> crypto%lu\n", dev->id);
> + else
> + ret = grub_cryptodisk_scan_device_real (diskname, disk);
>
> grub_disk_close (disk);
> if (disklast)
> *disklast = ')';
> -
> - return err;
> }
> +
> + err:
> + if (hdr_file)
> + grub_file_close (hdr_file);
> + if (key_file)
> + grub_file_close (key_file);
> + if (mkey_file)
> + grub_file_close (mkey_file);
> +
> + return ret;
> }
>
> static struct grub_disk_dev grub_cryptodisk_dev = {
> @@ -1299,7 +1356,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|-k
> file|-K file"), N_("Mount a crypto device."), options);
> grub_procfs_register ("luks_script", &luks_script);
> }
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 31d7166fc..c29b472e6 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -309,13 +309,22 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> grub_luks2_digest_t *d, grub_luks2_s
> /* Determine whether to use primary or secondary header */
> static grub_err_t
> -luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
> +luks2_read_header (grub_disk_t disk, grub_file_t hdr_file,
> grub_luks2_header_t *outhdr) {
> grub_luks2_header_t primary, secondary, *header = &primary;
> grub_err_t ret;
>
> /* Read the primary LUKS header. */
> - ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
> + if (hdr_file)
> + {
> + grub_file_seek (hdr_file, 0);
> + if (grub_file_read (hdr_file, &primary, sizeof (primary)) !=
> sizeof (primary))
> + ret = GRUB_ERR_READ_ERROR;
> + else
> + ret = GRUB_ERR_NONE;
> + }
> + else
> + ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
> if (ret)
> return ret;
>
> @@ -325,7 +334,16 @@ luks2_read_header (grub_disk_t disk,
> grub_luks2_header_t *outhdr) return GRUB_ERR_BAD_SIGNATURE;
>
> /* Read the secondary header. */
> - ret = grub_disk_read (disk, 0, grub_be_to_cpu64
> (primary.hdr_size), sizeof (secondary), &secondary);
> + if (hdr_file)
> + {
> + grub_file_seek (hdr_file, grub_be_to_cpu64 (primary.hdr_size));
> + if (grub_file_read (hdr_file, &secondary, sizeof (secondary))
> != sizeof (secondary))
> + ret = GRUB_ERR_READ_ERROR;
> + else
> + ret = GRUB_ERR_NONE;
> + }
> + else
> + ret = grub_disk_read (disk, 0, grub_be_to_cpu64
> (primary.hdr_size), sizeof (secondary), &secondary); if (ret)
> return ret;
>
> @@ -342,7 +360,7 @@ luks2_read_header (grub_disk_t disk,
> grub_luks2_header_t *outhdr) }
>
> static grub_cryptodisk_t
> -luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
> +luks2_scan (grub_disk_t disk, grub_file_t hdr_file, const char
> *check_uuid, int check_boot) {
> grub_cryptodisk_t cryptodisk;
> grub_luks2_header_t header;
> @@ -352,7 +370,7 @@ luks2_scan (grub_disk_t disk, const char
> *check_uuid, int check_boot) if (check_boot)
> return NULL;
>
> - if (luks2_read_header (disk, &header))
> + if (luks2_read_header (disk, hdr_file, &header))
> {
> grub_errno = GRUB_ERR_NONE;
> return NULL;
> @@ -415,7 +433,7 @@ luks2_verify_key (grub_luks2_digest_t *d,
> grub_uint8_t *candidate_key,
> static grub_err_t
> luks2_decrypt_key (grub_uint8_t *out_key,
> - grub_disk_t disk, grub_cryptodisk_t crypt,
> + grub_disk_t disk, grub_cryptodisk_t crypt,
> grub_file_t hdr_file, grub_luks2_keyslot_t *k,
> const grub_uint8_t *passphrase, grub_size_t
> passphraselen) {
> @@ -491,7 +509,16 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> }
>
> grub_errno = GRUB_ERR_NONE;
> - ret = grub_disk_read (disk, 0, k->area.offset, k->area.size,
> split_key);
> + if (hdr_file)
> + {
> + grub_file_seek (hdr_file, k->area.offset);
> + if (grub_file_read (hdr_file, split_key, k->area.size) !=
> k->area.size)
> + ret = GRUB_ERR_READ_ERROR;
> + else
> + ret = GRUB_ERR_NONE;
> + }
> + else
> + ret = grub_disk_read (disk, 0, k->area.offset, k->area.size,
> split_key); if (ret)
> {
> grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
> @@ -531,12 +558,16 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>
> static grub_err_t
> luks2_recover_key (grub_disk_t disk,
> - grub_cryptodisk_t crypt)
> + grub_cryptodisk_t crypt,
> + grub_file_t hdr_file, grub_file_t key_file,
> grub_file_t mkey_file) {
> + char cipher[32];
> + char *passphrase = NULL;
> grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
> - char passphrase[MAX_PASSPHRASE], cipher[32];
> + grub_size_t candidate_key_len, passphrase_len;
> char *json_header = NULL, *part = NULL, *ptr;
> - grub_size_t candidate_key_len = 0, i, size;
> + grub_off_t json_header_offset;
> + grub_size_t json_header_size, i, size;
> grub_luks2_header_t header;
> grub_luks2_keyslot_t keyslot;
> grub_luks2_digest_t digest;
> @@ -545,7 +576,7 @@ luks2_recover_key (grub_disk_t disk,
> grub_json_t *json = NULL, keyslots;
> grub_err_t ret;
>
> - ret = luks2_read_header (disk, &header);
> + ret = luks2_read_header (disk, hdr_file, &header);
> if (ret)
> return ret;
>
> @@ -554,8 +585,19 @@ luks2_recover_key (grub_disk_t disk,
> return GRUB_ERR_OUT_OF_MEMORY;
>
> /* Read the JSON area. */
> - ret = grub_disk_read (disk, 0, grub_be_to_cpu64
> (header.hdr_offset) + sizeof (header),
> - grub_be_to_cpu64 (header.hdr_size) - sizeof
> (header), json_header);
> + json_header_offset = grub_be_to_cpu64 (header.hdr_offset) + sizeof
> (header);
> + json_header_size = grub_be_to_cpu64 (header.hdr_size) - sizeof
> (header); +
> + if (hdr_file)
> + {
> + grub_file_seek (hdr_file, json_header_offset);
> + if (grub_file_read (hdr_file, json_header, json_header_size)
> != json_header_size)
> + ret = GRUB_ERR_READ_ERROR;
> + else
> + ret = GRUB_ERR_NONE;
> + }
> + else
> + ret = grub_disk_read (disk, 0, json_header_offset,
> json_header_size, json_header); if (ret)
> goto err;
>
> @@ -570,16 +612,47 @@ luks2_recover_key (grub_disk_t disk,
> goto err;
> }
>
> - /* Get the passphrase from the user. */
> - if (disk->partition)
> - part = grub_partition_get_name (disk->partition);
> - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name,
> - disk->partition ? "," : "", part ? : "",
> - crypt->uuid);
> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> + if (mkey_file)
> {
> - ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
> supplied");
> - goto err;
> + /* Get the master key from file. */
> + candidate_key_len = grub_file_read (mkey_file, candidate_key,
> GRUB_CRYPTODISK_MAX_KEYLEN);
> + if (candidate_key_len == (grub_size_t)-1)
> + {
> + ret = grub_error (GRUB_ERR_FILE_READ_ERROR, "Error reading
> key file");
> + goto err;
> + }
> + if (candidate_key_len == 0)
> + {
> + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Key not
> supplied");
> + goto err;
> + }
> + }
> + else if (key_file)
> + {
> + /* Get the passphrase from file. */
> + passphrase = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> + passphrase_len = grub_file_read (key_file, passphrase,
> GRUB_CRYPTODISK_MAX_PASSPHRASE);
> + if (passphrase_len == (grub_size_t)-1)
> + {
> + ret = grub_error (GRUB_ERR_FILE_READ_ERROR, "Error reading
> passphrase file");
> + goto err;
> + }
> + }
> + else
> + {
> + /* Get the passphrase from the user. */
> + if (disk->partition)
> + part = grub_partition_get_name (disk->partition);
> + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "),
> disk->name,
> + disk->partition ? "," : "", part ? : "",
> + crypt->uuid);
> + passphrase = grub_malloc (MAX_PASSPHRASE);
> + if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> + {
> + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
> supplied");
> + goto err;
> + }
> + passphrase_len = grub_strlen (passphrase);
> }
>
> if (grub_json_getvalue (&keyslots, json, "keyslots") ||
> @@ -604,6 +677,26 @@ luks2_recover_key (grub_disk_t disk,
>
> grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", i);
>
> + if (mkey_file)
> + {
> + if (candidate_key_len != keyslot.key_size)
> + {
> + grub_dprintf ("luks2", "Wrong key length for keyslot
> %"PRIuGRUB_SIZE": %s\n",
> + i, grub_errmsg);
> + continue;
> + }
> +
> + ret = luks2_verify_key (&digest, candidate_key,
> candidate_key_len);
> + if (ret)
> + {
> + grub_dprintf ("luks2", "Could not open keyslot
> %"PRIuGRUB_SIZE": %s\n",
> + i, grub_errmsg);
> + continue;
> + }
> + }
> + else
> + candidate_key_len = keyslot.key_size;
> +
> /* Set up disk according to keyslot's segment. */
> crypt->offset = grub_divmod64 (segment.offset,
> segment.sector_size, NULL); crypt->log_sector_size = sizeof (unsigned
> int) * 8 @@ -613,21 +706,24 @@ luks2_recover_key (grub_disk_t disk,
> else
> crypt->total_length = grub_strtoull (segment.size, NULL, 10);
>
> - ret = luks2_decrypt_key (candidate_key, disk, crypt, &keyslot,
> - (const grub_uint8_t *) passphrase,
> grub_strlen (passphrase));
> - if (ret)
> + if (passphrase)
> {
> - grub_dprintf ("luks2", "Decryption with keyslot
> %"PRIuGRUB_SIZE" failed: %s\n",
> - i, grub_errmsg);
> - continue;
> - }
> -
> - ret = luks2_verify_key (&digest, candidate_key,
> keyslot.key_size);
> - if (ret)
> - {
> - grub_dprintf ("luks2", "Could not open keyslot
> %"PRIuGRUB_SIZE": %s\n",
> - i, grub_errmsg);
> - continue;
> + ret = luks2_decrypt_key (candidate_key, disk, crypt,
> hdr_file, &keyslot,
> + (const grub_uint8_t *)
> passphrase, passphrase_len);
> + if (ret)
> + {
> + grub_dprintf ("luks2", "Decryption with keyslot
> %"PRIuGRUB_SIZE" failed: %s\n",
> + i, grub_errmsg);
> + continue;
> + }
> +
> + ret = luks2_verify_key (&digest, candidate_key,
> candidate_key_len);
> + if (ret)
> + {
> + grub_dprintf ("luks2", "Could not open keyslot
> %"PRIuGRUB_SIZE": %s\n",
> + i, grub_errmsg);
> + continue;
> + }
> }
>
> /*
> @@ -635,13 +731,14 @@ luks2_recover_key (grub_disk_t disk,
> * where each element is either empty or holds a key.
> */
> grub_printf_ (N_("Slot %"PRIuGRUB_SIZE" opened\n"), i);
> -
> - candidate_key_len = keyslot.key_size;
> break;
> }
> - if (candidate_key_len == 0)
> + if (i == size)
> {
> - ret = grub_error (GRUB_ERR_ACCESS_DENIED, "Invalid
> passphrase");
> + if (mkey_file)
> + ret = grub_error (GRUB_ERR_ACCESS_DENIED, "Invalid key");
> + else
> + ret = grub_error (GRUB_ERR_ACCESS_DENIED, "Invalid
> passphrase"); goto err;
> }
>
> @@ -666,6 +763,7 @@ luks2_recover_key (grub_disk_t disk,
>
> err:
> grub_free (part);
> + grub_free (passphrase);
> grub_free (json_header);
> grub_json_free (json);
> return ret;
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index e1b21e785..b53006854 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -21,6 +21,7 @@
>
> #include <grub/disk.h>
> #include <grub/crypto.h>
> +#include <grub/file.h>
> #include <grub/list.h>
> #ifdef GRUB_UTIL
> #include <grub/emu/hostdisk.h>
> @@ -53,6 +54,7 @@ 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 8192
>
> struct grub_cryptodisk;
>
> @@ -106,9 +108,10 @@ struct grub_cryptodisk_dev
> struct grub_cryptodisk_dev *next;
> 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);
> + grub_cryptodisk_t (*scan) (grub_disk_t disk, grub_file_t hdr_file,
> + const char *check_uuid, int boot_only);
> + grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,
> + grub_file_t hdr_file, grub_file_t
> key_file, grub_file_t mkey_file); };
> typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
>