-----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 <j...@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----- _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel