[PATCH v9 0/7] Cryptodisk detached headers and key files

[PATCH v9 0/7] Cryptodisk detached headers and key files

[Glenn Washburn]

Updates from v8:
* Add documentation patch
* Merge previous patch updating the cryptomount help string with key file
  options into the patch adding key file support
* Improve commit messages
* rename requested_keyfile_size -> keyfile_size
* Minor improvements to the code

This patch series adds LUKS deatched header and key file support to
cryptomount.

Glenn

Denis 'GNUtoo' Carikli (2):
  cryptodisk: luks: Unify grub_cryptodisk_dev function names
  cryptodisk: geli: Unify grub_cryptodisk_dev function names

Glenn Washburn (3):
  cryptodisk: Add --header option to cryptomount and fail to implement
    it in the backends
  luks2: Add detached header support
  docs: Add documentation on keyfile and detached header options to
    cryptomount

John Lane (2):
  cryptodisk: Add support for LUKS1 detached headers
  cryptodisk: Add options to cryptomount to support keyfiles

 docs/grub.texi              | 16 ++++--
 grub-core/disk/cryptodisk.c | 98 ++++++++++++++++++++++++++++++++++++-
 grub-core/disk/geli.c       | 18 +++++--
 grub-core/disk/luks.c       | 48 ++++++++++++++----
 grub-core/disk/luks2.c      | 59 ++++++++++++++++++----
 include/grub/cryptodisk.h   |  4 ++
 include/grub/file.h         |  4 ++
 7 files changed, 217 insertions(+), 30 deletions(-)

Range-diff against v8:
1:  9918a70dce ! 1:  40941ee45c cryptodisk: luks: unify grub_cryptodisk_dev function names
    @@ Metadata
     Author: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
     
      ## Commit message ##
    -    cryptodisk: luks: unify grub_cryptodisk_dev function names
    +    cryptodisk: luks: Unify grub_cryptodisk_dev function names
     
         Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
         Reviewed-by: Patrick Steinhardt <ps@pks.im>
2:  5d3ce5515e ! 2:  c259075bf3 cryptodisk: geli: unify grub_cryptodisk_dev function names
    @@ Metadata
     Author: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
     
      ## Commit message ##
    -    cryptodisk: geli: unify grub_cryptodisk_dev function names
    +    cryptodisk: geli: Unify grub_cryptodisk_dev function names
     
         Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
         Reviewed-by: Patrick Steinhardt <ps@pks.im>
3:  c7b8c290d7 ! 3:  1b2055ac5d cryptodisk: enable the backends to implement detached headers
    @@ Metadata
     Author: Glenn Washburn <development@efficientek.com>
     
      ## Commit message ##
    -    cryptodisk: enable the backends to implement detached headers
    +    cryptodisk: Add --header option to cryptomount and fail to implement it in the backends
    +
    +    Add a --header (short -H) option to cryptomount which takes a file argument.
    +    Pass the file to the backends via cargs struct and cause the backends to
    +    fail when passed a header. Detached header file support will be added later
    +    for individual backends.
     
         Signed-off-by: John Lane <john@lane.uk.net>
         GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
         Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
    -    development@efficientek.com: rebase, rework for cryptomount parameter passing
    +    development@efficientek.com: rebase, rework for cryptomount parameter passing,
    +      improve commit message
     
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: static const struct grub_arg_option options[] =
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
            cargs.key_len = grub_strlen (state[3].arg);
          }
      
    -+  if (state[4].set) /* Detached header */
    ++  if (state[4].set) /* header */
     +    {
     +      if (state[0].set)
     +	return grub_error (GRUB_ERR_BAD_ARGUMENT,
    -+			   N_("Cannot use UUID lookup with detached header"));
    ++			   N_("cannot use UUID lookup with detached header"));
     +
     +      cargs.hdr_file = grub_file_open (state[4].arg,
     +			    GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
    -+      if (!cargs.hdr_file)
    ++      if (cargs.hdr_file == NULL)
     +	return grub_errno;
     +    }
     +
4:  59c7c2abcb ! 4:  05c7ca844c cryptodisk: add support for LUKS1 detached headers
    @@ Metadata
     Author: John Lane <john@lane.uk.net>
     
      ## Commit message ##
    -    cryptodisk: add support for LUKS1 detached headers
    +    cryptodisk: Add support for LUKS1 detached headers
     
    -    cryptsetup supports having a detached header through the
    -    --header command line argument for both LUKS1 and LUKS2.
    -
    -    This adds support for LUKS1 detached headers.
    +    cryptsetup supports having a detached header through the --header command
    +    line argument for both LUKS1 and LUKS2. Allow the LUKS1 backend to use a
    +    given file as the LUKS1 header (aka detached header) instead of looking for
    +    the header on the disk.
     
         Signed-off-by: John Lane <john@lane.uk.net>
         GNUtoo@cyberdimension.org: rebase, small fixes, commit message
         Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
    -    development@efficientek.com: rebase
    +    development@efficientek.com: rebase, improve commit message
     
      ## grub-core/disk/luks.c ##
     @@
5:  9b436ce0e6 ! 5:  fb33d6810d cryptodisk: enable the backends to implement key files
    @@ Metadata
     Author: John Lane <john@lane.uk.net>
     
      ## Commit message ##
    -    cryptodisk: enable the backends to implement key files
    +    cryptodisk: Add options to cryptomount to support keyfiles
    +
    +    Add the options --key-file, --keyfile-offset, and --keyfile-size to
    +    cryptomount and code to put read the requested key file data and pass
    +    via the cargs struct. Note, key file data is for all intents and purposes
    +    equivalent to a password given to cryptomount. So there is no need to
    +    enable support for key files in the various crypto backends (eg. LUKS1)
    +    because the key data is passed just as if it were a password.
     
         Signed-off-by: John Lane <john@lane.uk.net>
         GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
         Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
    -    development@efficientek.com: rebase and rework to use cryptomount arg passing
    +    development@efficientek.com: rebase and rework to use cryptomount arg passing,
    +      minor fixes, improve commit message
     
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: static const struct grub_arg_option options[] =
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
     +      const char *p = NULL;
     +      grub_file_t keyfile;
     +      int keyfile_offset;
    -+      grub_size_t requested_keyfile_size = 0;
    ++      grub_size_t keyfile_size = 0;
     +
     +
     +      if (state[6].set) /* keyfile-offset */
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
     +
     +      if (state[7].set) /* keyfile-size */
     +	{
    -+	  requested_keyfile_size = grub_strtoul (state[7].arg, &p, 0);
    ++	  keyfile_size = grub_strtoul (state[7].arg, &p, 0);
     +
     +	  if (*p != '\0')
     +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
     +	  if (grub_errno != GRUB_ERR_NONE)
     +	    return grub_errno;
     +
    -+	  if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
    ++	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
     +	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
    -+			      N_("Key file size exceeds maximum (%d)\n"),
    -+			      GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
    ++			       N_("key file size exceeds maximum (%d)"),
    ++			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
     +
    -+	  if (requested_keyfile_size == 0)
    -+	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
    -+			      N_("Key file size is 0\n"));
    ++	  if (keyfile_size == 0)
    ++	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
     +	}
     +
     +      keyfile = grub_file_open (state[5].arg,
     +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
    -+      if (!keyfile)
    ++      if (keyfile == NULL)
     +	return grub_errno;
     +
     +      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
     +	return grub_errno;
     +
    -+      if (requested_keyfile_size)
    ++      if (keyfile_size > 0)
     +	{
    -+	  if (requested_keyfile_size > (keyfile->size - keyfile_offset))
    ++	  if (keyfile_size > (keyfile->size - keyfile_offset))
     +	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
    -+			       N_("Keyfile is too small: "
    ++			       N_("keyfile is too small: "
     +				  "requested %" PRIuGRUB_SIZE " bytes, "
     +				  "but the file only has %" PRIuGRUB_UINT64_T
    -+				  " bytes.\n"),
    -+			       requested_keyfile_size,
    ++				  " bytes"),
    ++			       keyfile_size,
     +			       keyfile->size);
     +
    -+	  cargs.key_len = requested_keyfile_size;
    ++	  cargs.key_len = keyfile_size;
     +	}
     +      else
     +	{
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
     +	}
     +
     +      cargs.key_data = grub_malloc (cargs.key_len);
    -+      if (!cargs.key_data)
    ++      if (cargs.key_data == NULL)
     +	return GRUB_ERR_OUT_OF_MEMORY;
     +
     +      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != (grub_ssize_t) cargs.key_len)
    -+	return grub_error (GRUB_ERR_FILE_READ_ERROR,
    -+			   (N_("Error reading key file\n")));
    ++	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file")));
     +    }
     +
        if (state[0].set) /* uuid */
          {
            int found_uuid;
    +@@ grub-core/disk/cryptodisk.c: GRUB_MOD_INIT (cryptodisk)
    + {
    +   grub_disk_dev_register (&grub_cryptodisk_dev);
    +   cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
    +-			      N_("[-p password] [-H file] <SOURCE|-u UUID|-a|-b>"),
    ++			      N_("[ [-p password] | [-k keyfile"
    ++				 " [-O keyoffset] [-S keysize] ] ] [-H file]"
    ++				 " <SOURCE|-u UUID|-a|-b>"),
    + 			      N_("Mount a crypto device."), options);
    +   grub_procfs_register ("luks_script", &luks_script);
    + }
     
      ## include/grub/cryptodisk.h ##
     @@ include/grub/cryptodisk.h: typedef enum
6:  ccb3bde361 < -:  ---------- cryptodisk: Improve cryptomount short help string
7:  0464e48e2d ! 6:  f15ff743c4 luks2: Add detached header support
    @@ Metadata
      ## Commit message ##
         luks2: Add detached header support
     
    +    If a header file is given to the LUKS2 backend, use that file as the LUKS2
    +    header, instead of looking for it on the disk.
    +
      ## grub-core/disk/luks2.c ##
     @@ grub-core/disk/luks2.c: luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s
      
-:  ---------- > 7:  53ba137d3b docs: Add documentation on keyfile and detached header options to cryptomount
-- 
2.25.1

[PATCH v9 1/7] cryptodisk: luks: Unify grub_cryptodisk_dev function names

[Glenn Washburn]

From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 46ae734efa..7f837d52c4 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -63,7 +63,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
 			  grub_size_t blocknumbers);
 
 static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
+luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   const char *iptr;
@@ -297,7 +297,7 @@ luks_recover_key (grub_disk_t source,
 }
 
 struct grub_cryptodisk_dev luks_crypto = {
-  .scan = configure_ciphers,
+  .scan = luks_scan,
   .recover_key = luks_recover_key
 };
 
-- 
2.25.1

Re: [PATCH v9 1/7] cryptodisk: luks: Unify grub_cryptodisk_dev function names

[Daniel Kiper]

On Mon, Apr 11, 2022 at 06:40:22AM +0000, Glenn Washburn wrote:
> From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
>
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v9 2/7] cryptodisk: geli: Unify grub_cryptodisk_dev function names

[Glenn Washburn]

From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/geli.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 445a668781..91eb101224 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -240,7 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
 #endif
 
 static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
+geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   struct grub_geli_phdr header;
@@ -395,7 +395,7 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
 }
 
 static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
+geli_recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
 {
   grub_size_t keysize;
   grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
@@ -567,8 +567,8 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
 }
 
 struct grub_cryptodisk_dev geli_crypto = {
-  .scan = configure_ciphers,
-  .recover_key = recover_key
+  .scan = geli_scan,
+  .recover_key = geli_recover_key
 };
 
 GRUB_MOD_INIT (geli)
-- 
2.25.1

Re: [PATCH v9 2/7] cryptodisk: geli: Unify grub_cryptodisk_dev function names

[Daniel Kiper]

On Mon, Apr 11, 2022 at 06:40:23AM +0000, Glenn Washburn wrote:
> From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
>
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v9 3/7] cryptodisk: Add --header option to cryptomount and fail to implement it in the backends

[Glenn Washburn]

Add a --header (short -H) option to cryptomount which takes a file argument.
Pass the file to the backends via cargs struct and cause the backends to
fail when passed a header. Detached header file support will be added later
for individual backends.

Signed-off-by: John Lane <john@lane.uk.net>
GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
development@efficientek.com: rebase, rework for cryptomount parameter passing,
  improve commit message
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 15 ++++++++++++++-
 grub-core/disk/geli.c       | 10 ++++++++++
 grub-core/disk/luks.c       |  8 ++++++++
 grub-core/disk/luks2.c      |  8 ++++++++
 include/grub/cryptodisk.h   |  2 ++
 include/grub/file.h         |  2 ++
 6 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9f5dc7acbc..063997d2f0 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -42,6 +42,7 @@ 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},
     {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
+    {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -1172,6 +1173,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       cargs.key_len = grub_strlen (state[3].arg);
     }
 
+  if (state[4].set) /* header */
+    {
+      if (state[0].set)
+	return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			   N_("cannot use UUID lookup with detached header"));
+
+      cargs.hdr_file = grub_file_open (state[4].arg,
+			    GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
+      if (cargs.hdr_file == NULL)
+	return grub_errno;
+    }
+
   if (state[0].set) /* uuid */
     {
       int found_uuid;
@@ -1384,7 +1397,7 @@ GRUB_MOD_INIT (cryptodisk)
 {
   grub_disk_dev_register (&grub_cryptodisk_dev);
   cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
-			      N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
+			      N_("[-p password] [-H file] <SOURCE|-u UUID|-a|-b>"),
 			      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 91eb101224..39b32a2add 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>
@@ -121,6 +122,7 @@ enum
 
 /* FIXME: support version 0.  */
 /* FIXME: support big-endian pre-version-4 volumes.  */
+/* FIXME: support for detached headers.  */
 /* FIXME: support for keyfiles.  */
 /* FIXME: support for HMAC.  */
 const char *algorithms[] = {
@@ -252,6 +254,10 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   grub_disk_addr_t sector;
   grub_err_t err;
 
+  /* Detached headers are not implemented yet */
+  if (cargs->hdr_file)
+    return NULL;
+
   if (2 * GRUB_MD_SHA256->mdlen + 1 > GRUB_CRYPTODISK_MAX_UUID_LENGTH)
     return NULL;
 
@@ -412,6 +418,10 @@ geli_recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_ar
   if (cargs->key_data == NULL || cargs->key_len == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
 
+  /* Detached headers are not implemented yet */
+  if (cargs->hdr_file)
+     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
   if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
     return grub_error (GRUB_ERR_BUG, "cipher block is too long");
 
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 7f837d52c4..fe1655c3c2 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -75,6 +75,10 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   char hashspec[sizeof (header.hashSpec) + 1];
   grub_err_t err;
 
+  /* Detached headers are not implemented yet */
+  if (cargs->hdr_file)
+    return NULL;
+
   if (cargs->check_boot)
     return NULL;
 
@@ -164,6 +168,10 @@ luks_recover_key (grub_disk_t source,
   if (cargs->key_data == NULL || cargs->key_len == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
 
+  /* Detached headers are not implemented yet */
+  if (cargs->hdr_file)
+     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
   err = grub_disk_read (source, 0, 0, sizeof (header), &header);
   if (err)
     return err;
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index bf741d70f3..349462c61a 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -353,6 +353,10 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   char uuid[sizeof (header.uuid) + 1];
   grub_size_t i, j;
 
+  /* Detached headers are not implemented yet */
+  if (cargs->hdr_file)
+    return NULL;
+
   if (cargs->check_boot)
     return NULL;
 
@@ -560,6 +564,10 @@ luks2_recover_key (grub_disk_t source,
   if (cargs->key_data == NULL || cargs->key_len == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
 
+  /* Detached headers are not implemented yet */
+  if (cargs->hdr_file)
+     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
   ret = luks2_read_header (source, &header);
   if (ret)
     return ret;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index c6524c9ea9..9fe451de92 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
@@ -77,6 +78,7 @@ struct grub_cryptomount_args
   grub_uint8_t *key_data;
   /* recover_key: Length of key_data */
   grub_size_t key_len;
+  grub_file_t hdr_file;
 };
 typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
 
diff --git a/include/grub/file.h b/include/grub/file.h
index 31567483cc..3a3c49a04c 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,
+    /* File holding the encryption metadata header */
+    GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER,
     /* File we open n grub-fstest.  */
     GRUB_FILE_TYPE_FSTEST,
     /* File we open n grub-mount.  */
-- 
2.25.1

Re: [PATCH v9 3/7] cryptodisk: Add --header option to cryptomount and fail to implement it in the backends

[Daniel Kiper]

On Mon, Apr 11, 2022 at 06:40:24AM +0000, Glenn Washburn wrote:
> Add a --header (short -H) option to cryptomount which takes a file argument.
> Pass the file to the backends via cargs struct and cause the backends to
> fail when passed a header. Detached header file support will be added later
> for individual backends.
>
> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> development@efficientek.com: rebase, rework for cryptomount parameter passing,
>   improve commit message
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v9 4/7] cryptodisk: Add support for LUKS1 detached headers

[Glenn Washburn]

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

cryptsetup supports having a detached header through the --header command
line argument for both LUKS1 and LUKS2. Allow the LUKS1 backend to use a
given file as the LUKS1 header (aka detached header) instead of looking for
the header on the disk.

Signed-off-by: John Lane <john@lane.uk.net>
GNUtoo@cyberdimension.org: rebase, small fixes, commit message
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
development@efficientek.com: rebase, improve commit message
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks.c | 48 ++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index fe1655c3c2..195c4bb8da 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>
@@ -73,17 +74,23 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   char ciphername[sizeof (header.cipherName) + 1];
   char ciphermode[sizeof (header.cipherMode) + 1];
   char hashspec[sizeof (header.hashSpec) + 1];
-  grub_err_t err;
-
-  /* Detached headers are not implemented yet */
-  if (cargs->hdr_file)
-    return NULL;
+  grub_err_t err = GRUB_ERR_NONE;
 
   if (cargs->check_boot)
     return NULL;
 
   /* Read the LUKS header.  */
-  err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
+  if (cargs->hdr_file)
+    {
+      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
+	return NULL;
+
+      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
+	return NULL;
+    }
+  else
+    err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
+
   if (err)
     {
       if (err == GRUB_ERR_OUT_OF_RANGE)
@@ -162,17 +169,24 @@ 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;
   grub_size_t max_stripes = 1;
+  grub_uint32_t sector;
 
   if (cargs->key_data == NULL || cargs->key_len == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
 
-  /* Detached headers are not implemented yet */
   if (cargs->hdr_file)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+    {
+      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
+	return grub_errno;
+
+      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
+	return grub_errno;
+    }
+  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;
 
@@ -227,13 +241,19 @@ 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 (cargs->hdr_file)
+      {
+        if (grub_file_seek (cargs->hdr_file, sector * 512) == (grub_off_t) -1)
+          return grub_errno;
+        if (grub_file_read (cargs->hdr_file, split_key, length) != (grub_ssize_t)length)
+          return grub_errno;
+      }
+      else
+        err = grub_disk_read (source, sector, 0, length, split_key);
       if (err)
 	{
 	  grub_free (split_key);
-- 
2.25.1

Re: [PATCH v9 4/7] cryptodisk: Add support for LUKS1 detached headers

[Daniel Kiper]

On Mon, Apr 11, 2022 at 06:40:25AM +0000, Glenn Washburn wrote:
> From: John Lane <john@lane.uk.net>
>
> cryptsetup supports having a detached header through the --header command
> line argument for both LUKS1 and LUKS2. Allow the LUKS1 backend to use a
> given file as the LUKS1 header (aka detached header) instead of looking for
> the header on the disk.
>
> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase, small fixes, commit message
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> development@efficientek.com: rebase, improve commit message
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/luks.c | 48 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index fe1655c3c2..195c4bb8da 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>
> @@ -73,17 +74,23 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>    char ciphername[sizeof (header.cipherName) + 1];
>    char ciphermode[sizeof (header.cipherMode) + 1];
>    char hashspec[sizeof (header.hashSpec) + 1];
> -  grub_err_t err;
> -
> -  /* Detached headers are not implemented yet */
> -  if (cargs->hdr_file)
> -    return NULL;
> +  grub_err_t err = GRUB_ERR_NONE;
>
>    if (cargs->check_boot)
>      return NULL;
>
>    /* Read the LUKS header.  */
> -  err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
> +  if (cargs->hdr_file)

I would prefer if you do "if (cargs->hdr_file != NULL)". Please fix this
in other places/patches too.

> +    {
> +      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
> +	return NULL;
> +
> +      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
> +	return NULL;
> +    }
> +  else
> +    err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
> +
>    if (err)
>      {
>        if (err == GRUB_ERR_OUT_OF_RANGE)
> @@ -162,17 +169,24 @@ 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;
>    grub_size_t max_stripes = 1;
> +  grub_uint32_t sector;
>
>    if (cargs->key_data == NULL || cargs->key_len == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
>
> -  /* Detached headers are not implemented yet */
>    if (cargs->hdr_file)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +    {
> +      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
> +	return grub_errno;
> +
> +      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
> +	return grub_errno;
> +    }
> +  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;
>
> @@ -227,13 +241,19 @@ 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 (cargs->hdr_file)
> +      {
> +        if (grub_file_seek (cargs->hdr_file, sector * 512) == (grub_off_t) -1)

s/512/1 << GRUB_LUKS1_LOG_SECTOR_SIZE/?

Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v9 5/7] cryptodisk: Add options to cryptomount to support keyfiles

[Glenn Washburn]

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

Add the options --key-file, --keyfile-offset, and --keyfile-size to
cryptomount and code to put read the requested key file data and pass
via the cargs struct. Note, key file data is for all intents and purposes
equivalent to a password given to cryptomount. So there is no need to
enable support for key files in the various crypto backends (eg. LUKS1)
because the key data is passed just as if it were a password.

Signed-off-by: John Lane <john@lane.uk.net>
GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
development@efficientek.com: rebase and rework to use cryptomount arg passing,
  minor fixes, improve commit message
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 85 ++++++++++++++++++++++++++++++++++++-
 include/grub/cryptodisk.h   |  2 +
 include/grub/file.h         |  2 +
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 063997d2f0..155cc7f0b4 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
     {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
     {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
     {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
+    {"key-file", '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}
   };
 
@@ -1185,6 +1188,84 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	return grub_errno;
     }
 
+  if (state[5].set) /* keyfile */
+    {
+      const char *p = NULL;
+      grub_file_t keyfile;
+      int keyfile_offset;
+      grub_size_t keyfile_size = 0;
+
+
+      if (state[6].set) /* keyfile-offset */
+	{
+	  keyfile_offset = grub_strtoul (state[6].arg, &p, 0);
+
+	  if (grub_errno != GRUB_ERR_NONE)
+	    return grub_errno;
+
+	  if (*p != '\0')
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       N_("unrecognized number"));
+	}
+      else
+	{
+	  keyfile_offset = 0;
+	}
+
+      if (state[7].set) /* keyfile-size */
+	{
+	  keyfile_size = grub_strtoul (state[7].arg, &p, 0);
+
+	  if (*p != '\0')
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       N_("unrecognized number"));
+
+	  if (grub_errno != GRUB_ERR_NONE)
+	    return grub_errno;
+
+	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
+	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			       N_("key file size exceeds maximum (%d)"),
+			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
+
+	  if (keyfile_size == 0)
+	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
+	}
+
+      keyfile = grub_file_open (state[5].arg,
+				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
+      if (keyfile == NULL)
+	return grub_errno;
+
+      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
+	return grub_errno;
+
+      if (keyfile_size > 0)
+	{
+	  if (keyfile_size > (keyfile->size - keyfile_offset))
+	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
+			       N_("keyfile is too small: "
+				  "requested %" PRIuGRUB_SIZE " bytes, "
+				  "but the file only has %" PRIuGRUB_UINT64_T
+				  " bytes"),
+			       keyfile_size,
+			       keyfile->size);
+
+	  cargs.key_len = keyfile_size;
+	}
+      else
+	{
+	  cargs.key_len = keyfile->size - keyfile_offset;
+	}
+
+      cargs.key_data = grub_malloc (cargs.key_len);
+      if (cargs.key_data == NULL)
+	return GRUB_ERR_OUT_OF_MEMORY;
+
+      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != (grub_ssize_t) cargs.key_len)
+	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file")));
+    }
+
   if (state[0].set) /* uuid */
     {
       int found_uuid;
@@ -1397,7 +1478,9 @@ GRUB_MOD_INIT (cryptodisk)
 {
   grub_disk_dev_register (&grub_cryptodisk_dev);
   cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
-			      N_("[-p password] [-H file] <SOURCE|-u UUID|-a|-b>"),
+			      N_("[ [-p password] | [-k keyfile"
+				 " [-O keyoffset] [-S keysize] ] ] [-H file]"
+				 " <SOURCE|-u UUID|-a|-b>"),
 			      N_("Mount a crypto device."), options);
   grub_procfs_register ("luks_script", &luks_script);
 }
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 9fe451de92..d94df68b65 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -62,6 +62,8 @@ typedef enum
 #define GRUB_CRYPTODISK_MAX_KEYLEN 128
 #define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
 
+#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
+
 struct grub_cryptodisk;
 
 typedef gcry_err_code_t
diff --git a/include/grub/file.h b/include/grub/file.h
index 3a3c49a04c..2d5d16cd22 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,
     /* File holding the encryption metadata header */
     GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER,
+    /* File holding the encryption key */
+    GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY,
     /* File we open n grub-fstest.  */
     GRUB_FILE_TYPE_FSTEST,
     /* File we open n grub-mount.  */
-- 
2.25.1

Re: [PATCH v9 5/7] cryptodisk: Add options to cryptomount to support keyfiles

[Daniel Kiper]

On Mon, Apr 11, 2022 at 06:40:26AM +0000, Glenn Washburn wrote:
> From: John Lane <john@lane.uk.net>
>
> Add the options --key-file, --keyfile-offset, and --keyfile-size to
> cryptomount and code to put read the requested key file data and pass
> via the cargs struct. Note, key file data is for all intents and purposes
> equivalent to a password given to cryptomount. So there is no need to
> enable support for key files in the various crypto backends (eg. LUKS1)
> because the key data is passed just as if it were a password.
>
> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> development@efficientek.com: rebase and rework to use cryptomount arg passing,
>   minor fixes, improve commit message
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 85 ++++++++++++++++++++++++++++++++++++-
>  include/grub/cryptodisk.h   |  2 +
>  include/grub/file.h         |  2 +
>  3 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 063997d2f0..155cc7f0b4 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
>      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
>      {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
>      {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> +    {"key-file", '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}
>    };
>
> @@ -1185,6 +1188,84 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>  	return grub_errno;
>      }
>
> +  if (state[5].set) /* keyfile */
> +    {
> +      const char *p = NULL;
> +      grub_file_t keyfile;
> +      int keyfile_offset;

I think this should be unsigned long if you do grub_strtoul() below.

> +      grub_size_t keyfile_size = 0;

I think this should be unsigned long too.

> +
> +

Please drop this extra line.

> +      if (state[6].set) /* keyfile-offset */
> +	{
> +	  keyfile_offset = grub_strtoul (state[6].arg, &p, 0);

Are you sure you want 0 base?

> +	  if (grub_errno != GRUB_ERR_NONE)
> +	    return grub_errno;
> +
> +	  if (*p != '\0')
> +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +			       N_("unrecognized number"));

This error check is unreliable. Please take a look at the commit
ac8a37dda (net/http: Allow use of non-standard TCP/IP ports) how it
should be done.

> +	}
> +      else
> +	{
> +	  keyfile_offset = 0;
> +	}

Why do not initialize it in definition above? If not please drop {}.

> +      if (state[7].set) /* keyfile-size */
> +	{
> +	  keyfile_size = grub_strtoul (state[7].arg, &p, 0);
> +
> +	  if (*p != '\0')
> +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +			       N_("unrecognized number"));
> +
> +	  if (grub_errno != GRUB_ERR_NONE)
> +	    return grub_errno;

Again, these checks are not reliable...

> +	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> +	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +			       N_("key file size exceeds maximum (%d)"),
> +			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> +
> +	  if (keyfile_size == 0)
> +	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
> +	}
> +
> +      keyfile = grub_file_open (state[5].arg,
> +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> +      if (keyfile == NULL)

Yeah, I like compare with NULL... :-)

> +	return grub_errno;
> +
> +      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)

Space before -1 please...

> +	return grub_errno;
> +
> +      if (keyfile_size > 0)
> +	{
> +	  if (keyfile_size > (keyfile->size - keyfile_offset))

What if somebody passes keyfile_offset larger than keyfile->size?
I would use grub_sub() here and check for underflow.

> +	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +			       N_("keyfile is too small: "
> +				  "requested %" PRIuGRUB_SIZE " bytes, "
> +				  "but the file only has %" PRIuGRUB_UINT64_T
> +				  " bytes"),
> +			       keyfile_size,
> +			       keyfile->size);
> +
> +	  cargs.key_len = keyfile_size;
> +	}
> +      else
> +	{
> +	  cargs.key_len = keyfile->size - keyfile_offset;

grub_sub() again?

> +	}

Please drop {} here...

> +      cargs.key_data = grub_malloc (cargs.key_len);
> +      if (cargs.key_data == NULL)
> +	return GRUB_ERR_OUT_OF_MEMORY;
> +
> +      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != (grub_ssize_t) cargs.key_len)
> +	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file")));
> +    }
> +
>    if (state[0].set) /* uuid */
>      {
>        int found_uuid;
> @@ -1397,7 +1478,9 @@ GRUB_MOD_INIT (cryptodisk)
>  {
>    grub_disk_dev_register (&grub_cryptodisk_dev);
>    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> -			      N_("[-p password] [-H file] <SOURCE|-u UUID|-a|-b>"),
> +			      N_("[ [-p password] | [-k keyfile"
> +				 " [-O keyoffset] [-S keysize] ] ] [-H file]"
> +				 " <SOURCE|-u UUID|-a|-b>"),
>  			      N_("Mount a crypto device."), options);
>    grub_procfs_register ("luks_script", &luks_script);
>  }
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 9fe451de92..d94df68b65 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -62,6 +62,8 @@ typedef enum
>  #define GRUB_CRYPTODISK_MAX_KEYLEN 128
>  #define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
>
> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192

This constant is not used here. I think it should be used in this patch
to check limits. Probably somewhere around proposed grub_sub(). Than
maybe we do not need grub_sub().

Daniel

Re: [PATCH v9 5/7] cryptodisk: Add options to cryptomount to support keyfiles

[Glenn Washburn]

On Fri, 29 Apr 2022 15:03:01 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Apr 11, 2022 at 06:40:26AM +0000, Glenn Washburn wrote:
> > From: John Lane <john@lane.uk.net>
> >
> > Add the options --key-file, --keyfile-offset, and --keyfile-size to
> > cryptomount and code to put read the requested key file data and pass
> > via the cargs struct. Note, key file data is for all intents and purposes
> > equivalent to a password given to cryptomount. So there is no need to
> > enable support for key files in the various crypto backends (eg. LUKS1)
> > because the key data is passed just as if it were a password.
> >
> > Signed-off-by: John Lane <john@lane.uk.net>
> > GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
> > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> > development@efficientek.com: rebase and rework to use cryptomount arg passing,
> >   minor fixes, improve commit message
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 85 ++++++++++++++++++++++++++++++++++++-
> >  include/grub/cryptodisk.h   |  2 +
> >  include/grub/file.h         |  2 +
> >  3 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 063997d2f0..155cc7f0b4 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
> >      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> >      {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
> >      {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> > +    {"key-file", '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}
> >    };
> >
> > @@ -1185,6 +1188,84 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >  	return grub_errno;
> >      }
> >
> > +  if (state[5].set) /* keyfile */
> > +    {
> > +      const char *p = NULL;
> > +      grub_file_t keyfile;
> > +      int keyfile_offset;
> 
> I think this should be unsigned long if you do grub_strtoul() below.
> 
> > +      grub_size_t keyfile_size = 0;
> 
> I think this should be unsigned long too.

Ok.

> > +
> > +
> 
> Please drop this extra line.
> 
> > +      if (state[6].set) /* keyfile-offset */
> > +	{
> > +	  keyfile_offset = grub_strtoul (state[6].arg, &p, 0);
> 
> Are you sure you want 0 base?

Having base 0 tells grub_strtoull that it should guess the base, which
means base 16 if a "0x" prefix, base 8 if "0" prefix, and base 10
otherwise. This is conventient for the user.

> > +	  if (grub_errno != GRUB_ERR_NONE)
> > +	    return grub_errno;
> > +
> > +	  if (*p != '\0')
> > +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +			       N_("unrecognized number"));
> 
> This error check is unreliable. Please take a look at the commit
> ac8a37dda (net/http: Allow use of non-standard TCP/IP ports) how it
> should be done.

Ok.

> > +	}
> > +      else
> > +	{
> > +	  keyfile_offset = 0;
> > +	}
> 
> Why do not initialize it in definition above? If not please drop {}.

I guess it was to avoid an unnecessary write if the if is true. But
yeah, I don't think it gets you much.

> > +      if (state[7].set) /* keyfile-size */
> > +	{
> > +	  keyfile_size = grub_strtoul (state[7].arg, &p, 0);
> > +
> > +	  if (*p != '\0')
> > +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +			       N_("unrecognized number"));
> > +
> > +	  if (grub_errno != GRUB_ERR_NONE)
> > +	    return grub_errno;
> 
> Again, these checks are not reliable...

Are you also saying grub_errno shouldn't be checked here? That doesn't
seem correct to me.

> > +	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)

This is where the macro at the end of the patch is used.

> > +	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > +			       N_("key file size exceeds maximum (%d)"),
> > +			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> > +
> > +	  if (keyfile_size == 0)
> > +	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
> > +	}
> > +
> > +      keyfile = grub_file_open (state[5].arg,
> > +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> > +      if (keyfile == NULL)
> 
> Yeah, I like compare with NULL... :-)
> 
> > +	return grub_errno;
> > +
> > +      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
> 
> Space before -1 please...

Ok.

> > +	return grub_errno;
> > +
> > +      if (keyfile_size > 0)
> > +	{
> > +	  if (keyfile_size > (keyfile->size - keyfile_offset))
> 
> What if somebody passes keyfile_offset larger than keyfile->size?
> I would use grub_sub() here and check for underflow.

Good point, I'll clamp it down to keyfile->size above.

> > +	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> > +			       N_("keyfile is too small: "
> > +				  "requested %" PRIuGRUB_SIZE " bytes, "
> > +				  "but the file only has %" PRIuGRUB_UINT64_T
> > +				  " bytes"),
> > +			       keyfile_size,
> > +			       keyfile->size);
> > +
> > +	  cargs.key_len = keyfile_size;
> > +	}
> > +      else
> > +	{
> > +	  cargs.key_len = keyfile->size - keyfile_offset;
> 
> grub_sub() again?

With keyfile_offset <= keyfile->size, this shouldn't be a problem.

> > +	}
> 
> Please drop {} here...

Yep.

> > +      cargs.key_data = grub_malloc (cargs.key_len);
> > +      if (cargs.key_data == NULL)
> > +	return GRUB_ERR_OUT_OF_MEMORY;
> > +
> > +      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != (grub_ssize_t) cargs.key_len)
> > +	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file")));
> > +    }
> > +
> >    if (state[0].set) /* uuid */
> >      {
> >        int found_uuid;
> > @@ -1397,7 +1478,9 @@ GRUB_MOD_INIT (cryptodisk)
> >  {
> >    grub_disk_dev_register (&grub_cryptodisk_dev);
> >    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> > -			      N_("[-p password] [-H file] <SOURCE|-u UUID|-a|-b>"),
> > +			      N_("[ [-p password] | [-k keyfile"
> > +				 " [-O keyoffset] [-S keysize] ] ] [-H file]"
> > +				 " <SOURCE|-u UUID|-a|-b>"),
> >  			      N_("Mount a crypto device."), options);
> >    grub_procfs_register ("luks_script", &luks_script);
> >  }
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index 9fe451de92..d94df68b65 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -62,6 +62,8 @@ typedef enum
> >  #define GRUB_CRYPTODISK_MAX_KEYLEN 128
> >  #define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
> >
> > +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
> 
> This constant is not used here. I think it should be used in this patch
> to check limits. Probably somewhere around proposed grub_sub(). Than
> maybe we do not need grub_sub().

Yep, its used, see above.

Glenn

[PATCH v9 6/7] luks2: Add detached header support

[Glenn Washburn]

If a header file is given to the LUKS2 backend, use that file as the LUKS2
header, instead of looking for it on the disk.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 67 ++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 349462c61a..af5bc4fc82 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -313,13 +313,22 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s
 
 /* Determine whether to use primary or secondary header */
 static grub_err_t
-luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
+luks2_read_header (grub_disk_t disk, grub_file_t hdr_file, grub_luks2_header_t *outhdr)
 {
   grub_luks2_header_t primary, secondary, *header = &primary;
-  grub_err_t ret;
+  grub_err_t ret = GRUB_ERR_NONE;
 
   /* Read the primary LUKS header. */
-  ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
+  if (hdr_file)
+    {
+      if (grub_file_seek (hdr_file, 0) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (hdr_file, &primary, sizeof (primary)) != sizeof (primary))
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
   if (ret)
     return ret;
 
@@ -329,7 +338,16 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
     return GRUB_ERR_BAD_SIGNATURE;
 
   /* Read the secondary header. */
-  ret = grub_disk_read (disk, 0, grub_be_to_cpu64 (primary.hdr_size), sizeof (secondary), &secondary);
+  if (hdr_file)
+    {
+      if (grub_file_seek (hdr_file, grub_be_to_cpu64 (primary.hdr_size)) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (hdr_file, &secondary, sizeof (secondary)) != sizeof (secondary))
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (disk, 0, grub_be_to_cpu64 (primary.hdr_size), sizeof (secondary), &secondary);
   if (ret)
     return ret;
 
@@ -353,14 +371,10 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   char uuid[sizeof (header.uuid) + 1];
   grub_size_t i, j;
 
-  /* Detached headers are not implemented yet */
-  if (cargs->hdr_file)
-    return NULL;
-
   if (cargs->check_boot)
     return NULL;
 
-  if (luks2_read_header (disk, &header))
+  if (luks2_read_header (disk, cargs->hdr_file, &header))
     {
       grub_errno = GRUB_ERR_NONE;
       return NULL;
@@ -427,6 +441,7 @@ luks2_verify_key (grub_luks2_digest_t *d, grub_uint8_t *candidate_key,
 static grub_err_t
 luks2_decrypt_key (grub_uint8_t *out_key,
 		   grub_disk_t source, grub_cryptodisk_t crypt,
+		   grub_cryptomount_args_t cargs,
 		   grub_luks2_keyslot_t *k,
 		   const grub_uint8_t *passphrase, grub_size_t passphraselen)
 {
@@ -502,7 +517,17 @@ luks2_decrypt_key (grub_uint8_t *out_key,
     }
 
   grub_errno = GRUB_ERR_NONE;
-  ret = grub_disk_read (source, 0, k->area.offset, k->area.size, split_key);
+  if (cargs->hdr_file)
+    {
+      if (grub_file_seek (cargs->hdr_file, k->area.offset) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (cargs->hdr_file, split_key, k->area.size) != k->area.size)
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (source, 0, k->area.offset, k->area.size, split_key);
+
   if (ret)
     {
       grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
@@ -564,11 +589,7 @@ luks2_recover_key (grub_disk_t source,
   if (cargs->key_data == NULL || cargs->key_len == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
 
-  /* Detached headers are not implemented yet */
-  if (cargs->hdr_file)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
-
-  ret = luks2_read_header (source, &header);
+  ret = luks2_read_header (source, cargs->hdr_file, &header);
   if (ret)
     return ret;
 
@@ -577,8 +598,18 @@ luks2_recover_key (grub_disk_t source,
       return GRUB_ERR_OUT_OF_MEMORY;
 
   /* Read the JSON area. */
-  ret = grub_disk_read (source, 0, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header),
-			grub_be_to_cpu64 (header.hdr_size) - sizeof (header), json_header);
+  if (cargs->hdr_file)
+    {
+      if (grub_file_seek (cargs->hdr_file, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header)) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (cargs->hdr_file, json_header, grub_be_to_cpu64 (header.hdr_size) - sizeof (header)) != (grub_be_to_cpu64 (header.hdr_size) - sizeof (header)))
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (source, 0, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header),
+			  grub_be_to_cpu64 (header.hdr_size) - sizeof (header), json_header);
+
   if (ret)
       goto err;
 
@@ -716,7 +747,7 @@ luks2_recover_key (grub_disk_t source,
 	  crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors;
 	}
 
-      ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
+      ret = luks2_decrypt_key (candidate_key, source, crypt, cargs, &keyslot,
 			       cargs->key_data, cargs->key_len);
       if (ret)
 	{
-- 
2.25.1

Re: [PATCH v9 6/7] luks2: Add detached header support

[Daniel Kiper]

On Mon, Apr 11, 2022 at 06:40:27AM +0000, Glenn Washburn wrote:
> If a header file is given to the LUKS2 backend, use that file as the LUKS2
> header, instead of looking for it on the disk.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/luks2.c | 67 ++++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 349462c61a..af5bc4fc82 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -313,13 +313,22 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s
>
>  /* Determine whether to use primary or secondary header */
>  static grub_err_t
> -luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
> +luks2_read_header (grub_disk_t disk, grub_file_t hdr_file, grub_luks2_header_t *outhdr)
>  {
>    grub_luks2_header_t primary, secondary, *header = &primary;
> -  grub_err_t ret;
> +  grub_err_t ret = GRUB_ERR_NONE;
>
>    /* Read the primary LUKS header. */
> -  ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
> +  if (hdr_file)

if (hdr_file != NULL) and below please...

> +    {
> +      if (grub_file_seek (hdr_file, 0) == (grub_off_t) -1)
> +	ret = grub_errno;

Hmmm... Why do not "return grub_errno;"?

> +
> +      else if (grub_file_read (hdr_file, &primary, sizeof (primary)) != sizeof (primary))
> +	ret = grub_errno;

Ditto. And then you can drop "else"...

> +    }
> +  else
> +    ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
>    if (ret)

... and this "if" ... Or to be precise add {} after else and convert to
"if (ret != GRUB_ERR_NONE)".

And it seems to me you can do similar optimizations below.

Daniel

[PATCH v9 7/7] docs: Add documentation on keyfile and detached header options to cryptomount

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 docs/grub.texi | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 9835c878af..c1bf532636 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4347,11 +4347,17 @@ Alias for @code{hashsum --hash crc32 arg @dots{}}. See command @command{hashsum}
 @node cryptomount
 @subsection cryptomount
 
-@deffn Command cryptomount [@option{-p} password] device|@option{-u} uuid|@option{-a}|@option{-b}
-Setup access to encrypted device. If @option{-p} is not given, a passphrase
-is requested interactively. Otherwise, the given @var{password} will be used and
-no passphrase will be requested interactively.
-Option @var{device} configures specific grub device
+@deffn Command cryptomount [ [@option{-p} password] | [@option{-k} keyfile [@option{-O} keyoffset] [@option{-S} keysize] ] ] [@option{-H} file] device|@option{-u} uuid|@option{-a}|@option{-b}
+Setup access to encrypted device. A passphrase will be requested interactively,
+if neither the @option{-p} nor @option{-k} options are given. The option
+@option{-p} can be used to supply a passphrase (useful for scripts).
+Alternatively the @option{-k} option can be used to supply a keyfile with
+options @option{-O} and @option{-S} optionally supplying the offset and size,
+respectively, of the key data in the given key file. The @option{-H} options can
+be used to supply cryptomount backends with an alternative header file (aka
+detached header). Not all backends have headers nor support alternative header
+files (currently only LUKS1 and LUKS2 support them).
+Argument @var{device} configures specific grub device
 (@pxref{Naming convention}); option @option{-u} @var{uuid} configures device
 with specified @var{uuid}; option @option{-a} configures all detected encrypted
 devices; option @option{-b} configures all geli containers that have boot flag set.
-- 
2.25.1

Re: [PATCH v9 7/7] docs: Add documentation on keyfile and detached header options to cryptomount

[Daniel Kiper]

On Mon, Apr 11, 2022 at 06:40:28AM +0000, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel