Re: Patches to cryptomount (plain support, keyfiles and LUKS detached headers)

[John Lane]

On 13/06/15 06:27, Andrei Borzenkov wrote:
> В Fri, 12 Jun 2015 20:15:32 +0100
> John Lane  пишет:
>
>>> Sorry, we cannot accept patches which aren't sent to this ml by author.
>> I've attached the patches here. They apply clean to c945ca75.
> Sending them as patch series in mail body would make it easier to
> review and comment on them.
Ok, sure I'll do that. please bear with me...
I'll re-jig the patches so that the plain-mode stuff is separated.
May be a delay as I'll need to find the time to do it.
>
>>> I'm not sure that all features are good. For starters plain mode is just
>>> difficult to setup and use. Please provide usecases not already covered
>>> by current features.
>> My target was to establish LUKS volumes with detached headers and key
>> files and this is not already covered by current features.
>>
>> My specific use-case is booting secured systems where the boot
>> environment (Grub, LUKS headers and keys) is contained on removable
>> media such as a USB key. The non-removable hard-drive has no boot code
>> on it; it just appears as an unformatted disk unless the removable key
>> is used.
>>
>> To support this, it was necessary to add support to Grub for detached
>> LUKS headers and keys.
>>
>> I am aware of a number of other people enquiring about this specific
>> functionality so I am not alone in thinking it's a valid use-case.
>>
>> Regarding plain mode,  I don't understand why plain mode is "difficult
>> to setup and use". I did the work on plain mode at the same time because
>> one of the disks that I needed to work with was a plain mode disk. I
>> asked about the existing but non-functioning "peter/devmapper" branch
>> and spent some time trying to get that to work. In the end, and as I
>> understand how LUKS uses dm-crypt, it seemed better to re-use the
>> existing code base in the cryptodisk routines because this is more
>> current, used and tested. By doing that I was able to get it to work
>> very quickly.
>>
> The problem is not coding it. It is impossible to identify plain mode
> crypto-containers at run time. So it depends on user knowing (or
> guessing) which of disks to use. It is pure manual stuff which cannot
> be integrated in GRUB grub-install or grub-mkconfig.
>
> You have 5 disks each encrypted with different key that are plain/use
> detached header. How can you setup GRUB to automatically find out the
> right key/header for each disk?
>
> I personally would appreciate keyfile support as separate patch, not
> buried in plain mode support. Especially if it would be accompanied by
> grub-mkconfig changes to automate generation of grub.cfg to use it. 
I'll separate the plain from LUKS stuff as said above... might take a
little while to find some time though...
>> I've been using my changes in daily use since my original postings last
>> December. I've just updated to the latest head and the patches still
>> merge cleanly.
>>
>> I'd appreciate it if these changes could be considered. If any more
>> information would be useful please let me know.
>>
>
>> @@ -488,6 +496,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
>>  
>>if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
>>  {
>> +  grub_crypto_uuid_dehyphenate((char *)name + sizeof ("cryptouuid/"));
> This alters original name passed to grub_disk_open. As already
> mentioned it is better to use comparison function that ignores hyphens.
The reason I did it that way was because it was easy, coming from the
point of view that I was only getting to know the code-base.
I simply converted the uuid at the first opportunity into the format
that the existing code base expects. That way the rest of the code can
carry on as if a uuid without hyphens was presented. I thought that was
pretty harmless given that the dehyphenate function does nothing to a
uuid that has no hyphens in it. Anyway, I will resubmit the patch by
email as described above - we can discuss it further under that...

>




signature.asc
Description: OpenPGP digital signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 2/4] Cryptomount support key files

[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 

Cryptomount enhancements: detached headers, key-files and plain mode

[John Lane]

These patches provide extensions to the "cryptomount" command. There are four 
patches:

1. Support LUKS detached headers so that the header can be separated from the 
data payload, e.g. by storing on external removable media such as a USB key.

2. Support key files so that passphrase entry can be suppressed. The passphrase 
can be stored in a "key file" that can be stored, for example, on external 
removable media such as a USB key.

3. Support plain dm-crypt mode. Allow plain volumes to be opened. This is 
largely a re-factoring of exisitng code to allow the crypto routines be used 
independently of LUKS.

4. Support for hyphens in UUID. The "-u" option of cryptomount accepts a UUID. 
This option allows that to be delimited with hyphens so that the same format 
can be given to Grub as is passed to the Linux kernel boot options.

I can supply more information as required in reply to the individual patches.


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 4/4] Cryptomount support for hyphens in UUID

[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


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 1/4] Cryptomount support LUKS detached header

[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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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 c

[PATCH 3/4] Cryptomount support plain dm-crypt

[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, 

Re: [PATCH 3/4] Cryptomount support plain dm-crypt

[John Lane]

A little explanation of what the patch does; most of the code in this
patch already existed.

I extracted the in-line code from "luks.c" that creates the crypto disk
into a new cryptomount function called "grub_cryptodisk_create" that is
then used by the luks module and is also avilable to the cryptomount
module.

I extracted the "set_passphrase" function from the "devmapper.c"
committed (e7f405ab) in the "peter/devmapper" branch so that I could use
it in cryptomount to hash a manually entered passphrase.

I then wrote some additional options and a small block of code to
implement plain mode using the above.





signature.asc
Description: OpenPGP digital signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/4] Cryptomount support key files

[John Lane]

-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  пишет:
>
>> ---
>>  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" too

Re: [PATCH 2/4] Cryptomount support key files

[John Lane]

On 24/06/15 07:59, Andrei Borzenkov wrote:
> On Tue, Jun 23, 2015 at 8:27 PM, John Lane  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.




___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/4] Cryptomount support key files

[John Lane]

On 24/06/15 13:02, Andrei Borzenkov wrote:
> On Wed, Jun 24, 2015 at 2:26 PM, John Lane  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 ;)



___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 1/5] Cryptomount support LUKS detached header

[John Lane]

From: John Lane 

---
 grub-core/disk/cryptodisk.c | 22 ++
 grub-core/disk/geli.c   |  7 +--
 grub-core/disk/luks.c   | 45 +
 include/grub/cryptodisk.h   |  5 +++--
 4 files changed, 63 insertions(+), 16 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 82a3dcb..6f596a0 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,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
argc, char **args)
   if (argc < 1 && !state[1].set && !state[2].set)
 return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
 
+  if (state[3].set) /* LUKS detached header */
+{
+  if (state[0].set) /* Cannot use UUID lookup with detached header */
+return GRUB_ERR_BAD_ARGUMENT;
+
+  hdr = grub_file_open (state[3].arg);
+  if (!hdr)
+return grub_errno;
+}
+  else
+hdr = NULL;
+
   have_it = 0;
   if (state[0].set)
 {
@@ -1125,7 +1139,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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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 (

Cryptomount enhancements - revised

[John Lane]

These patches provide extensions to the "cryptomount" command. There are five 
patches

 [PATCH 1/5] Cryptomount support LUKS detached header
 Support LUKS detached headers so that the header can be separated from the 
data payload, e.g. by storing on external removable media such as a USB key.

 [PATCH 2/5] Cryptomount support key files
 Support key files so that passphrase entry can be suppressed. The passphrase 
can be stored in a "key file" that can be stored, for example, on external 
removable media such as a USB key.

 [PATCH 3/5] cryptomount luks allow multiple passphrase attempts
 Allow a second attempt to enter a passphrase. If unlocking fails on the first 
attempt then the user is presented with the passphrase entry prompt again. If a 
key file is given that does not unlock the device then the user is given the 
opportunity to enter a passphrase.

 [PATCH 4/5] Cryptomount support plain dm-crypt
 Support plain dm-crypt mode. Allow plain volumes to be opened. This is largely 
a re-factoring of exisitng code to allow the crypto routines be used 
independently of LUKS.

 [PATCH 5/5] Cryptomount support for hyphens in UUID
 Support for hyphens in UUID. The "-u" option of cryptomount accepts a UUID. 
This option allows that to be delimited with hyphens so that the same format 
can be given to Grub as is passed to the Linux kernel boot options.


This is a revised patch set following feedback from patches sent on 16/6/15.

 grub-core/disk/cryptodisk.c | 382 
++--
 grub-core/disk/geli.c   |   9 +-
 grub-core/disk/luks.c   | 508 
+
 include/grub/cryptodisk.h   |  18 ++-
 4 files changed, 581 insertions(+), 336 deletions(-)


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 2/5] Cryptomount support key files

[John Lane]

From: John Lane 

---
 grub-core/disk/cryptodisk.c | 46 -
 grub-core/disk/geli.c   |  4 +++-
 grub-core/disk/luks.c   | 44 +--
 include/grub/cryptodisk.h   |  5 -
 4 files changed, 82 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 6f596a0..a27e70c 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,9 @@ static const struct grub_arg_option options[] =
 {"all", 'a', 0, N_("Mount all."), 0, 0},
 {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
 {"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STRING},
+{"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
+{"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
+{"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, 
ARG_TYPE_INT},
 {0, 0, 0, 0, 0, 0}
   };
 
@@ -805,6 +808,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 +840,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);
@@ -943,6 +948,45 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
argc, char **args)
 hdr = NULL;
 
   have_it = 0;
+  key = NULL;
+
+  if (state[4].set) /* Key file; fails back to passphrase entry */
+{
+  grub_file_t keyfile;
+  int keyfile_offset;
+  grub_size_t requested_keyfile_size;
+
+  requested_keyfile_size = state[6].set ? grub_strtoul(state[6].arg, 0, 0) 
: 0;
+
+  if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
+grub_printf (N_("Key file size exceeds maximum (%llu)\n"), \
+(unsigned long long) 
GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
+  else
+{
+  keyfile_offset = state[5].set ? grub_strtoul (state[5].arg, 0, 0) : 
0;
+  keyfile_size = requested_keyfile_size ? requested_keyfile_size : \
+GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
+
+  keyfile = grub_file_open (state[4].arg);
+  if (!keyfile)
+grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
+  else if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
+grub_printf (N_("Unable to seek to offset %d in key file\n"), 
keyfile_offset);
+  else
+{
+  keyfile_size = grub_file_read (keyfile, keyfile_buffer, 
keyfile_size);
+  if (keyfile_size == (grub_size_t)-1)
+ grub_printf (N_("Error reading key file\n"));
+ else if (requested_keyfile_size && (keyfile_size != 
requested_keyfile_size))
+ grub_printf (N_("Cannot read %llu bytes for key file (read 
%llu bytes)\n"),
+(unsigned long long) 
requested_keyfile_size,
+   (unsigned long long) 
keyfile_size);
+  else
+key = keyfile_buffer;
+   }
+}
+}
+
   if (state[0].set)
 {
   grub_cryptodisk_t dev;
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 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..5882368 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -322,12 +322,16 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
 static grub_err_t
 luks_recover_key (grub_disk_t source,
  grub_cryptodisk_t dev,
- grub_file_t hdr)
+ grub_file_t hdr,
+ grub_uint8_t *keyfile_bytes,
+ grub_size_t keyfile_bytes_size)
 {
   struct grub_luks_phdr head

[PATCH 4/5] Cryptomount support plain dm-crypt

[John Lane]

From: John Lane 

---
 grub-core/disk/cryptodisk.c | 298 +++-
 grub-core/disk/luks.c   | 195 +
 include/grub/cryptodisk.h   |   8 ++
 3 files changed, 310 insertions(+), 191 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index a27e70c..cd5cfc9 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -44,6 +44,12 @@ static const struct grub_arg_option options[] =
 {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
 {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
 {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, 
ARG_TYPE_INT},
+{"plain", 'p', 0, N_("Plain (no LUKS header)"), 0, ARG_TYPE_NONE},
+{"cipher", 'c', 0, N_("Plain mode cipher"), 0, ARG_TYPE_STRING},
+{"digest", 'd', 0, N_("Plain mode passphrase digest (hash)"), 0, 
ARG_TYPE_STRING},
+{"offset", 'o', 0, N_("Plain mode data sector offset"), 0, ARG_TYPE_INT},
+{"size", 's', 0, N_("Size of raw device (sectors, defaults to whole 
device)"), 0, ARG_TYPE_INT},
+{"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
 {0, 0, 0, 0, 0, 0}
   };
 
@@ -927,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)
 {
@@ -1046,7 +1094,63 @@ 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[7].set) /* Plain mode */
+{
+  char *cipher;
+  char *mode;
+  char *digest;
+  int offset, size, key_size;
+
+  cipher = grub_strdup (state[8].set ? state[8].arg : 
GRUB_CRYPTODISK_PLAIN_CIPHER);
+  digest = grub_strdup (state[9].set ? state[9].arg : 
GRUB_CRYPTODISK_PLAIN_DIGEST);
+  offset = state[10].set ? grub_strtoul (state[10].arg, 0, 0) : 0;
+  size   = state[11].set ? grub_strtoul (state[11].arg, 0, 0) : 0;
+  key_size = ( state[12].set ? grub_strtoul (state[12].arg, 0, 0) \
+: GRUB_CRYPTODISK_PLAIN_KEYSIZE ) / 8;
+
+  /* no strtok, do it manually */
+  mode = grub_strchr(cipher,'-');
+  if (!mode)
+return GRUB_ERR_BAD_ARGUMENT;
+  else
+*mode++ = 0;
+
+  dev = grub_cryptodisk_create (disk, NULL, cipher, mode, digest);
+
+  dev->offset = offset;
+ if (size) dev->total_length = size;
+
+  if (key)
+   {
+  err = grub_cryptodisk_setkey (dev, key, key_size);
+  if (err)
+return err;
+   }
+ else
+   {
+  char passphrase[GRUB_CRYPTODISK_MAX_PASSPHRASE] = "";
+
+  grub_printf_ (N_("Enter passphrase for %s: "), diskname);
+  if (!grub_password_get (passphrase, 
GRUB_CRYPTODISK_MAX_PASSPHRASE))
+return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not 
supplied");
+
+  err = set_passphrase (dev, key_size, passphrase);
+  if (err)
+{
+  grub_crypto_cipher_close (dev->cipher);
+  return err;
+}
+   }
+
+  grub_cryptodisk_insert (dev, diskname,

[PATCH 3/5] cryptomount luks allow multiple passphrase attempts

[John Lane]

From: John Lane 

---
 grub-core/disk/luks.c | 278 ++
 1 file changed, 143 insertions(+), 135 deletions(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 5882368..11e437e 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -321,10 +321,10 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
 
 static grub_err_t
 luks_recover_key (grub_disk_t source,
- grub_cryptodisk_t dev,
- grub_file_t hdr,
- grub_uint8_t *keyfile_bytes,
- grub_size_t keyfile_bytes_size)
+  grub_cryptodisk_t dev,
+  grub_file_t hdr,
+  grub_uint8_t *keyfile_bytes,
+  grub_size_t keyfile_bytes_size)
 {
   struct grub_luks_phdr header;
   grub_size_t keysize;
@@ -339,6 +339,7 @@ luks_recover_key (grub_disk_t source,
   grub_size_t max_stripes = 1;
   char *tmp;
   grub_uint32_t sector;
+  unsigned attempts = 2;
 
   err = GRUB_ERR_NONE;
 
@@ -361,151 +362,158 @@ luks_recover_key (grub_disk_t source,
 
   for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
 if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED
-   && grub_be_to_cpu32 (header.keyblock[i].stripes) > max_stripes)
+&& grub_be_to_cpu32 (header.keyblock[i].stripes) > max_stripes)
   max_stripes = grub_be_to_cpu32 (header.keyblock[i].stripes);
 
   split_key = grub_malloc (keysize * max_stripes);
   if (!split_key)
 return grub_errno;
 
-  if (keyfile_bytes)
+  while (attempts)
 {
-  /* Use bytestring from key file as passphrase */
-  passphrase = keyfile_bytes;
-  passphrase_length = keyfile_bytes_size;
-}
-  else
-{
-  /* Get the passphrase from the user.  */
-  tmp = NULL;
-  if (source->partition)
-tmp = grub_partition_get_name (source->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-   source->partition ? "," : "", tmp ? : "", dev->uuid);
-  grub_free (tmp);
-  if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
+  if (keyfile_bytes)
 {
-  grub_free (split_key);
-  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
-}
-
-  passphrase = (grub_uint8_t *)interactive_passphrase;
-  passphrase_length = grub_strlen (interactive_passphrase);
-
-}
-
-  /* Try to recover master key from each active keyslot.  */
-  for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
-{
-  gcry_err_code_t gcry_err;
-  grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
-  grub_uint8_t digest[GRUB_CRYPTODISK_MAX_KEYLEN];
-
-  /* Check if keyslot is enabled.  */
-  if (grub_be_to_cpu32 (header.keyblock[i].active) != LUKS_KEY_ENABLED)
-   continue;
-
-  grub_dprintf ("luks", "Trying keyslot %d\n", i);
-
-  /* Calculate the PBKDF2 of the user supplied passphrase.  */
-  gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
-passphrase_length,
-header.keyblock[i].passwordSalt,
-sizeof (header.keyblock[i].passwordSalt),
-grub_be_to_cpu32 (header.keyblock[i].
-  passwordIterations),
-digest, keysize);
-
-  if (gcry_err)
-   {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
-   }
-
-  grub_dprintf ("luks", "PBKDF2 done\n");
-
-  gcry_err = grub_cryptodisk_setkey (dev, digest, keysize); 
-  if (gcry_err)
-   {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
-   }
-
-  sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
-  length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
-
-  /* Read and decrypt the key material from the disk.  */
-  if (hdr)
-{
- grub_file_seek (hdr, sector * 512);
-  if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
-err = GRUB_ERR_READ_ERROR;
+  /* Use bytestring from key file as passphrase */
+  passphrase = keyfile_bytes;
+  passphrase_length = keyfile_bytes_size;
+ keyfile_bytes = NULL; /* use it only once */
 }
   else
-err = grub_disk_read (source, sector, 0, length, split_key);
-  if (err)
-   {
- grub_free (split_key);
- return err;
-   }
-
-  gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
-  if (gcry_err)
-   {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
-   

[PATCH 5/5] Cryptomount support for hyphens in UUID

[John Lane]

From: John Lane 

---
 grub-core/disk/cryptodisk.c | 20 +---
 grub-core/disk/luks.c   | 26 --
 include/grub/cryptodisk.h   |  2 ++
 3 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index cd5cfc9..d36d16b 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -113,6 +113,20 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const 
grub_uint8_t *b)
 }
 }
 
+int
+grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b)
+{
+  while ((*uuid_a != '\0') && (*uuid_b != '\0'))
+{
+  while (*uuid_a == '-') uuid_a++;
+  while (*uuid_b == '-') uuid_b++;
+  if (grub_toupper(*uuid_a) != grub_toupper(*uuid_b)) break;
+  uuid_a++;
+  uuid_b++;
+}
+  return (*uuid_a == '\0') && (*uuid_b == '\0');
+}
+
 static gcry_err_code_t
 grub_crypto_pcbc_decrypt (grub_crypto_cipher_handle_t cipher,
 void *out, void *in, grub_size_t size,
@@ -507,8 +521,8 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
   if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
 {
   for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-   if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
- break;
+if (grub_cryptodisk_uuidcmp(name + sizeof ("cryptouuid/") - 1, 
dev->uuid))
+  break;
 }
   else
 {
@@ -739,7 +753,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
 {
   grub_cryptodisk_t dev;
   for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-if (grub_strcasecmp (dev->uuid, uuid) == 0)
+if (grub_cryptodisk_uuidcmp(dev->uuid, uuid))
   return dev;
   return NULL;
 }
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 4ebe21b..80a7606 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -68,9 +68,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   int check_boot, grub_file_t hdr)
 {
   grub_cryptodisk_t newdev;
-  const char *iptr;
   struct grub_luks_phdr header;
-  char *optr;
   char uuid[sizeof (header.uuid) + 1];
   char ciphername[sizeof (header.cipherName) + 1];
   char ciphermode[sizeof (header.cipherMode) + 1];
@@ -104,22 +102,6 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
   || grub_be_to_cpu16 (header.version) != 1)
 return NULL;
 
-  optr = uuid;
-  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
-   iptr++)
-{
-  if (*iptr != '-')
-*optr++ = *iptr;
-}
-  *optr = 0;
-
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-{
-  grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
-  return NULL;
-}
-
-
   /* Make sure that strings are null terminated.  */
   grub_memcpy (ciphername, header.cipherName, sizeof (header.cipherName));
   ciphername[sizeof (header.cipherName)] = 0;
@@ -127,6 +109,14 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
   ciphermode[sizeof (header.cipherMode)] = 0;
   grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
   hashspec[sizeof (header.hashSpec)] = 0;
+  grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
+  uuid[sizeof (header.uuid)] = 0;
+
+  if ( check_uuid && ! grub_cryptodisk_uuidcmp(check_uuid, uuid))
+{
+  grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
+  return NULL;
+}
 
   newdev = grub_cryptodisk_create (disk, uuid, ciphername, ciphermode, 
hashspec);
 
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 4076412..a564f2c 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -167,4 +167,6 @@ grub_cryptodisk_t grub_cryptodisk_get_by_source_disk 
(grub_disk_t disk);
 grub_cryptodisk_t grub_cryptodisk_create (grub_disk_t disk, char *uuid,
   char *ciphername, char *ciphermode, char 
*digest);
 
+int
+grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b);
 #endif
-- 
2.1.2


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 5/5] Cryptomount support for hyphens in UUID

[John Lane]

This revised patch uses a compare function. It doesn't modify the input
supplied by the user.

There are 3 places where UUIDs are compared:
1. when a crypto disk is referenced as a (cryptouuid/...) #cryotodisk.c
grub_cryptodisk_open
2. when the -u argument is given to cryptomount it checks by the uuid
that the device isn't already mounted #cryptodisk.c
grub_cryptodisk_get_by_uuid
3. when locating a device by uuid it compares the given uuid with the
LUKS header #luks.c configure_ciphers






___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/5] cryptomount luks allow multiple passphrase attempts

[John Lane]

When opening a LUKS volume the user has 2 chances to supply a correct
passphrase.

if the first attempt fails then the passphrase prompt appears again.
if keyfile given then the first attempt is the uses that instead of
prompting; failure will then prompt the user for a passphrase.

This does not apply to plain mode because there is no way to know that a
key is incorrect in plain mode. Whatever key or passphrase is given will
succeed (but the unlocked data may appear as garbage, of course.).
That's inherent in the implementation of plain mode.


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Help output inconsistency: -v vs. -V

[John Lane]

My natural inclination is to reach for "-v" when I want verbose output
from something and fall back to --verbose if that doesn't work. I
usually go straight for --version when wanting that info. So, "-V
for--version and -v for --verbose" works for me.

On 29/06/15 18:50, Andrei Borzenkov wrote:
> We currently have situation that C converted tools use -V for
> --version and -v for --verbose while shell based tools (quite a lot
> left) use -v for --version and most do not have --verbose at all.
>
> -V is quite deep in argp, but of course it is possible to get rid of it
> by basically replicating argp code in grub (OK, we do not need check
> for any hooks as we know there are none). 
>
> It's not really new, and had been this way for some time, just that in
> the past C programs were considered "for internal use only" and now
> they are user facing so it is incompatible change.
>
> Just gathering opinions what would be preferable. 
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>



___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Cryptomount enhancements - revised

[John Lane]

On 29/06/15 15:30, John Lane wrote:
> These patches provide extensions to the "cryptomount" command. There are five 
> patches
>
>  [PATCH 1/5] Cryptomount support LUKS detached header
>  Support LUKS detached headers so that the header can be separated from the 
> data payload, e.g. by storing on external removable media such as a USB key.
>
>  [PATCH 2/5] Cryptomount support key files
>  Support key files so that passphrase entry can be suppressed. The passphrase 
> can be stored in a "key file" that can be stored, for example, on external 
> removable media such as a USB key.
>
>  [PATCH 3/5] cryptomount luks allow multiple passphrase attempts
>  Allow a second attempt to enter a passphrase. If unlocking fails on the 
> first attempt then the user is presented with the passphrase entry prompt 
> again. If a key file is given that does not unlock the device then the user 
> is given the opportunity to enter a passphrase.
>
>  [PATCH 4/5] Cryptomount support plain dm-crypt
>  Support plain dm-crypt mode. Allow plain volumes to be opened. This is 
> largely a re-factoring of exisitng code to allow the crypto routines be used 
> independently of LUKS.
>
>  [PATCH 5/5] Cryptomount support for hyphens in UUID
>  Support for hyphens in UUID. The "-u" option of cryptomount accepts a UUID. 
> This option allows that to be delimited with hyphens so that the same format 
> can be given to Grub as is passed to the Linux kernel boot options.
>
>
> This is a revised patch set following feedback from patches sent on 16/6/15.
>
>  grub-core/disk/cryptodisk.c | 382 
> ++--
>  grub-core/disk/geli.c   |   9 +-
>  grub-core/disk/luks.c   | 508 
> +
>  include/grub/cryptodisk.h   |  18 ++-
>  4 files changed, 581 insertions(+), 336 deletions(-)
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
Just wondering if there's any feedback on these patches... It's been a
while since I posted them.


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Cryptomount enhancements - revised

[John Lane]

On 28/07/15 22:38, Vladimir 'phcoder' Serbinenko wrote:
>
> Other than 3 and 5 they require difficult configuration. Mapping
> devices in GRUB isn't trivial. Those features are difficult to
> autoconfigure. Consider "plain" mode: how will you find which disk is
> yours when you have 5 disks all looking as random data?
>
>
I don't see what's difficult about providing a LUKs header and key but I
am aware of the issue re device identification in plain mode. However,
if one has a use-case for these crypto routines then I think that would
be a valid use-case for manually configuring grub.cfg if it's beyond
what autoconfiguration supports. If an end user wants to make the choice
then why deny him, just because it may be difficult to autoconfigure ?

There does seem to be interest in this functionality. Surely
auto-configuration would't be a bar to supporting this? I don't think I
am the only one who thinks these features are useful...

Regarding device identification, I had some thoughts on that and was
willing to try implementing something. However I wanted to put this
patch-set to bed before starting on something else.


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 5/5] Cryptomount support for hyphens in UUID

[John Lane]

On 29/07/15 04:08, Andrei Borzenkov wrote:
> I still believe that generally ignoring hyphens for every future crypto
> implementation is wrong. In future we simply should avoid mangling
> UUID. So this should be restricted to LUKS only, where the problem
> exists.
>
>
Andrei I modified this in response to your earlier feedback so that it
doesn't modify the user-supplied data and doesn't have any effect beyond
the three places I mentioned. These have no negative impact on uuid
values used elesewere. If you don't want to support this fair enough -
this can be dropped without impacting the other patches.


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 5/5] Cryptomount support for hyphens in UUID

[John Lane]

On 29/07/15 17:51, Andrei Borzenkov wrote:
>
> That not what I mean. This patch ignores hyphens in UUID for any
> current and future crypto backends. This means that it cannot
> distinguish between 11-122 and 111-22 as UUID. And we cannot be sure we
> never meet such backend. 
do you really think that's likely?
> And GELI as far as I can tell does not
> actually use hyphens.
>
> We already made mistake of mangling native UUID, I'd rather avoid
> continue to do it.
>
> Can we restrict this to LUKS only? 
One of the three places that UUIDs are compared is in the LUKS module.
The other two are in the generic cryptodisk module, one when the "-u"
argument is given (which I believe is LUKS-specific although not in the
LUKS module) and the other when a crypto disk is referenced as a
(cryptouuid/...) which is generic code, not LUKS-specific. Making it
LUKS-specific would mean deleting the third case, which is actually the
most useful IMHO.

Unless we could identify a LUKS disk from within a grub_cryptodisk_t but
I don't think that's possible ?
If it were, we could make the "uuidcmp" function accept an
"ignore-hyphens" parameter that is true for LUKS and false for
everything else.

But it all seems to be getting overcomplicated for what was meant to be
a simple tweak.




___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Cryptomount enhancements - revised

[John Lane]

On 29/07/15 18:21, Andrei Borzenkov wrote:
> В Wed, 29 Jul 2015 07:48:41 +0100
> John Lane  пишет:
>
>> On 28/07/15 22:38, Vladimir 'phcoder' Serbinenko wrote:
>>> Other than 3 and 5 they require difficult configuration. Mapping
>>> devices in GRUB isn't trivial. Those features are difficult to
>>> autoconfigure. Consider "plain" mode: how will you find which disk is
>>> yours when you have 5 disks all looking as random data?
>>>
>>>
>> I don't see what's difficult about providing a LUKs header and key but I
>> am aware of the issue re device identification in plain mode. However,
>> if one has a use-case for these crypto routines then I think that would
>> be a valid use-case for manually configuring grub.cfg if it's beyond
>> what autoconfiguration supports. If an end user wants to make the choice
>> then why deny him, just because it may be difficult to autoconfigure ?
>>
> Yes, it appears people ask for it. At the end, the worst that can
> happen is reading garbage.
>
>> There does seem to be interest in this functionality. Surely
>> auto-configuration would't be a bar to supporting this? I don't think I
>> am the only one who thinks these features are useful...
>>
>> Regarding device identification, I had some thoughts on that and was
>> willing to try implementing something. However I wanted to put this
>> patch-set to bed before starting on something else.
>>
> One think I'd like is to separate self-identified containers managed by
> cryptomount and dmsetup-like stuff to avoid impression that it is fully
> supported.
>
I'm unclear on what the next step is, having responded to feedback and
made changes to address the issues previously raised. Is anything
outstanding that absolutely has to happen before these patches can be
accepted?

Ideally I'd prefer to wrap up this set of changes up before thinking
about other features.


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Support for detached LUKS header

[John Lane]

> Could anyone give their approval for merging?

> May I ask you or the original author to rebase the patches
> on the latest master? Then I will take a look. And please
> do not forget to CC me.

> Daniel

Daniel I am the original author of the aforementioned patches.

I have reapplied my patches to the current master head
(28b0d19061d66e3633148ac8e44decda914bf266).

I had to make a slight tweak to patch 4 to change one line of context
surrounding one hunk of the patch, but I didn't need to change any of
the patch code.

There are also two new patches with changes received from contributors'
pull requests.

I will forward the patches shortly.

My patches apply cleanly to the current head.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 6/7] Retain constness of parameters.

[John Lane]

From: Denis Kasak 

---
 grub-core/disk/cryptodisk.c | 2 +-
 include/grub/cryptodisk.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index c442d3a34..6fc2c23aa 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -115,7 +115,7 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const 
grub_uint8_t *b)
 }
 
 int
-grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b)
+grub_cryptodisk_uuidcmp(const char *uuid_a, const char *uuid_b)
 {
   while ((*uuid_a != '\0') && (*uuid_b != '\0'))
 {
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 01c02696e..cd6a54582 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -169,5 +169,5 @@ grub_cryptodisk_t grub_cryptodisk_create (grub_disk_t disk, 
char *uuid,
   char *ciphername, char *ciphermode, char 
*digest);
 
 int
-grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b);
+grub_cryptodisk_uuidcmp(const char *uuid_a, const char *uuid_b);
 #endif
-- 
2.16.2


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 2/7] Cryptomount support key files

[John Lane]

From: John Lane 

---
 grub-core/disk/cryptodisk.c | 46 -
 grub-core/disk/geli.c   |  4 +++-
 grub-core/disk/luks.c   | 44 +--
 include/grub/cryptodisk.h   |  5 -
 4 files changed, 82 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 5230a5a9a..5261af547 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -42,6 +42,9 @@ static const struct grub_arg_option options[] =
 {"all", 'a', 0, N_("Mount all."), 0, 0},
 {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
 {"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STRING},
+{"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
+{"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
+{"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, 
ARG_TYPE_INT},
 {0, 0, 0, 0, 0, 0}
   };
 
@@ -811,6 +814,8 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
 static int check_boot, have_it;
 static char *search_uuid;
 static grub_file_t hdr;
+static grub_uint8_t *key, keyfile_buffer[GRUB_CRYPTODISK_MAX_KEYFILE_SIZE];
+static grub_size_t keyfile_size;
 
 static void
 cryptodisk_close (grub_cryptodisk_t dev)
@@ -841,7 +846,7 @@ grub_cryptodisk_scan_device_real (const char *name, 
grub_disk_t source)
 if (!dev)
   continue;
 
-err = cr->recover_key (source, dev, hdr);
+err = cr->recover_key (source, dev, hdr, key, keyfile_size);
 if (err)
 {
   cryptodisk_close (dev);
@@ -949,6 +954,45 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
argc, char **args)
 hdr = NULL;
 
   have_it = 0;
+  key = NULL;
+
+  if (state[4].set) /* Key file; fails back to passphrase entry */
+{
+  grub_file_t keyfile;
+  int keyfile_offset;
+  grub_size_t requested_keyfile_size;
+
+  requested_keyfile_size = state[6].set ? grub_strtoul(state[6].arg, 0, 0) 
: 0;
+
+  if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
+grub_printf (N_("Key file size exceeds maximum (%llu)\n"), \
+(unsigned long long) 
GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
+  else
+{
+  keyfile_offset = state[5].set ? grub_strtoul (state[5].arg, 0, 0) : 
0;
+  keyfile_size = requested_keyfile_size ? requested_keyfile_size : \
+GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
+
+  keyfile = grub_file_open (state[4].arg);
+  if (!keyfile)
+grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
+  else if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
+grub_printf (N_("Unable to seek to offset %d in key file\n"), 
keyfile_offset);
+  else
+{
+  keyfile_size = grub_file_read (keyfile, keyfile_buffer, 
keyfile_size);
+  if (keyfile_size == (grub_size_t)-1)
+ grub_printf (N_("Error reading key file\n"));
+ else if (requested_keyfile_size && (keyfile_size != 
requested_keyfile_size))
+ grub_printf (N_("Cannot read %llu bytes for key file (read 
%llu bytes)\n"),
+(unsigned long long) 
requested_keyfile_size,
+   (unsigned long long) 
keyfile_size);
+  else
+key = keyfile_buffer;
+   }
+}
+}
+
   if (state[0].set)
 {
   grub_cryptodisk_t dev;
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index f4394eb42..da6aa6a63 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -401,7 +401,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
 
 static grub_err_t
 recover_key (grub_disk_t source, grub_cryptodisk_t dev,
-grub_file_t hdr __attribute__ ((unused)) )
+grub_file_t hdr __attribute__ ((unused)),
+grub_uint8_t *key __attribute__ ((unused)),
+grub_size_t keyfile_size __attribute__ ((unused)) )
 {
   grub_size_t keysize;
   grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 66e64c0e0..588236888 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -322,12 +322,16 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
 static grub_err_t
 luks_recover_key (grub_disk_t source,
  grub_cryptodisk_t dev,
- grub_file_t hdr)
+ grub_file_t hdr,
+ grub_uint8_t *keyfile_bytes,
+ grub_size_t keyfile_bytes_size)
 {
   struct gr

[PATCH 5/7] Cryptomount support for hyphens in UUID

[John Lane]

From: John Lane 

---
 grub-core/disk/cryptodisk.c | 20 +---
 grub-core/disk/luks.c   | 26 --
 include/grub/cryptodisk.h   |  2 ++
 3 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 7f656f75c..c442d3a34 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -114,6 +114,20 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const 
grub_uint8_t *b)
 }
 }
 
+int
+grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b)
+{
+  while ((*uuid_a != '\0') && (*uuid_b != '\0'))
+{
+  while (*uuid_a == '-') uuid_a++;
+  while (*uuid_b == '-') uuid_b++;
+  if (grub_toupper(*uuid_a) != grub_toupper(*uuid_b)) break;
+  uuid_a++;
+  uuid_b++;
+}
+  return (*uuid_a == '\0') && (*uuid_b == '\0');
+}
+
 static gcry_err_code_t
 grub_crypto_pcbc_decrypt (grub_crypto_cipher_handle_t cipher,
 void *out, void *in, grub_size_t size,
@@ -509,8 +523,8 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
   if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
 {
   for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-   if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
- break;
+if (grub_cryptodisk_uuidcmp(name + sizeof ("cryptouuid/") - 1, 
dev->uuid))
+  break;
 }
   else
 {
@@ -742,7 +756,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
 {
   grub_cryptodisk_t dev;
   for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-if (grub_strcasecmp (dev->uuid, uuid) == 0)
+if (grub_cryptodisk_uuidcmp(dev->uuid, uuid))
   return dev;
   return NULL;
 }
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 4ebe21b4e..80a760670 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -68,9 +68,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   int check_boot, grub_file_t hdr)
 {
   grub_cryptodisk_t newdev;
-  const char *iptr;
   struct grub_luks_phdr header;
-  char *optr;
   char uuid[sizeof (header.uuid) + 1];
   char ciphername[sizeof (header.cipherName) + 1];
   char ciphermode[sizeof (header.cipherMode) + 1];
@@ -104,22 +102,6 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
   || grub_be_to_cpu16 (header.version) != 1)
 return NULL;
 
-  optr = uuid;
-  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
-   iptr++)
-{
-  if (*iptr != '-')
-*optr++ = *iptr;
-}
-  *optr = 0;
-
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-{
-  grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
-  return NULL;
-}
-
-
   /* Make sure that strings are null terminated.  */
   grub_memcpy (ciphername, header.cipherName, sizeof (header.cipherName));
   ciphername[sizeof (header.cipherName)] = 0;
@@ -127,6 +109,14 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
   ciphermode[sizeof (header.cipherMode)] = 0;
   grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
   hashspec[sizeof (header.hashSpec)] = 0;
+  grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
+  uuid[sizeof (header.uuid)] = 0;
+
+  if ( check_uuid && ! grub_cryptodisk_uuidcmp(check_uuid, uuid))
+{
+  grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
+  return NULL;
+}
 
   newdev = grub_cryptodisk_create (disk, uuid, ciphername, ciphermode, 
hashspec);
 
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index bb25ab730..01c02696e 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -168,4 +168,6 @@ grub_cryptodisk_t grub_cryptodisk_get_by_source_disk 
(grub_disk_t disk);
 grub_cryptodisk_t grub_cryptodisk_create (grub_disk_t disk, char *uuid,
   char *ciphername, char *ciphermode, char 
*digest);
 
+int
+grub_cryptodisk_uuidcmp(char *uuid_a, char *uuid_b);
 #endif
-- 
2.16.2


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 1/7] Cryptomount support LUKS detached header

[John Lane]

From: John Lane 

---
 grub-core/disk/cryptodisk.c | 22 ++
 grub-core/disk/geli.c   |  7 +--
 grub-core/disk/luks.c   | 45 +
 include/grub/cryptodisk.h   |  5 +++--
 4 files changed, 63 insertions(+), 16 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index bd60a66b3..5230a5a9a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,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}
   };
 
@@ -809,6 +810,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)
@@ -833,13 +835,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);
@@ -880,7 +882,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const 
char *cheat)
 
   FOR_CRYPTODISK_DEVS (cr)
   {
-dev = cr->scan (source, search_uuid, check_boot);
+dev = cr->scan (source, search_uuid, check_boot,0);
 if (grub_errno)
   return grub_errno;
 if (!dev)
@@ -934,6 +936,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
argc, char **args)
   if (argc < 1 && !state[1].set && !state[2].set)
 return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
 
+  if (state[3].set) /* LUKS detached header */
+{
+  if (state[0].set) /* Cannot use UUID lookup with detached header */
+return GRUB_ERR_BAD_ARGUMENT;
+
+  hdr = grub_file_open (state[3].arg);
+  if (!hdr)
+return grub_errno;
+}
+  else
+hdr = NULL;
+
   have_it = 0;
   if (state[0].set)
 {
@@ -1141,7 +1155,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 e9d23299a..f4394eb42 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -52,6 +52,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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 86c50c612..66e64c0e0 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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 = gr

[PATCH 3/7] cryptomount luks allow multiple passphrase attempts

[John Lane]

From: John Lane 

---
 grub-core/disk/luks.c | 278 ++
 1 file changed, 143 insertions(+), 135 deletions(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 588236888..11e437edb 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -321,10 +321,10 @@ configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
 
 static grub_err_t
 luks_recover_key (grub_disk_t source,
- grub_cryptodisk_t dev,
- grub_file_t hdr,
- grub_uint8_t *keyfile_bytes,
- grub_size_t keyfile_bytes_size)
+  grub_cryptodisk_t dev,
+  grub_file_t hdr,
+  grub_uint8_t *keyfile_bytes,
+  grub_size_t keyfile_bytes_size)
 {
   struct grub_luks_phdr header;
   grub_size_t keysize;
@@ -339,6 +339,7 @@ luks_recover_key (grub_disk_t source,
   grub_size_t max_stripes = 1;
   char *tmp;
   grub_uint32_t sector;
+  unsigned attempts = 2;
 
   err = GRUB_ERR_NONE;
 
@@ -361,151 +362,158 @@ luks_recover_key (grub_disk_t source,
 
   for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
 if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED
-   && grub_be_to_cpu32 (header.keyblock[i].stripes) > max_stripes)
+&& grub_be_to_cpu32 (header.keyblock[i].stripes) > max_stripes)
   max_stripes = grub_be_to_cpu32 (header.keyblock[i].stripes);
 
   split_key = grub_malloc (keysize * max_stripes);
   if (!split_key)
 return grub_errno;
 
-  if (keyfile_bytes)
+  while (attempts)
 {
-  /* Use bytestring from key file as passphrase */
-  passphrase = keyfile_bytes;
-  passphrase_length = keyfile_bytes_size;
-}
-  else
-{
-  /* Get the passphrase from the user.  */
-  tmp = NULL;
-  if (source->partition)
-tmp = grub_partition_get_name (source->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-   source->partition ? "," : "", tmp ? : "", dev->uuid);
-  grub_free (tmp);
-  if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
+  if (keyfile_bytes)
 {
-  grub_free (split_key);
-  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
-}
-
-  passphrase = (grub_uint8_t *)interactive_passphrase;
-  passphrase_length = grub_strlen (interactive_passphrase);
-
-}
-
-  /* Try to recover master key from each active keyslot.  */
-  for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
-{
-  gcry_err_code_t gcry_err;
-  grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
-  grub_uint8_t digest[GRUB_CRYPTODISK_MAX_KEYLEN];
-
-  /* Check if keyslot is enabled.  */
-  if (grub_be_to_cpu32 (header.keyblock[i].active) != LUKS_KEY_ENABLED)
-   continue;
-
-  grub_dprintf ("luks", "Trying keyslot %d\n", i);
-
-  /* Calculate the PBKDF2 of the user supplied passphrase.  */
-  gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
-passphrase_length,
-header.keyblock[i].passwordSalt,
-sizeof (header.keyblock[i].passwordSalt),
-grub_be_to_cpu32 (header.keyblock[i].
-  passwordIterations),
-digest, keysize);
-
-  if (gcry_err)
-   {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
-   }
-
-  grub_dprintf ("luks", "PBKDF2 done\n");
-
-  gcry_err = grub_cryptodisk_setkey (dev, digest, keysize); 
-  if (gcry_err)
-   {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
-   }
-
-  sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
-  length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
-
-  /* Read and decrypt the key material from the disk.  */
-  if (hdr)
-{
- grub_file_seek (hdr, sector * 512);
-  if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
-err = GRUB_ERR_READ_ERROR;
+  /* Use bytestring from key file as passphrase */
+  passphrase = keyfile_bytes;
+  passphrase_length = keyfile_bytes_size;
+ keyfile_bytes = NULL; /* use it only once */
 }
   else
-err = grub_disk_read (source, sector, 0, length, split_key);
-  if (err)
-   {
- grub_free (split_key);
- return err;
-   }
-
-  gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
-  if (gcry_err)
-   {
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
-   

[PATCH 4/7] Cryptomount support plain dm-crypt

[John Lane]

From: John Lane 

Patch modified to take into account a change to context
brought about by c93d3e694713b8230fa2cf88414fabe005b56782

grub-core/disk/cryptodisk.c
142c142
<if (disklast)
---
>
---
 grub-core/disk/cryptodisk.c | 298 +++-
 grub-core/disk/luks.c   | 195 +
 include/grub/cryptodisk.h   |   8 ++
 3 files changed, 310 insertions(+), 191 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 5261af547..7f656f75c 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -45,6 +45,12 @@ static const struct grub_arg_option options[] =
 {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
 {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
 {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, 
ARG_TYPE_INT},
+{"plain", 'p', 0, N_("Plain (no LUKS header)"), 0, ARG_TYPE_NONE},
+{"cipher", 'c', 0, N_("Plain mode cipher"), 0, ARG_TYPE_STRING},
+{"digest", 'd', 0, N_("Plain mode passphrase digest (hash)"), 0, 
ARG_TYPE_STRING},
+{"offset", 'o', 0, N_("Plain mode data sector offset"), 0, ARG_TYPE_INT},
+{"size", 's', 0, N_("Size of raw device (sectors, defaults to whole 
device)"), 0, ARG_TYPE_INT},
+{"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
 {0, 0, 0, 0, 0, 0}
   };
 
@@ -933,6 +939,48 @@ grub_cryptodisk_scan_device (const char *name,
   return have_it && search_uuid ? 1 : 0;
 }
 
+/* Hashes a passphrase into a key and stores it with cipher. */
+static gcry_err_code_t
+set_passphrase (grub_cryptodisk_t dev, grub_size_t keysize, const char 
*passphrase)
+{
+  grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = 
derived_hash;
+  char *p;
+  unsigned int round, i;
+  unsigned int len, size;
+
+  /* Need no passphrase if there's no key */
+  if (keysize == 0)
+return GPG_ERR_INV_KEYLEN;
+
+  /* Hack to support the "none" hash */
+  if (dev->hash)
+len = dev->hash->mdlen;
+  else
+len = grub_strlen (passphrase);
+
+  if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN || len > GRUB_CRYPTODISK_MAX_KEYLEN)
+return GPG_ERR_INV_KEYLEN;
+
+  p = grub_malloc (grub_strlen (passphrase) + 2 + keysize / len);
+  if (!p)
+return grub_errno;
+
+  for (round = 0, size = keysize; size; round++, dh += len, size -= len)
+{
+  for (i = 0; i < round; i++)
+   p[i] = 'A';
+
+  grub_strcpy (p + i, passphrase);
+
+  if (len > size)
+   len = size;
+
+  grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
+}
+
+  return grub_cryptodisk_setkey (dev, derived_hash, keysize);
+}
+
 static grub_err_t
 grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 {
@@ -1060,7 +1108,63 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
argc, char **args)
  return GRUB_ERR_NONE;
}
 
-  err = grub_cryptodisk_scan_device_real (diskname, disk);
+  if (state[7].set) /* Plain mode */
+{
+  char *cipher;
+  char *mode;
+  char *digest;
+  int offset, size, key_size;
+
+  cipher = grub_strdup (state[8].set ? state[8].arg : 
GRUB_CRYPTODISK_PLAIN_CIPHER);
+  digest = grub_strdup (state[9].set ? state[9].arg : 
GRUB_CRYPTODISK_PLAIN_DIGEST);
+  offset = state[10].set ? grub_strtoul (state[10].arg, 0, 0) : 0;
+  size   = state[11].set ? grub_strtoul (state[11].arg, 0, 0) : 0;
+  key_size = ( state[12].set ? grub_strtoul (state[12].arg, 0, 0) \
+: GRUB_CRYPTODISK_PLAIN_KEYSIZE ) / 8;
+
+  /* no strtok, do it manually */
+  mode = grub_strchr(cipher,'-');
+  if (!mode)
+return GRUB_ERR_BAD_ARGUMENT;
+  else
+*mode++ = 0;
+
+  dev = grub_cryptodisk_create (disk, NULL, cipher, mode, digest);
+
+  dev->offset = offset;
+ if (size) dev->total_length = size;
+
+  if (key)
+   {
+  err = grub_cryptodisk_setkey (dev, key, key_size);
+  if (err)
+return err;
+   }
+ else
+   {
+  char passphrase[GRUB_CRYPTODISK_MAX_PASSPHRASE] = "";
+
+  grub_printf_ (N_("Enter passphrase for %s: "), diskname);
+  if (!grub_password_get (passphrase, 
GRUB_CRYPTODISK_MAX_PASSPHRASE))
+return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not 
supplied");
+
+  err = set_passphrase (dev, key_size, passphrase);
+  if (err)
+ 

[PATCH 7/7] Add support for using a whole device as a keyfile

[John Lane]

From: Paul Gideon Dann 

---
 grub-core/disk/cryptodisk.c | 86 +++--
 1 file changed, 68 insertions(+), 18 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 6fc2c23aa..a8937e5e3 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1032,26 +1032,76 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
argc, char **args)
   else
 {
   keyfile_offset = state[5].set ? grub_strtoul (state[5].arg, 0, 0) : 
0;
-  keyfile_size = requested_keyfile_size ? requested_keyfile_size : \
-GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
-
-  keyfile = grub_file_open (state[4].arg);
-  if (!keyfile)
-grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
-  else if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
-grub_printf (N_("Unable to seek to offset %d in key file\n"), 
keyfile_offset);
-  else
+
+  if (grub_strchr (state[4].arg, '/'))
 {
-  keyfile_size = grub_file_read (keyfile, keyfile_buffer, 
keyfile_size);
-  if (keyfile_size == (grub_size_t)-1)
- grub_printf (N_("Error reading key file\n"));
- else if (requested_keyfile_size && (keyfile_size != 
requested_keyfile_size))
- grub_printf (N_("Cannot read %llu bytes for key file (read 
%llu bytes)\n"),
-(unsigned long long) 
requested_keyfile_size,
-   (unsigned long long) 
keyfile_size);
+  keyfile_size = requested_keyfile_size ? requested_keyfile_size : 
\
+ 
GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
+  keyfile = grub_file_open (state[4].arg);
+  if (!keyfile)
+grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
+  else if (grub_file_seek (keyfile, keyfile_offset) == 
(grub_off_t)-1)
+grub_printf (N_("Unable to seek to offset %d in key file\n"), 
keyfile_offset);
   else
-key = keyfile_buffer;
-   }
+{
+  keyfile_size = grub_file_read (keyfile, keyfile_buffer, 
keyfile_size);
+  if (keyfile_size == (grub_size_t)-1)
+ grub_printf (N_("Error reading key file\n"));
+  else if (requested_keyfile_size && (keyfile_size != 
requested_keyfile_size))
+ grub_printf (N_("Cannot read %llu bytes for key file 
(read %llu bytes)\n"),
+(unsigned long long) 
requested_keyfile_size,
+(unsigned long long) 
keyfile_size);
+  else
+key = keyfile_buffer;
+}
+}
+  else
+{
+  grub_disk_t keydisk;
+  char* keydisk_name;
+  grub_err_t err;
+  grub_uint64_t total_sectors;
+
+  keydisk_name = grub_file_get_device_name(state[4].arg);
+  keydisk = grub_disk_open (keydisk_name);
+  if (!keydisk)
+{
+  grub_printf (N_("Unable to open disk %s\n"), keydisk_name);
+  goto cleanup_keydisk_name;
+}
+
+  total_sectors = grub_disk_get_size (keydisk);
+  if (total_sectors == GRUB_DISK_SIZE_UNKNOWN)
+{
+  grub_printf (N_("Unable to determine size of disk %s\n"), 
keydisk_name);
+  goto cleanup_keydisk;
+}
+
+  keyfile_size = (total_sectors << GRUB_DISK_SECTOR_BITS);
+  if (requested_keyfile_size > 0 && requested_keyfile_size < 
keyfile_size)
+keyfile_size = requested_keyfile_size;
+  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
+{
+  grub_printf (N_("Key file size exceeds maximum (%llu)\n"), \
+   (unsigned long long) 
GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
+  goto cleanup_keydisk;
+}
+
+  err = grub_disk_read (keydisk, 0, keyfile_offset, keyfile_size, 
keyfile_buffer);
+  if (err != GRUB_ERR_NONE)
+{
+  grub_printf (N_("Failed to read from disk %s\n"), 
keydisk_name);
+  keyfile_size = 0;
+  goto cleanup_keydisk;
+}
+
+  key = keyfile_buffer;
+
+  cleanup_keydisk:
+  grub_disk_close (keydisk);
+  cleanup_keydisk_name:
+  grub_free (keydisk_name);
+}
 }
 }
 
-- 
2.16.2


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.

Re: [PATCH 1/7] Cryptomount support LUKS detached header

[John Lane]

On 14/03/18 13:05, Daniel Kiper wrote:
> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
>> From: John Lane 
> 
> I have just skimmed through the series. First of all, most if not
> all patches beg for full blown commit messages. Just vague statements
> in the subject are insufficient for me. And please add patch 0 which
> introduces the series. git send-email --compose is your friend.
> 
> Daniel
> 

Sorry Daniel, this isn't something I do that often - submitting patches
to ML. Do you want me to resubmit them again, or is the below ok for you ?



These patches provide extensions to the "cryptomount" command. There are
my original five patches, plus an additional two patches from other
contributors who sent pull requests.

 [PATCH 1/7] Cryptomount support LUKS detached header
 Support LUKS detached headers so that the header can be separated from
the data payload, e.g. by storing on external removable media such as a
USB key.

 [PATCH 2/7] Cryptomount support key files
 Support key files so that passphrase entry can be suppressed. The
passphrase can be stored in a "key file" that can be stored, for
example, on external removable media such as a USB key.

 [PATCH 3/7] cryptomount luks allow multiple passphrase attempts
 Allow a second attempt to enter a passphrase. If unlocking fails on the
first attempt then the user is presented with the passphrase entry
prompt again. If a key file is given that does not unlock the device
then the user is given the opportunity to enter a passphrase.

This feature was added following feedback from Andrei Borzenkov back in
2015 when I first submitted these patches.

 [PATCH 4/7] Cryptomount support plain dm-crypt
 Support plain dm-crypt mode. Allow plain volumes to be opened. This is
largely a re-factoring of exisitng code to allow the crypto routines be
used independently of LUKS.

 An important thing to recognise with this patch is that it largely
moves code from luks to cryptodisk so that it can be used outside of
luks (i.e. as plain dm-crypt). The crypto implementation was not changed
- most of the code in this patch already existed.

A little explanation of what the patch does:

I extracted the in-line code from "luks.c" that creates the crypto disk
into a new cryptomount function called "grub_cryptodisk_create" that is
then used by the luks module and is also avilable to the cryptomount
module.

I extracted the "set_passphrase" function from the "devmapper.c"
committed (e7f405ab) in the "peter/devmapper" branch so that I could use
it in cryptomount to hash a manually entered passphrase.

I then wrote some additional options and a small block of code to
implement plain mode using the above.


 [PATCH 5/7] Cryptomount support for hyphens in UUID
 Support for hyphens in UUID. The "-u" option of cryptomount accepts a
UUID. This option allows that to be delimited with hyphens so that the
same format can be given to Grub as is passed to the Linux kernel boot
options.

Contributor patches

 [PATCH 6/7] Retain constness of parameters.
  Don't drop constness on the parameters since they are being only read
anyway. Without this patch, compilation may fail if the compiler
complains that the constness of passed in parameters is being dropped.

 [PATCH 7/7] Add support for using a whole device as a keyfile
 This fixes the situation where a device is passed as a parameter to
--keyfile instead of the path to a file.

It is probably worth reviewing the email threads beginning with [1] and
[2], and especially my reply [3] in which I answer FAQs about this work.
discusses the self-generation (grub-mkconfig) aspects of this,
especially with respect to plain mode (there isn't any - but I had some
ideas in that mail about how it could possibly be achieved).

[1] http://lists.gnu.org/archive/html/grub-devel/2015-06/msg00109.html
[2] http://lists.gnu.org/archive/html/help-grub/2017-02/msg00017.html
[3] http://lists.gnu.org/archive/html/help-grub/2017-02/msg00029.html

end

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] Cryptomount support LUKS detached header

[John Lane]

On 17/03/18 11:09, TJ wrote:
> On 14/03/18 09:44, John Lane wrote:
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -880,7 +882,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, 
>> const char *cheat)
>>  
>>FOR_CRYPTODISK_DEVS (cr)
>>{
>> -dev = cr->scan (source, search_uuid, check_boot);
>> +dev = cr->scan (source, search_uuid, check_boot,0);
> 
> Minor nit; "boot,0" > "boot, 0"
> 
>> @@ -934,6 +936,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
>> argc, char **args)
>>if (argc < 1 && !state[1].set && !state[2].set)
>>  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>>  
>> +  if (state[3].set) /* LUKS detached header */
>> +{
>> +  if (state[0].set) /* Cannot use UUID lookup with detached header */
> 
> Should there be specific warning for the user here, at least for #ifdef
> GRUB_UTIL
> 
>> --- a/grub-core/disk/luks.c
>> +++ b/grub-core/disk/luks.c
>> @@ -391,13 +415,18 @@ luks_recover_key (grub_disk_t source,
>>return grub_crypto_gcry_error (gcry_err);
>>  }
>>  
>> +  sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
>>length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
>>  
>>/* Read and decrypt the key material from the disk.  */
>> -  err = grub_disk_read (source,
>> -grub_be_to_cpu32 (header.keyblock
>> -  [i].keyMaterialOffset), 0,
>> -length, split_key);
>> +  if (hdr)
>> +{
>> +  grub_file_seek (hdr, sector * 512);
> 
> Shouldn't 512 be GRUB_DISK_SECTOR_SIZE ?
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

I have applied these changes. Builds clean but I need to test it. I
might have time to do that tomorrow, otherwise next weekend will be soonest.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/7] Cryptomount support key files

[John Lane]

On 17/03/18 11:10, TJ wrote:
> On 14/03/18 09:44, John Lane wrote:
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -949,6 +954,45 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
>> argc, char **args)
>>  hdr = NULL;
>>  
>>have_it = 0;
>> +  key = NULL;
>> +
>> +  if (state[4].set) /* Key file; fails back to passphrase entry */
>> +{
>> +  grub_file_t keyfile;
>> +  int keyfile_offset;
>> +  grub_size_t requested_keyfile_size;
>> +
>> +  requested_keyfile_size = state[6].set ? grub_strtoul(state[6].arg, 0, 
>> 0) : 0;
>> +
>> +  if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
>> +grub_printf (N_("Key file size exceeds maximum (%llu)\n"), \
>> + (unsigned long long) 
>> GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
>> +  else
>> +{
>> +  keyfile_offset = state[5].set ? grub_strtoul (state[5].arg, 0, 0) 
>> : 0;
>> +  keyfile_size = requested_keyfile_size ? requested_keyfile_size : \
>> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
>> +
>> +  keyfile = grub_file_open (state[4].arg);
>> +  if (!keyfile)
>> +grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
>> +  else if (grub_file_seek (keyfile, keyfile_offset) == 
>> (grub_off_t)-1)
>> +grub_printf (N_("Unable to seek to offset %d in key file\n"), 
>> keyfile_offset);
>> +  else
>> +{
>> +  keyfile_size = grub_file_read (keyfile, keyfile_buffer, 
>> keyfile_size);
>> +  if (keyfile_size == (grub_size_t)-1)
> 
> grub_file_read() returns grub_ssize_t (signed). Is casting to
> grub_size_t (unsigned) required or going to work as intended?
> 
> Is the only possible error -1? Underlying readwrite functions can return
> error codes via grub_error() that are > 0: see include/grub/err.h
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 


I have applied these changes. Builds clean but I need to test it. I
might have time to do that tomorrow, otherwise next weekend will be soonest.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/7] cryptomount luks allow multiple passphrase attempts

[John Lane]

On 17/03/18 11:10, TJ wrote:
> On 14/03/18 09:45, John Lane wrote:
>> --- a/grub-core/disk/luks.c
>> +++ b/grub-core/disk/luks.c
>> @@ -321,10 +321,10 @@ configure_ciphers (grub_disk_t disk, const char 
>> *check_uuid,
>>  
>>  static grub_err_t
>>  luks_recover_key (grub_disk_t source,
>> -  grub_cryptodisk_t dev,
>> -  grub_file_t hdr,
>> -  grub_uint8_t *keyfile_bytes,
>> -  grub_size_t keyfile_bytes_size)
>> +  grub_cryptodisk_t dev,
>> +  grub_file_t hdr,
>> +  grub_uint8_t *keyfile_bytes,
>> +  grub_size_t keyfile_bytes_size)
> 
> ---8-<--- snip
> 
> Much of this patch is moving existing code around, could it be
> refactored to avoid that so as to make the new code stand out?
> 

The code that was moved was just indented into a while loop.
Most of it pre-existed prior to my patches (#357-464), a small part was
added by patch#2. I'm not sure how I would refactor it - any change
would result in 100-ish lines changing position and/or indent and would
lead to a similarly sized patch.

The way the patch presents the changes is confusing but I am not sure
how to control that.

If it helps, all this patch did was add a while loop around the
passphrase reading code to allow the user 3 attempts. i.e

  while (attempts)
{

  

  grub_printf_ (N_("Failed to decrypt master key.\n"));
  if (--attempts) grub_printf_ (N_("%u attempt%s remaining.\n"),
attempts,
(attempts==1) ? "" : "s");
}




___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] Cryptomount support LUKS detached header

[John Lane]

On 22/03/18 14:22, TJ wrote:
> On 22/03/18 12:38, Daniel Kiper wrote:
>> Hi John,
>>
>> On Wed, Mar 14, 2018 at 07:00:11PM +, John Lane wrote:
>>> On 14/03/18 13:05, Daniel Kiper wrote:
>>>> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
>>>>> From: John Lane 
>>>>
>>>> I have just skimmed through the series. First of all, most if not
>>>> all patches beg for full blown commit messages. Just vague statements
>>>> in the subject are insufficient for me. And please add patch 0 which
>>>> introduces the series. git send-email --compose is your friend.
>>>>
>>>> Daniel
>>>>
>>>
>>> Sorry Daniel, this isn't something I do that often - submitting patches
>>
>> Not a problem.
>>
>>> to ML. Do you want me to resubmit them again, or is the below ok for you ?
>>
>> Please resubmit whole patch series and do not forget to take into
>> account comments posted by others.
>>
>> Daniel
> 
> I've spent a couple of days doing a thorough review of this patch series.
> 
> Firstly I want to say a big thanks to John for bringing this along -
> it's long been a large missing piece of the LUKS support.

Thanks for that. I will take a look at the below and try and work a
patch series as you suggest. But it might take a few days to find some
time to dedicate to it, due to other commitments I have right now.

I'll report back when I have something to show.

> 
> My observations:
> 
> Break the series up. There are five distinct sets of functionality
> change here:
>   a) LUKS detached headers
>   b) LUKS key files
>   c) allow multiple unlock attempts
>   d) plain dm-crypt
>   e) hyphens in UUIDs
> 
> (a) and (b) are in reasonable shape but there's some work to do; mostly
> in preparing the way for the new functionality by cleaning up error exit
> paths in luks.c::luks_recover_key() first - which I've done and will attach.
> 
> With that clean-up John's changes are easier to insert and verify.
> 
> (c) creates a lot of churn just due to indenting code that is being
> wrapped in a while() loop. I've refactored that so the loop is in a
> separate function which reduces the patch from 323 to 41 lines. It's in
> my branch detailed below.
> 
> I'd suggest submitting (a) (b) and (c) as a series on their own. Get
> them accepted and then introduce (e) and (d). I'd say (e) first since
> it's relatively small.
> 
> (d) is a major new tranch of functionality dealing with core
> cryptographic constructs so will need careful review by cryptographers
> as well as GRUB developers and therefore could take some time. It'd be a
> shame to have this hold up the excellent improvements to LUKS which
> don't touch the cryptographic operations.
> 
> All in all an excellent contribution which I look forward to being
> available.
> 
> My "cryptomount_luks_v5" branch for the LUKS changes can be got using:
> 
> git remote add iam.tj git://iam.tj/grub.git
> git fetch iam.tj
> 
> and seen at:
> 
> http://iam.tj/gitweb/?p=grub.git;a=shortlog;h=refs/heads/cryptomount_luks_v5
> 
> 
> ---
> commit 19896820737344fb820ab6487d16719e31cae763
> Author: TJ 
> Date:   Wed Mar 21 14:07:22 2018 +
> 
> LUKS: refactor multiple return paths
> 
> Convert multiple return statements to goto with a single return so there
> is only one place where memory needs to be free-d in error conditions.
> 
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 86c50c6..a7c5b39 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -318,18 +318,23 @@ luks_recover_key (grub_disk_t source,
>grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
>unsigned i;
>grub_size_t length;
> -  grub_err_t err;
> +  grub_err_t err = GRUB_ERR_NONE;
> +  char *err_msg = NULL;
>grub_size_t max_stripes = 1;
>char *tmp;
> 
>err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>if (err)
> -return err;
> +goto fail;
> 
>grub_puts_ (N_("Attempting to decrypt master key..."));
>keysize = grub_be_to_cpu32 (header.keyBytes);
>if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN)
> -return grub_error (GRUB_ERR_BAD_FS, "key is too long");
> +{
> +  err = GRUB_ERR_BAD_FS;
> +  err_msg = "key is too long";
> +  goto fail;
> +}
> 
>for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
>  if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED
> @@ -338,7 +3

Patches to cryptomount (plain support, keyfiles and LUKS detached headers)

[John Lane]

Hello, I've been working over the past couple of weeks on adding some
functionality to the "cryptomount" command to support plain-mode
dm-crypt, keyfiles and LUKS detached headers. I've put my work on GitHub
and written a few notes on http://grub.johnlane.ie, along with my
patches. I believe this is the right list to post this kind of thing on.

I can explain in detail what I did if there's interest in what I've
done. I haven't added much code -  I mostly made use of what was already
there, using it in different ways to support some additional use-cases
that I needed.

Best,
John Lane






___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Patches to cryptomount (plain support, keyfiles and LUKS detached headers)

[John Lane]

I did some work a while ago to update the crypto routines to support
LUKS detached headers.
I've been busy on other things but just found some time to update to the
current master head.

On 22/01/15 21:04, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 15.12.2014 12:30, John Lane wrote:
>> Hello, I've been working over the past couple of weeks on adding some
>> functionality to the "cryptomount" command to support plain-mode
>> dm-crypt, keyfiles and LUKS detached headers. I've put my work on GitHub
>> and written a few notes on http://grub.johnlane.ie, along with my
>> patches. I believe this is the right list to post this kind of thing on.
>>
> Sorry, we cannot accept patches which aren't sent to this ml by author.
I've attached the patches here. They apply clean to c945ca75.
> I'm not sure that all features are good. For starters plain mode is just
> difficult to setup and use. Please provide usecases not already covered
> by current features.

My target was to establish LUKS volumes with detached headers and key
files and this is not already covered by current features.

My specific use-case is booting secured systems where the boot
environment (Grub, LUKS headers and keys) is contained on removable
media such as a USB key. The non-removable hard-drive has no boot code
on it; it just appears as an unformatted disk unless the removable key
is used.

To support this, it was necessary to add support to Grub for detached
LUKS headers and keys.

I am aware of a number of other people enquiring about this specific
functionality so I am not alone in thinking it's a valid use-case.

Regarding plain mode,  I don't understand why plain mode is "difficult
to setup and use". I did the work on plain mode at the same time because
one of the disks that I needed to work with was a plain mode disk. I
asked about the existing but non-functioning "peter/devmapper" branch
and spent some time trying to get that to work. In the end, and as I
understand how LUKS uses dm-crypt, it seemed better to re-use the
existing code base in the cryptodisk routines because this is more
current, used and tested. By doing that I was able to get it to work
very quickly.

I've been using my changes in daily use since my original postings last
December. I've just updated to the latest head and the patches still
merge cleanly.

I'd appreciate it if these changes could be considered. If any more
information would be useful please let me know.

>> I can explain in detail what I did if there's interest in what I've
>> done. I haven't added much code -  I mostly made use of what was already
>> there, using it in different ways to support some additional use-cases
>> that I needed.
>>
>> Best,
>> John Lane
>>
>>
>>
>>
>>
>>
>> ___
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

From be5328baa13459fddcef2e55fde398eb6932ebf5 Mon Sep 17 00:00:00 2001
From: John Lane 
Date: Sat, 29 Nov 2014 18:59:43 +
Subject: [PATCH 1/1] Cryptomount support for hyphens in UUID

---
 grub-core/disk/cryptodisk.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f0e3a90..cdb9950 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -103,6 +103,14 @@ gf_mul_be (grub_uint8_t *o, const grub_uint8_t *a, const grub_uint8_t *b)
 }
 }
 
+void
+grub_crypto_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,
@@ -488,6 +496,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
 
   if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
 {
+  grub_crypto_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;
@@ -925,6 +934,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 {
   grub_cryptodisk_t dev;
 
+  grub_crypto_uuid_dehyphenate(args[0]);
   dev = grub_cryptodisk_get_by_uuid (args[0]);
   if (dev)
 	{
-- 
2.1.2

From b6bc9bb59d1245a8510948cc5bf51112aa005506 Mon Sep 17 00:00:00 2001
From: John Lane 
Date: Sun, 30 Nov 2014 19:47:49 +
Subject: [PATCH 1/1] Cryptomount