All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] Cryptomount support LUKS detached header
@ 2020-02-21 21:03 Denis 'GNUtoo' Carikli
  2020-02-21 21:03 ` [PATCH 2/2] Cryptomount support key files Denis 'GNUtoo' Carikli
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Denis 'GNUtoo' Carikli @ 2020-02-21 21:03 UTC (permalink / raw)
  To: Grub-devel; +Cc: John Lane, Denis 'GNUtoo' Carikli

From: John Lane <john@lane.uk.net>

Signed-off-by: John Lane <john@lane.uk.net>
GNUtoo@cyberdimension.org: rebase
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 grub-core/disk/cryptodisk.c | 22 ++++++++++++++----
 grub-core/disk/geli.c       |  7 ++++--
 grub-core/disk/luks.c       | 45 ++++++++++++++++++++++++++++++-------
 include/grub/cryptodisk.h   |  5 +++--
 include/grub/file.h         |  2 ++
 5 files changed, 65 insertions(+), 16 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 1897acc4b..6d4befc6f 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}
   };
 
@@ -970,6 +971,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)
@@ -994,13 +996,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);
@@ -1041,7 +1043,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)
@@ -1095,6 +1097,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, GRUB_FILE_TYPE_LUKS_DETACHED_HEADER);
+      if (!hdr)
+        return grub_errno;
+    }
+  else
+    hdr = NULL;
+
   have_it = 0;
   if (state[0].set)
     {
@@ -1302,7 +1316,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 <grub/dl.h>
 #include <grub/err.h>
 #include <grub/disk.h>
+#include <grub/file.h>
 #include <grub/crypto.h>
 #include <grub/partition.h>
 #include <grub/i18n.h>
@@ -243,7 +244,8 @@ grub_util_get_geli_uuid (const char *dev)
 
 static grub_cryptodisk_t
 configure_ciphers (grub_disk_t disk, const char *check_uuid,
-		   int boot_only)
+		   int boot_only,
+		   grub_file_t hdr __attribute__ ((unused)) )
 {
   grub_cryptodisk_t newdev;
   struct grub_geli_phdr header;
@@ -398,7 +400,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
 }
 
 static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev)
+recover_key (grub_disk_t source, grub_cryptodisk_t dev,
+	     grub_file_t hdr __attribute__ ((unused)) )
 {
   grub_size_t keysize;
   grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 410cd6f84..950e89237 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -23,6 +23,7 @@
 #include <grub/dl.h>
 #include <grub/err.h>
 #include <grub/disk.h>
+#include <grub/file.h>
 #include <grub/crypto.h>
 #include <grub/partition.h>
 #include <grub/i18n.h>
@@ -66,7 +67,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
 
 static grub_cryptodisk_t
 configure_ciphers (grub_disk_t disk, const char *check_uuid,
-		   int check_boot)
+		   int check_boot, grub_file_t hdr)
 {
   grub_cryptodisk_t newdev;
   const char *iptr;
@@ -78,11 +79,21 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   char hashspec[sizeof (header.hashSpec) + 1];
   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)
@@ -146,12 +157,14 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
     }
 
   COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
+
   return newdev;
 }
 
 static grub_err_t
 luks_recover_key (grub_disk_t source,
-		  grub_cryptodisk_t dev)
+		  grub_cryptodisk_t dev,
+	          grub_file_t hdr)
 {
   struct grub_luks_phdr header;
   grub_size_t keysize;
@@ -163,8 +176,19 @@ luks_recover_key (grub_disk_t source,
   grub_err_t err;
   grub_size_t max_stripes = 1;
   char *tmp;
+  grub_uint32_t sector;
+
+  err = GRUB_ERR_NONE;
+
+  if (hdr)
+  {
+    grub_file_seek (hdr, 0);
+    if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
+        err = GRUB_ERR_READ_ERROR;
+  }
+  else
+    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
 
-  err = grub_disk_read (source, 0, 0, sizeof (header), &header);
   if (err)
     return err;
 
@@ -233,13 +257,18 @@ luks_recover_key (grub_disk_t source,
 	  return grub_crypto_gcry_error (gcry_err);
 	}
 
+      sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
       length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
 
       /* Read and decrypt the key material from the disk.  */
-      err = grub_disk_read (source,
-			    grub_be_to_cpu32 (header.keyblock
-					      [i].keyMaterialOffset), 0,
-			    length, split_key);
+      if (hdr)
+        {
+	  grub_file_seek (hdr, sector * 512);
+          if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
+            err = GRUB_ERR_READ_ERROR;
+        }
+      else
+        err = grub_disk_read (source, sector, 0, length, split_key);
       if (err)
 	{
 	  grub_free (split_key);
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index e1b21e785..ccbe7ce93 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -20,6 +20,7 @@
 #define GRUB_CRYPTODISK_HEADER	1
 
 #include <grub/disk.h>
+#include <grub/file.h>
 #include <grub/crypto.h>
 #include <grub/list.h>
 #ifdef GRUB_UTIL
@@ -107,8 +108,8 @@ struct grub_cryptodisk_dev
   struct grub_cryptodisk_dev **prev;
 
   grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
-			     int boot_only);
-  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev);
+			     int boot_only, grub_file_t hdr);
+  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr);
 };
 typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
 
diff --git a/include/grub/file.h b/include/grub/file.h
index 31567483c..49428bf8d 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -90,6 +90,8 @@ enum grub_file_type
     GRUB_FILE_TYPE_FONT,
     /* File holding encryption key for encrypted ZFS.  */
     GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
+    /* LUKS detached (separated) metadata header */
+    GRUB_FILE_TYPE_LUKS_DETACHED_HEADER,
     /* File we open n grub-fstest.  */
     GRUB_FILE_TYPE_FSTEST,
     /* File we open n grub-mount.  */
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] Cryptomount support key files
  2020-02-21 21:03 [PATCH 1/2] Cryptomount support LUKS detached header Denis 'GNUtoo' Carikli
@ 2020-02-21 21:03 ` Denis 'GNUtoo' Carikli
  2020-03-01  8:39   ` Patrick Steinhardt
  2020-02-24 11:12 ` [PATCH 1/2] Cryptomount support LUKS detached header Daniel Kiper
  2020-03-01  8:26 ` Patrick Steinhardt
  2 siblings, 1 reply; 6+ messages in thread
From: Denis 'GNUtoo' Carikli @ 2020-02-21 21:03 UTC (permalink / raw)
  To: Grub-devel; +Cc: John Lane, Denis 'GNUtoo' Carikli

From: John Lane <john@lane.uk.net>

Signed-off-by: John Lane <john@lane.uk.net>
GNUtoo@cyberdimension.org: rebase
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 grub-core/disk/cryptodisk.c | 46 ++++++++++++++++++++++++++++++++++++-
 grub-core/disk/geli.c       |  4 +++-
 grub-core/disk/luks.c       | 44 ++++++++++++++++++++++++-----------
 include/grub/cryptodisk.h   |  5 +++-
 include/grub/file.h         |  2 ++
 5 files changed, 84 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 6d4befc6f..ee2f300dd 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}
   };
 
@@ -972,6 +975,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)
@@ -1002,7 +1007,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);
@@ -1110,6 +1115,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, GRUB_FILE_TYPE_LUKS_KEY_FILE);
+          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 950e89237..54b1cfe70 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -164,12 +164,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;
@@ -206,18 +210,30 @@ luks_recover_key (grub_disk_t source,
   if (!split_key)
     return grub_errno;
 
-  /* Get the passphrase from the user.  */
-  tmp = NULL;
-  if (source->partition)
-    tmp = grub_partition_get_name (source->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-	       source->partition ? "," : "", tmp ? : "",
-	       dev->uuid);
-  grub_free (tmp);
-  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
+  if (keyfile_bytes)
     {
-      grub_free (split_key);
-      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+      /* Use bytestring from key file as passphrase */
+      passphrase = keyfile_bytes;
+      passphrase_length = keyfile_bytes_size;
+    }
+  else
+    {
+      /* Get the passphrase from the user.  */
+      tmp = NULL;
+      if (source->partition)
+        tmp = grub_partition_get_name (source->partition);
+      grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+		    source->partition ? "," : "", tmp ? : "", dev->uuid);
+      grub_free (tmp);
+      if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
+        {
+          grub_free (split_key);
+          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+        }
+
+      passphrase = (grub_uint8_t *)interactive_passphrase;
+      passphrase_length = grub_strlen (interactive_passphrase);
+
     }
 
   /* Try to recover master key from each active keyslot.  */
@@ -235,7 +251,7 @@ luks_recover_key (grub_disk_t source,
 
       /* Calculate the PBKDF2 of the user supplied passphrase.  */
       gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
-				     grub_strlen (passphrase),
+				     passphrase_length,
 				     header.keyblock[i].passwordSalt,
 				     sizeof (header.keyblock[i].passwordSalt),
 				     grub_be_to_cpu32 (header.keyblock[i].
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index ccbe7ce93..ac67ad8fe 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -55,6 +55,8 @@ typedef enum
 #define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
 #define GRUB_CRYPTODISK_MAX_KEYLEN 128
 
+#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
+
 struct grub_cryptodisk;
 
 typedef gcry_err_code_t
@@ -109,7 +111,8 @@ struct grub_cryptodisk_dev
 
   grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
 			     int boot_only, grub_file_t hdr);
-  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr);
+  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,
+			    grub_file_t hdr, grub_uint8_t *key, grub_size_t keyfile_size);
 };
 typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
 
diff --git a/include/grub/file.h b/include/grub/file.h
index 49428bf8d..ef7b89f24 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -92,6 +92,8 @@ enum grub_file_type
     GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
     /* LUKS detached (separated) metadata header */
     GRUB_FILE_TYPE_LUKS_DETACHED_HEADER,
+    /* LUKS decryption key */
+    GRUB_FILE_TYPE_LUKS_KEY_FILE,
     /* File we open n grub-fstest.  */
     GRUB_FILE_TYPE_FSTEST,
     /* File we open n grub-mount.  */
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] Cryptomount support LUKS detached header
  2020-02-21 21:03 [PATCH 1/2] Cryptomount support LUKS detached header Denis 'GNUtoo' Carikli
  2020-02-21 21:03 ` [PATCH 2/2] Cryptomount support key files Denis 'GNUtoo' Carikli
@ 2020-02-24 11:12 ` Daniel Kiper
  2020-02-25 21:46   ` Patrick Steinhardt
  2020-03-01  8:26 ` Patrick Steinhardt
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel Kiper @ 2020-02-24 11:12 UTC (permalink / raw)
  To: Denis 'GNUtoo' Carikli; +Cc: Grub-devel, John Lane, ps

Adding Patrick...

On Fri, Feb 21, 2020 at 10:03:48PM +0100, Denis 'GNUtoo' Carikli wrote:
> From: John Lane <john@lane.uk.net>

Both patches require explanation what they do and more importantly why
they are needed... And it would be nice to have cover letter.

...and please CC Patrick next time...

Daniel

> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> ---
>  grub-core/disk/cryptodisk.c | 22 ++++++++++++++----
>  grub-core/disk/geli.c       |  7 ++++--
>  grub-core/disk/luks.c       | 45 ++++++++++++++++++++++++++++++-------
>  include/grub/cryptodisk.h   |  5 +++--
>  include/grub/file.h         |  2 ++
>  5 files changed, 65 insertions(+), 16 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 1897acc4b..6d4befc6f 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}
>    };
>
> @@ -970,6 +971,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)
> @@ -994,13 +996,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);
> @@ -1041,7 +1043,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)
> @@ -1095,6 +1097,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, GRUB_FILE_TYPE_LUKS_DETACHED_HEADER);
> +      if (!hdr)
> +        return grub_errno;
> +    }
> +  else
> +    hdr = NULL;
> +
>    have_it = 0;
>    if (state[0].set)
>      {
> @@ -1302,7 +1316,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 <grub/dl.h>
>  #include <grub/err.h>
>  #include <grub/disk.h>
> +#include <grub/file.h>
>  #include <grub/crypto.h>
>  #include <grub/partition.h>
>  #include <grub/i18n.h>
> @@ -243,7 +244,8 @@ grub_util_get_geli_uuid (const char *dev)
>
>  static grub_cryptodisk_t
>  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> -		   int boot_only)
> +		   int boot_only,
> +		   grub_file_t hdr __attribute__ ((unused)) )
>  {
>    grub_cryptodisk_t newdev;
>    struct grub_geli_phdr header;
> @@ -398,7 +400,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>  }
>
>  static grub_err_t
> -recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> +recover_key (grub_disk_t source, grub_cryptodisk_t dev,
> +	     grub_file_t hdr __attribute__ ((unused)) )
>  {
>    grub_size_t keysize;
>    grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 410cd6f84..950e89237 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -23,6 +23,7 @@
>  #include <grub/dl.h>
>  #include <grub/err.h>
>  #include <grub/disk.h>
> +#include <grub/file.h>
>  #include <grub/crypto.h>
>  #include <grub/partition.h>
>  #include <grub/i18n.h>
> @@ -66,7 +67,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
>
>  static grub_cryptodisk_t
>  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> -		   int check_boot)
> +		   int check_boot, grub_file_t hdr)
>  {
>    grub_cryptodisk_t newdev;
>    const char *iptr;
> @@ -78,11 +79,21 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>    char hashspec[sizeof (header.hashSpec) + 1];
>    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)
> @@ -146,12 +157,14 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>      }
>
>    COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> +
>    return newdev;
>  }
>
>  static grub_err_t
>  luks_recover_key (grub_disk_t source,
> -		  grub_cryptodisk_t dev)
> +		  grub_cryptodisk_t dev,
> +	          grub_file_t hdr)
>  {
>    struct grub_luks_phdr header;
>    grub_size_t keysize;
> @@ -163,8 +176,19 @@ luks_recover_key (grub_disk_t source,
>    grub_err_t err;
>    grub_size_t max_stripes = 1;
>    char *tmp;
> +  grub_uint32_t sector;
> +
> +  err = GRUB_ERR_NONE;
> +
> +  if (hdr)
> +  {
> +    grub_file_seek (hdr, 0);
> +    if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
> +        err = GRUB_ERR_READ_ERROR;
> +  }
> +  else
> +    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>
> -  err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>    if (err)
>      return err;
>
> @@ -233,13 +257,18 @@ luks_recover_key (grub_disk_t source,
>  	  return grub_crypto_gcry_error (gcry_err);
>  	}
>
> +      sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
>        length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
>
>        /* Read and decrypt the key material from the disk.  */
> -      err = grub_disk_read (source,
> -			    grub_be_to_cpu32 (header.keyblock
> -					      [i].keyMaterialOffset), 0,
> -			    length, split_key);
> +      if (hdr)
> +        {
> +	  grub_file_seek (hdr, sector * 512);
> +          if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
> +            err = GRUB_ERR_READ_ERROR;
> +        }
> +      else
> +        err = grub_disk_read (source, sector, 0, length, split_key);
>        if (err)
>  	{
>  	  grub_free (split_key);
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index e1b21e785..ccbe7ce93 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -20,6 +20,7 @@
>  #define GRUB_CRYPTODISK_HEADER	1
>
>  #include <grub/disk.h>
> +#include <grub/file.h>
>  #include <grub/crypto.h>
>  #include <grub/list.h>
>  #ifdef GRUB_UTIL
> @@ -107,8 +108,8 @@ struct grub_cryptodisk_dev
>    struct grub_cryptodisk_dev **prev;
>
>    grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
> -			     int boot_only);
> -  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev);
> +			     int boot_only, grub_file_t hdr);
> +  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr);
>  };
>  typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
>
> diff --git a/include/grub/file.h b/include/grub/file.h
> index 31567483c..49428bf8d 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -90,6 +90,8 @@ enum grub_file_type
>      GRUB_FILE_TYPE_FONT,
>      /* File holding encryption key for encrypted ZFS.  */
>      GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
> +    /* LUKS detached (separated) metadata header */
> +    GRUB_FILE_TYPE_LUKS_DETACHED_HEADER,
>      /* File we open n grub-fstest.  */
>      GRUB_FILE_TYPE_FSTEST,
>      /* File we open n grub-mount.  */
> --
> 2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] Cryptomount support LUKS detached header
  2020-02-24 11:12 ` [PATCH 1/2] Cryptomount support LUKS detached header Daniel Kiper
@ 2020-02-25 21:46   ` Patrick Steinhardt
  0 siblings, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2020-02-25 21:46 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: Denis 'GNUtoo' Carikli, John Lane

[-- Attachment #1: Type: text/plain, Size: 10038 bytes --]

On Mon, Feb 24, 2020 at 12:12:37PM +0100, Daniel Kiper wrote:
> Adding Patrick...
> 
> On Fri, Feb 21, 2020 at 10:03:48PM +0100, Denis 'GNUtoo' Carikli wrote:
> > From: John Lane <john@lane.uk.net>
> 
> Both patches require explanation what they do and more importantly why
> they are needed... And it would be nice to have cover letter.
> 
> ...and please CC Patrick next time...
> 
> Daniel

I'll have a look towards the end of this week.

Patrick

> > Signed-off-by: John Lane <john@lane.uk.net>
> > GNUtoo@cyberdimension.org: rebase
> > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> > ---
> >  grub-core/disk/cryptodisk.c | 22 ++++++++++++++----
> >  grub-core/disk/geli.c       |  7 ++++--
> >  grub-core/disk/luks.c       | 45 ++++++++++++++++++++++++++++++-------
> >  include/grub/cryptodisk.h   |  5 +++--
> >  include/grub/file.h         |  2 ++
> >  5 files changed, 65 insertions(+), 16 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 1897acc4b..6d4befc6f 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}
> >    };
> >
> > @@ -970,6 +971,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)
> > @@ -994,13 +996,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);
> > @@ -1041,7 +1043,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)
> > @@ -1095,6 +1097,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, GRUB_FILE_TYPE_LUKS_DETACHED_HEADER);
> > +      if (!hdr)
> > +        return grub_errno;
> > +    }
> > +  else
> > +    hdr = NULL;
> > +
> >    have_it = 0;
> >    if (state[0].set)
> >      {
> > @@ -1302,7 +1316,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 <grub/dl.h>
> >  #include <grub/err.h>
> >  #include <grub/disk.h>
> > +#include <grub/file.h>
> >  #include <grub/crypto.h>
> >  #include <grub/partition.h>
> >  #include <grub/i18n.h>
> > @@ -243,7 +244,8 @@ grub_util_get_geli_uuid (const char *dev)
> >
> >  static grub_cryptodisk_t
> >  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> > -		   int boot_only)
> > +		   int boot_only,
> > +		   grub_file_t hdr __attribute__ ((unused)) )
> >  {
> >    grub_cryptodisk_t newdev;
> >    struct grub_geli_phdr header;
> > @@ -398,7 +400,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >  }
> >
> >  static grub_err_t
> > -recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> > +recover_key (grub_disk_t source, grub_cryptodisk_t dev,
> > +	     grub_file_t hdr __attribute__ ((unused)) )
> >  {
> >    grub_size_t keysize;
> >    grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 410cd6f84..950e89237 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -23,6 +23,7 @@
> >  #include <grub/dl.h>
> >  #include <grub/err.h>
> >  #include <grub/disk.h>
> > +#include <grub/file.h>
> >  #include <grub/crypto.h>
> >  #include <grub/partition.h>
> >  #include <grub/i18n.h>
> > @@ -66,7 +67,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
> >
> >  static grub_cryptodisk_t
> >  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> > -		   int check_boot)
> > +		   int check_boot, grub_file_t hdr)
> >  {
> >    grub_cryptodisk_t newdev;
> >    const char *iptr;
> > @@ -78,11 +79,21 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >    char hashspec[sizeof (header.hashSpec) + 1];
> >    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)
> > @@ -146,12 +157,14 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >      }
> >
> >    COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> > +
> >    return newdev;
> >  }
> >
> >  static grub_err_t
> >  luks_recover_key (grub_disk_t source,
> > -		  grub_cryptodisk_t dev)
> > +		  grub_cryptodisk_t dev,
> > +	          grub_file_t hdr)
> >  {
> >    struct grub_luks_phdr header;
> >    grub_size_t keysize;
> > @@ -163,8 +176,19 @@ luks_recover_key (grub_disk_t source,
> >    grub_err_t err;
> >    grub_size_t max_stripes = 1;
> >    char *tmp;
> > +  grub_uint32_t sector;
> > +
> > +  err = GRUB_ERR_NONE;
> > +
> > +  if (hdr)
> > +  {
> > +    grub_file_seek (hdr, 0);
> > +    if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
> > +        err = GRUB_ERR_READ_ERROR;
> > +  }
> > +  else
> > +    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> >
> > -  err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> >    if (err)
> >      return err;
> >
> > @@ -233,13 +257,18 @@ luks_recover_key (grub_disk_t source,
> >  	  return grub_crypto_gcry_error (gcry_err);
> >  	}
> >
> > +      sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
> >        length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
> >
> >        /* Read and decrypt the key material from the disk.  */
> > -      err = grub_disk_read (source,
> > -			    grub_be_to_cpu32 (header.keyblock
> > -					      [i].keyMaterialOffset), 0,
> > -			    length, split_key);
> > +      if (hdr)
> > +        {
> > +	  grub_file_seek (hdr, sector * 512);
> > +          if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
> > +            err = GRUB_ERR_READ_ERROR;
> > +        }
> > +      else
> > +        err = grub_disk_read (source, sector, 0, length, split_key);
> >        if (err)
> >  	{
> >  	  grub_free (split_key);
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index e1b21e785..ccbe7ce93 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -20,6 +20,7 @@
> >  #define GRUB_CRYPTODISK_HEADER	1
> >
> >  #include <grub/disk.h>
> > +#include <grub/file.h>
> >  #include <grub/crypto.h>
> >  #include <grub/list.h>
> >  #ifdef GRUB_UTIL
> > @@ -107,8 +108,8 @@ struct grub_cryptodisk_dev
> >    struct grub_cryptodisk_dev **prev;
> >
> >    grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
> > -			     int boot_only);
> > -  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev);
> > +			     int boot_only, grub_file_t hdr);
> > +  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_file_t hdr);
> >  };
> >  typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
> >
> > diff --git a/include/grub/file.h b/include/grub/file.h
> > index 31567483c..49428bf8d 100644
> > --- a/include/grub/file.h
> > +++ b/include/grub/file.h
> > @@ -90,6 +90,8 @@ enum grub_file_type
> >      GRUB_FILE_TYPE_FONT,
> >      /* File holding encryption key for encrypted ZFS.  */
> >      GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
> > +    /* LUKS detached (separated) metadata header */
> > +    GRUB_FILE_TYPE_LUKS_DETACHED_HEADER,
> >      /* File we open n grub-fstest.  */
> >      GRUB_FILE_TYPE_FSTEST,
> >      /* File we open n grub-mount.  */
> > --
> > 2.25.1
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] Cryptomount support LUKS detached header
  2020-02-21 21:03 [PATCH 1/2] Cryptomount support LUKS detached header Denis 'GNUtoo' Carikli
  2020-02-21 21:03 ` [PATCH 2/2] Cryptomount support key files Denis 'GNUtoo' Carikli
  2020-02-24 11:12 ` [PATCH 1/2] Cryptomount support LUKS detached header Daniel Kiper
@ 2020-03-01  8:26 ` Patrick Steinhardt
  2 siblings, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2020-03-01  8:26 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: John Lane, Denis 'GNUtoo' Carikli

[-- Attachment #1: Type: text/plain, Size: 7749 bytes --]

On Fri, Feb 21, 2020 at 10:03:48PM +0100, Denis 'GNUtoo' Carikli wrote:
> From: John Lane <john@lane.uk.net>
> 
> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

We usually prefix commit subjects by the respective subsystem they're
touching. For example something like "cryptodisk: add support for
detached headers". It would also be nice to have the commit message
explain what you're doing in this commit in the future.

> ---
>  grub-core/disk/cryptodisk.c | 22 ++++++++++++++----
>  grub-core/disk/geli.c       |  7 ++++--
>  grub-core/disk/luks.c       | 45 ++++++++++++++++++++++++++++++-------
>  include/grub/cryptodisk.h   |  5 +++--
>  include/grub/file.h         |  2 ++
>  5 files changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 1897acc4b..6d4befc6f 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}
>    };
>  
> @@ -970,6 +971,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)
> @@ -994,13 +996,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);
> @@ -1041,7 +1043,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)
> @@ -1095,6 +1097,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;

We should return a proper error to the user so that he knows that both
options are incompatible. E.g.

    return grub_error (GRUB_ERR_BAD_ARGUMENT,
                       "Cannot use UUID lookup with detached header");

That'd also make the comment obsolete.

> +
> +      hdr = grub_file_open (state[3].arg, GRUB_FILE_TYPE_LUKS_DETACHED_HEADER);
> +      if (!hdr)
> +        return grub_errno;
> +    }
> +  else
> +    hdr = NULL;
> +
>    have_it = 0;
>    if (state[0].set)
>      {
> @@ -1302,7 +1316,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 <grub/dl.h>
>  #include <grub/err.h>
>  #include <grub/disk.h>
> +#include <grub/file.h>
>  #include <grub/crypto.h>
>  #include <grub/partition.h>
>  #include <grub/i18n.h>
> @@ -243,7 +244,8 @@ grub_util_get_geli_uuid (const char *dev)
>  
>  static grub_cryptodisk_t
>  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> -		   int boot_only)
> +		   int boot_only,
> +		   grub_file_t hdr __attribute__ ((unused)) )
>  {
>    grub_cryptodisk_t newdev;
>    struct grub_geli_phdr header;
> @@ -398,7 +400,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>  }
>  
>  static grub_err_t
> -recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> +recover_key (grub_disk_t source, grub_cryptodisk_t dev,
> +	     grub_file_t hdr __attribute__ ((unused)) )
>  {
>    grub_size_t keysize;
>    grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];

You've probably started working on an oldish commit, as we've gained
LUKS2 support in the meantime. Means that you'll also have to change
function signatures for LUKS2 as it will otherwise fail to compile.

> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 410cd6f84..950e89237 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -23,6 +23,7 @@
>  #include <grub/dl.h>
>  #include <grub/err.h>
>  #include <grub/disk.h>
> +#include <grub/file.h>
>  #include <grub/crypto.h>
>  #include <grub/partition.h>
>  #include <grub/i18n.h>
> @@ -66,7 +67,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
>  
>  static grub_cryptodisk_t
>  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> -		   int check_boot)
> +		   int check_boot, grub_file_t hdr)
>  {
>    grub_cryptodisk_t newdev;
>    const char *iptr;
> @@ -78,11 +79,21 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>    char hashspec[sizeof (header.hashSpec) + 1];
>    grub_err_t err;
>  
> +  err = GRUB_ERR_NONE;
> +

err could just be initialized directly with its declaration.

>    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;
> +  }

You should check the return value of `grub_file_seek()`. I also feel
like we should not clobber `err` with our own value, but instead we
should probably return error codes of `grub_file_seek()` and
`grub_file_read()` directly.

> +  else
> +    err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
> +
>    if (err)
>      {
>        if (err == GRUB_ERR_OUT_OF_RANGE)
> @@ -146,12 +157,14 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>      }
>  
>    COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> +
>    return newdev;

Needless code churn :)

>  }
>  
>  static grub_err_t
>  luks_recover_key (grub_disk_t source,
> -		  grub_cryptodisk_t dev)
> +		  grub_cryptodisk_t dev,
> +	          grub_file_t hdr)
>  {
>    struct grub_luks_phdr header;
>    grub_size_t keysize;
> @@ -163,8 +176,19 @@ luks_recover_key (grub_disk_t source,
>    grub_err_t err;
>    grub_size_t max_stripes = 1;
>    char *tmp;
> +  grub_uint32_t sector;
> +
> +  err = GRUB_ERR_NONE;
> +
> +  if (hdr)
> +  {
> +    grub_file_seek (hdr, 0);
> +    if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
> +        err = GRUB_ERR_READ_ERROR;
> +  }

Same as above with regards to error handling.

The rest looks fine to me.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] Cryptomount support key files
  2020-02-21 21:03 ` [PATCH 2/2] Cryptomount support key files Denis 'GNUtoo' Carikli
@ 2020-03-01  8:39   ` Patrick Steinhardt
  0 siblings, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2020-03-01  8:39 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: John Lane, Denis 'GNUtoo' Carikli

[-- Attachment #1: Type: text/plain, Size: 8219 bytes --]

On Fri, Feb 21, 2020 at 10:03:49PM +0100, Denis 'GNUtoo' Carikli wrote:
> From: John Lane <john@lane.uk.net>
> 
> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

Same remarks with regards to commit subject and message as for the other
patch.

> ---
>  grub-core/disk/cryptodisk.c | 46 ++++++++++++++++++++++++++++++++++++-
>  grub-core/disk/geli.c       |  4 +++-
>  grub-core/disk/luks.c       | 44 ++++++++++++++++++++++++-----------
>  include/grub/cryptodisk.h   |  5 +++-
>  include/grub/file.h         |  2 ++
>  5 files changed, 84 insertions(+), 17 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 6d4befc6f..ee2f300dd 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}
>    };
>  
> @@ -972,6 +975,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)
> @@ -1002,7 +1007,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);
> @@ -1110,6 +1115,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;

I think we should check for conversion errors here when calling
`grub_strtoul()`. Defaulting to `GRUB_CRYPTODISK_MAX_KEYFILE_SIZE` in
case the user has typoed seems surprising to me.

> +      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);

Shouldn't we error out in this case? Same for the below error cases. If
we do so then we could also get rid of the cascading if statements by
returning early in case any error occurs.

> +      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;

Hum. So if we're given a keyfile size, then we obviously should read
exactly that many bytes. I'd argue in the case where the user didn't
pass a size that we should not use GRUB_CRYPTODISK_MAX_KEYFILE_SIZE, but
instead stat the file and read it in full. The benefit would be that we
know how many bytes to expect when calling `grub_file_read()`.

> +          keyfile = grub_file_open (state[4].arg, GRUB_FILE_TYPE_LUKS_KEY_FILE);
> +          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;
> +	    }
> +        }
> +    }
> +

Indentation in the above block is wrong, eight consecutive spaces should
be converted to a tab.

>    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];

Same as with the other patch, you'll also have to adjust
"grub-core/disk/luks2.c".

> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 950e89237..54b1cfe70 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -164,12 +164,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;
> @@ -206,18 +210,30 @@ luks_recover_key (grub_disk_t source,
>    if (!split_key)
>      return grub_errno;
>  
> -  /* Get the passphrase from the user.  */
> -  tmp = NULL;
> -  if (source->partition)
> -    tmp = grub_partition_get_name (source->partition);
> -  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> -	       source->partition ? "," : "", tmp ? : "",
> -	       dev->uuid);
> -  grub_free (tmp);
> -  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> +  if (keyfile_bytes)
>      {
> -      grub_free (split_key);
> -      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> +      /* Use bytestring from key file as passphrase */
> +      passphrase = keyfile_bytes;
> +      passphrase_length = keyfile_bytes_size;
> +    }
> +  else
> +    {
> +      /* Get the passphrase from the user.  */
> +      tmp = NULL;
> +      if (source->partition)
> +        tmp = grub_partition_get_name (source->partition);
> +      grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +		    source->partition ? "," : "", tmp ? : "", dev->uuid);
> +      grub_free (tmp);
> +      if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
> +        {
> +          grub_free (split_key);
> +          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
> +        }
> +
> +      passphrase = (grub_uint8_t *)interactive_passphrase;
> +      passphrase_length = grub_strlen (interactive_passphrase);
> +
>      }

Please remove the newline before the brace.

The rest looks fine to me.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-03-01  8:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-21 21:03 [PATCH 1/2] Cryptomount support LUKS detached header Denis 'GNUtoo' Carikli
2020-02-21 21:03 ` [PATCH 2/2] Cryptomount support key files Denis 'GNUtoo' Carikli
2020-03-01  8:39   ` Patrick Steinhardt
2020-02-24 11:12 ` [PATCH 1/2] Cryptomount support LUKS detached header Daniel Kiper
2020-02-25 21:46   ` Patrick Steinhardt
2020-03-01  8:26 ` Patrick Steinhardt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.