Thanks for feedback. First of all I think it's preferable to introduce the master-key option at the beginning. Because I see no point for a user to use a standard slot key along with a detached header. Decryption from key to master key takes a long time. (30 seconds for argon2i). Regarding keyfile-offset and keyfile-size options I suggest to introduce this options in separate patches. Yes, maybe you variant to pass the pointer and size (correct me if I am wrong) to luks2_recover_key is better of files. But then you need to think in advance how to add master-key option.
Dmitry пн, 9 нояб. 2020 г. в 06:16, Glenn Washburn <developm...@efficientek.com>: > 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 <reagen...@gmail.com> wrote: > > > From: Dmitry Baranov <reagen...@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; > > >
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel