[PATCH v6 1/1] plainmount: Support plain encryption mode

[PATCH v6 1/1] plainmount: Support plain encryption mode

[Maxim Fomin]

From 683357e227467c05272facc7da534a82becc9d8a Mon Sep 17 00:00:00 2001
From: Maxim Fomin <maxim@fomin.one>
Date: Sun, 31 Jul 2022 14:06:42 +0100
Subject: [PATCH v6 1/1] plainmount: Support plain encryption mode

This patch adds support for plain encryption mode (plain dm-crypt) via
new module/command named 'plainmount'.

Signed-off-by: Maxim Fomin <maxim@fomin.one>
---
 docs/grub.texi              |  80 +++++++
 grub-core/Makefile.core.def |   5 +
 grub-core/disk/plainmount.c | 450 ++++++++++++++++++++++++++++++++++++
 3 files changed, 535 insertions(+)
 create mode 100644 grub-core/disk/plainmount.c

diff --git a/docs/grub.texi b/docs/grub.texi
index af119dea3..22c73580c 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4265,6 +4265,7 @@ you forget a command, you can run the command @command{help}
 * parttool::                    Modify partition table entries
 * password::                    Set a clear-text password
 * password_pbkdf2::             Set a hashed password
+* plainmount::                  Open device encrypted in plain mode
 * play::                        Play a tune
 * probe::                       Retrieve device info
 * rdmsr::                       Read values from model-specific registers
@@ -4552,6 +4553,14 @@ function is supported, as Argon2 is not yet supported.

 Also, note that, unlike filesystem UUIDs, UUIDs for encrypted devices must be
 specified without dash separators.
+
+Successfully decrypted disks are named as (cryptoX) and have increasing numeration
+suffix for each new decrypted disk. If the encrypted disk hosts some higher level
+of abstraction (like LVM2 or MDRAID) it will be created under a separate device
+namespace in addition to the cryptodisk namespace.
+
+Support for plain encryption mode (plain dm-crypt) is provided via separate
+@command{@pxref{plainmount}} command.
 @end deffn

 @node cutmem
@@ -5060,6 +5069,77 @@ to generate password hashes.  @xref{Security}.
 @end deffn


+@node plainmount
+@subsection plainmount
+
+@deffn Command plainmount device @option{-c} cipher @option{-s} key size [@option{-h} hash]
+[@option{-S} sector size] [@option{-p} password] [@option{-u} uuid]
+[[@option{-d} keyfile] [@option{-O} keyfile offset]]
+
+
+Setup access to the encrypted device in plain mode. Offset of the encrypted
+data at the device is specified in terms of 512 byte sectors with the blocklist
+syntax and loopback device. The following example shows how to specify 1MiB
+offset:
+
+@example
+loopback node (hd0,gpt1)2048+
+plainmount node
+@end example
+
+The @command{plainmount} command can be used to open LUKS encrypted volume
+if its master key and parameters (key size, cipher, offset, etc) are known.
+
+There are two ways to specify password: a keyfile and a secret passphrase.
+Keyfile path parameter has higher priority than secret passphrase and is
+specified with the option @option{-d}. Password data obtained from keyfiles
+is not hashed and is used directly as a cipher key. Optional offset of password
+data in the keyfile can be specified with the option @option{-O} or directly
+with the option @option{-d} and GRUB blocklist syntax. The following example
+shows both methods to specify password data in the keyfile at offset 1MiB:
+
+@example
+plainmount -d (hd0,gpt1)2048+
+plainmount -d (hd0,gpt1) -O 1048576
+@end example
+
+If no keyfile is specified then the password is set to data specified by
+option @option{-p} or is requested interactively from the console. In both
+cases the provided password is hashed with the algorithm specified by the
+option @option{-h}. This option is mandatory if no keyfile is specified, but
+it can be set to @code{plain} which means that no hashing is done and such
+password is used directly as a key.
+
+Cipher @option{-c} and keysize @option{-s} options specify the cipher
+algorithm and the key size respectively and are mandatory options. Cipher
+must be specified with the mode separated by a dash (for example,
+'aes-xts-plain64'). Key size option @option{-s} is the key size of the cipher,
+not to be confused with the offset of the key data in a keyfile specified with
+the @option{-O} option. It must not exceed 128 bytes and must be specified in
+bits (so a 32 byte key would be specified as 256 bits).
+
+The optional parameter @option{-S} specifies encrypted device sector size. It
+must be at least 512 bytes long (default value) and a power of 2. @footnote{Current
+implementation of cryptsetup supports only 512/1024/2048/4096 byte sectors}.
+Disk sector size is configured when creating the encrypted volume. Attempting
+to decrypt volumes with a different sector size than it was created with will
+not result in an error, but will decrypt to random bytes and thus prevent
+accessing the volume (in some cases filesystem driver can detect the filesystem
+presense, but nevertheless will refuse to mount it).
+
+By default new plainmount devices will be given a UUID starting with
+'109fea84-a6b7-34a8-4bd1-1c506305a401' where the last digits are incremented
+by one for each plainmounted device beyond the first up to 2^10 devices.
+
+All encryption arguments (cipher, hash, key size, disk offset and disk sector
+size) must match the parameters used to create the volume. If any of them does
+not match the actual arguments used during the initial encryption, plainmount
+will create virtual device with the garbage data and GRUB will report unknown
+filesystem for such device. Writing data to such virtual device will result in
+the data loss if the underlying partition contained desired data.
+@end deffn
+
+
 @node play
 @subsection play

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 715994872..3910b7670 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1174,6 +1174,11 @@ module = {
   common = disk/cryptodisk.c;
 };

+module = {
+  name = plainmount;
+  common = disk/plainmount.c;
+};
+
 module = {
   name = json;
   common = lib/json/json.c;
diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
new file mode 100644
index 000000000..8b735a425
--- /dev/null
+++ b/grub-core/disk/plainmount.c
@@ -0,0 +1,450 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2022  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/* plaimount.c - Open device encrypted in plain mode. */
+
+#include <grub/cryptodisk.h>
+#include <grub/dl.h>
+#include <grub/err.h>
+#include <grub/extcmd.h>
+#include <grub/partition.h>
+#include <grub/file.h>
+
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
+#define PLAINMOUNT_DEFAULT_UUID        "109fea84-a6b7-34a8-4bd1-1c506305a400"
+
+
+enum PLAINMOUNT_OPTION
+  {
+    OPTION_HASH,
+    OPTION_CIPHER,
+    OPTION_KEY_SIZE,
+    OPTION_SECTOR_SIZE,
+    OPTION_PASSWORD,
+    OPTION_KEYFILE,
+    OPTION_KEYFILE_OFFSET,
+    OPTION_UUID
+  };
+
+
+static const struct grub_arg_option options[] =
+  {
+    /* TRANSLATORS: It's still restricted to this module only.  */
+    {"hash", 'h', 0, N_("Password hash"), 0, ARG_TYPE_STRING},
+    {"cipher", 'c', 0, N_("Password cipher"), 0, ARG_TYPE_STRING},
+    {"key-size", 's', 0, N_("Key size (in bits)"), 0, ARG_TYPE_INT},
+    {"sector-size", 'S', 0, N_("Device sector size"), 0, ARG_TYPE_INT},
+    {"password", 'p', 0, N_("Password (key)"), 0, ARG_TYPE_STRING},
+    {"keyfile", 'd', 0, N_("Keyfile path"), 0, ARG_TYPE_STRING},
+    {"keyfile-offset", 'O', 0, N_("Keyfile offset"), 0, ARG_TYPE_INT},
+    {"uuid", 'u', 0, N_("Set device UUID"), 0, ARG_TYPE_STRING},
+    {0, 0, 0, 0, 0, 0}
+  };
+
+
+/* Cryptodisk setkey() function wrapper */
+static grub_err_t
+plainmount_setkey (grub_cryptodisk_t dev, grub_uint8_t *key,
+		   grub_size_t size)
+{
+  gcry_err_code_t code = grub_cryptodisk_setkey (dev, key, size);
+  if (code != GPG_ERR_NO_ERROR)
+    {
+      grub_dprintf ("plainmount", "failed to set cipher key with code: %d\n", code);
+      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot set specified key"));
+    }
+  return GRUB_ERR_NONE;
+}
+
+
+/* Configure cryptodisk uuid */
+static void plainmount_set_uuid (grub_cryptodisk_t dev, const char *user_uuid)
+{
+  grub_size_t pos = 0;
+
+  /* Size of user_uuid is checked in main func */
+  if (user_uuid != NULL)
+      grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
+  else
+    {
+      /*
+       * Set default UUID. Last digits start from 1 and are incremented for
+       * each new plainmount device by snprintf().
+       */
+      grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%36lx", dev->id+1);
+      while (dev->uuid[++pos] == ' ');
+      grub_memcpy (dev->uuid, PLAINMOUNT_DEFAULT_UUID, pos);
+    }
+  COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (PLAINMOUNT_DEFAULT_UUID));
+}
+
+
+/* Configure cryptodevice sector size (-S option) */
+static grub_err_t
+plainmount_configure_sectors (grub_cryptodisk_t dev, grub_disk_t disk,
+			      grub_size_t sector_size)
+{
+  dev->total_sectors = grub_disk_native_sectors (disk);
+  if (dev->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
+    return grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot determine disk %s size"),
+		       disk->name);
+
+  /* Convert size to sectors */
+  dev->log_sector_size = grub_log2ull (sector_size);
+  dev->total_sectors = grub_convert_sector (dev->total_sectors,
+					    GRUB_DISK_SECTOR_BITS,
+					    dev->log_sector_size);
+  if (dev->total_sectors == 0)
+    return grub_error (GRUB_ERR_BAD_DEVICE,
+		       N_("cannot set specified sector size on disk %s"),
+		       disk->name);
+
+  grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"
+		PRIuGRUB_SIZE"\n", dev->log_sector_size, dev->total_sectors);
+  return GRUB_ERR_NONE;
+}
+
+
+/* Hashes a password into a key and stores it with cipher. */
+static grub_uint8_t*
+plainmount_configure_password (grub_cryptodisk_t dev, const char *hash,
+			       grub_uint8_t *key_data, grub_size_t key_size)
+{
+  grub_uint8_t *derived_hash, *dh;
+  char *p;
+  unsigned int round, i;
+  unsigned int len, size;
+
+  /* Support none (plain) hash */
+  if (grub_strcmp (hash, "plain") == 0)
+    {
+      dev->hash = NULL;
+      return key_data;
+    }
+
+  /* Hash argument was checked at main func */
+  dev->hash = grub_crypto_lookup_md_by_name (hash);
+  len = dev->hash->mdlen;
+  p = grub_malloc (key_size + 2 + (key_size / len));
+  derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
+  if (p == NULL || derived_hash == NULL)
+    {
+      grub_free (p);
+      grub_free (derived_hash);
+      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
+      return NULL;
+    }
+  dh = derived_hash;
+
+  /*
+   * Hash password. Adapted from cryptsetup.
+   * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
+   */
+  for (round = 0, size = key_size; size; round++, dh += len, size -= len)
+    {
+      for (i = 0; i < round; i++)
+	p[i] = 'A';
+
+      grub_strcpy (p + i, (char*) key_data);
+
+      if (len > size)
+	len = size;
+
+      grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
+    }
+  grub_free (p);
+  return derived_hash;
+}
+
+
+/* Read key material from keyfile */
+static grub_err_t
+plainmount_configure_keyfile (char *keyfile, grub_uint8_t *key_data,
+			      grub_size_t key_size, grub_size_t keyfile_offset)
+{
+  grub_file_t g_keyfile = grub_file_open (keyfile, GRUB_FILE_TYPE_NONE);
+  if (g_keyfile == NULL)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile %s"),
+		       keyfile);
+
+  if (grub_file_seek (g_keyfile, keyfile_offset) == (grub_off_t)-1)
+    return grub_error (GRUB_ERR_FILE_READ_ERROR,
+		       N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
+		       keyfile_offset);
+
+  if (key_size > (g_keyfile->size - keyfile_offset))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Specified key size (%"
+		       PRIuGRUB_SIZE") is too small for keyfile size (%"
+		       PRIuGRUB_SIZE") and offset (%"PRIuGRUB_SIZE")"),
+		       key_size, g_keyfile->size, keyfile_offset);
+
+  if (grub_file_read (g_keyfile, key_data, key_size) != (grub_ssize_t) key_size)
+    return grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading key file"));
+  return GRUB_ERR_NONE;
+}
+
+
+/* Plainmount command entry point */
+static grub_err_t
+grub_cmd_plainmount (grub_extcmd_context_t ctxt, int argc, char **args)
+{
+  struct grub_arg_list *state = ctxt->state;
+  grub_cryptodisk_t dev = NULL;
+  grub_disk_t disk = NULL;
+  const gcry_md_spec_t *gcry_hash;
+  char *diskname, *disklast = NULL, *cipher, *mode, *hash, *keyfile, *uuid;
+  grub_size_t len, key_size, sector_size, keyfile_offset = 0;
+  grub_err_t err;
+  const char *p;
+  grub_uint8_t *key_data;
+  grub_uint8_t *hashed_key_data;
+
+  if (argc < 1)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
+
+  /* Check whether required arguments are specified */
+  if (!state[OPTION_CIPHER].set || !state[OPTION_KEY_SIZE].set)
+      return grub_error (GRUB_ERR_BAD_ARGUMENT, "cipher and key size must be set");
+  if (!state[OPTION_HASH].set && !state[OPTION_KEYFILE].set)
+      return grub_error (GRUB_ERR_BAD_ARGUMENT, "hash algorithm must be set");
+
+  /* Check hash */
+  if (!state[OPTION_KEYFILE].set)
+  {
+    gcry_hash = grub_crypto_lookup_md_by_name (state[OPTION_HASH].arg);
+    if (!gcry_hash)
+      return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("couldn't load hash %s"),
+			 state[OPTION_HASH].arg);
+
+    if (gcry_hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
+      return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			 N_("hash length %"PRIuGRUB_SIZE" exceeds maximum %d bits"),
+			 gcry_hash->mdlen * GRUB_CHAR_BIT,
+			 GRUB_CRYPTODISK_MAX_KEYLEN * GRUB_CHAR_BIT);
+   }
+
+  /* Check cipher mode */
+  if (!grub_strchr (state[OPTION_CIPHER].arg,'-'))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       N_("invalid cipher mode, must be of format cipher-mode"));
+
+  /* Check password size */
+  if (state[OPTION_PASSWORD].set && grub_strlen (state[OPTION_PASSWORD].arg) >
+		                                 GRUB_CRYPTODISK_MAX_PASSPHRASE)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       N_("password exceeds maximium size"));
+
+  /* Check uuid length */
+  if (state[OPTION_UUID].set && grub_strlen (state[OPTION_UUID].arg) >
+				sizeof (PLAINMOUNT_DEFAULT_UUID))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       N_("specified UUID exceeds maximum size"));
+
+  /* Parse plainmount arguments */
+  grub_errno = GRUB_ERR_NONE;
+  keyfile_offset = state[OPTION_KEYFILE_OFFSET].set ?
+		   grub_strtoull (state[OPTION_KEYFILE_OFFSET].arg, &p, 0) : 0;
+  if (state[OPTION_KEYFILE_OFFSET].set &&
+     (state[OPTION_KEYFILE_OFFSET].arg[0] == '\0' || *p != '\0' ||
+      grub_errno != GRUB_ERR_NONE))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized keyfile offset"));
+
+  sector_size = state[OPTION_SECTOR_SIZE].set ?
+		grub_strtoull (state[OPTION_SECTOR_SIZE].arg, &p, 0) :
+		PLAINMOUNT_DEFAULT_SECTOR_SIZE;
+  if (state[OPTION_SECTOR_SIZE].set && (state[OPTION_SECTOR_SIZE].arg[0] == '\0' ||
+					*p != '\0' || grub_errno != GRUB_ERR_NONE))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector size"));
+
+  /* Check key size */
+  key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0);
+  if (state[OPTION_KEY_SIZE].arg[0] == '\0' || *p != '\0' ||
+      grub_errno != GRUB_ERR_NONE)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
+  if (key_size % GRUB_CHAR_BIT != 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       N_("key size is not multiple of %d bits"), GRUB_CHAR_BIT);
+  key_size = key_size / GRUB_CHAR_BIT;
+  if (key_size > GRUB_CRYPTODISK_MAX_KEYLEN)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       N_("key size %"PRIuGRUB_SIZE" exceeds maximum %d bits"),
+		       key_size * GRUB_CHAR_BIT,
+		       GRUB_CRYPTODISK_MAX_KEYLEN * GRUB_CHAR_BIT);
+
+  /* Check disk sector size */
+  if (sector_size < GRUB_DISK_SECTOR_SIZE)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       N_("sector size -S must be at least %d"),
+		       GRUB_DISK_SECTOR_SIZE);
+  if ((sector_size & (sector_size - 1)) != 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+		       N_("sector size -S %"PRIuGRUB_SIZE" is not power of 2"),
+		       sector_size);
+
+  /* Allocate all stuff here */
+  hash =  state[OPTION_HASH].set ? grub_strdup (state[OPTION_HASH].arg) : NULL;
+  cipher = grub_strdup (state[OPTION_CIPHER].arg);
+  keyfile = state[OPTION_KEYFILE].set ?
+	    grub_strdup (state[OPTION_KEYFILE].arg) : NULL;
+  dev = grub_zalloc (sizeof *dev);
+  key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+  uuid = state[OPTION_UUID].set ? grub_strdup (state[OPTION_UUID].arg) : NULL;
+  if ((hash == NULL && state[OPTION_HASH].set) || cipher == NULL || dev == NULL ||
+      (keyfile == NULL && state[OPTION_KEYFILE].set) || key_data == NULL ||
+      (uuid == NULL && state[OPTION_UUID].set))
+    {
+      err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
+      goto exit;
+    }
+
+  /* Copy user password from -p option */
+  if (state[OPTION_PASSWORD].set)
+    grub_memcpy (key_data, state[OPTION_PASSWORD].arg,
+		 grub_strlen (state[OPTION_PASSWORD].arg));
+
+  /* Copy user UUID from -u option */
+  if (state[OPTION_UUID].set)
+    grub_memcpy (uuid, state[OPTION_UUID].arg,
+		 grub_strlen (state[OPTION_UUID].arg));
+
+  /* Set cipher mode (tested above) */
+  mode = grub_strchr (cipher,'-');
+  *mode++ = '\0';
+
+  /* Check cipher */
+  if (grub_cryptodisk_setcipher (dev, cipher, mode) != GRUB_ERR_NONE)
+    {
+      err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher %s"), cipher);
+      goto exit;
+    }
+
+  /* Open SOURCE disk */
+  diskname = args[0];
+  len = grub_strlen (diskname);
+  if (len && diskname[0] == '(' && diskname[len - 1] == ')')
+    {
+      disklast = &diskname[len - 1];
+      *disklast = '\0';
+      diskname++;
+    }
+  disk = grub_disk_open (diskname);
+  if (disk == NULL)
+    {
+      if (disklast)
+        *disklast = ')';
+      err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot open disk %s"), diskname);
+      goto exit;
+    }
+
+  /* Get password from console */
+  if (keyfile == NULL && key_data[0] == '\0')
+  {
+    char *part = grub_partition_get_name (disk->partition);
+    grub_printf_ (N_("Enter passphrase for %s%s%s: "), disk->name,
+		  disk->partition != NULL ? "," : "",
+		  part != NULL ? part : N_("UNKNOWN"));
+    grub_free (part);
+
+    if (!grub_password_get ((char*)key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE-1))
+        grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error reading password"));
+  }
+
+  /* Warn if hash and keyfile are both provided */
+  if (keyfile != NULL && state[OPTION_HASH].arg)
+    grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
+
+  /* Warn if -p option is specified with keyfile */
+  if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
+    grub_printf_ (N_("warning: password specified with -p option "
+		     "is ignored if keyfile is provided\n"));
+
+  /* Warn of -O is provided without keyfile */
+  if (state[OPTION_KEYFILE_OFFSET].set && !state[OPTION_KEYFILE].set)
+    grub_printf_ (N_("warning: keyfile offset option -O "
+		     "specified without keyfile option -d\n"));
+
+  grub_dprintf ("plainmount", "parameters: cipher=%s, hash=%s, key_size=%"
+		PRIuGRUB_SIZE", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
+		cipher, hash, key_size, keyfile, keyfile_offset);
+
+  err = plainmount_configure_sectors (dev, disk, sector_size);
+  if (err != GRUB_ERR_NONE)
+    goto exit;
+
+  /* Configure keyfile or password */
+  if (keyfile != NULL)
+    {
+      err = plainmount_configure_keyfile (keyfile, key_data, key_size, keyfile_offset);
+      if (err != GRUB_ERR_NONE)
+        goto exit;
+      err = plainmount_setkey (dev, key_data, key_size);
+      if (err != GRUB_ERR_NONE)
+        goto exit;
+    }
+  else
+    {
+      hashed_key_data = plainmount_configure_password (dev, hash, key_data, key_size);
+      if (hashed_key_data == NULL)
+        goto exit;
+      err = plainmount_setkey (dev, hashed_key_data, key_size);
+      if (err != GRUB_ERR_NONE)
+      {
+        grub_free (hashed_key_data);
+        goto exit;
+      }
+    }
+
+  err = grub_cryptodisk_insert (dev, diskname, disk);
+  if (err != GRUB_ERR_NONE)
+    goto exit;
+
+  dev->modname = "plainmount";
+  dev->source_disk = disk;
+  plainmount_set_uuid (dev, uuid);
+
+exit:
+  grub_free (hash);
+  grub_free (cipher);
+  grub_free (keyfile);
+  grub_free (key_data);
+  grub_free (uuid);
+  if (err != GRUB_ERR_NONE && disk)
+    grub_disk_close (disk);
+  if (err != GRUB_ERR_NONE && dev)
+    grub_free (dev);
+  return err;
+}
+
+static grub_extcmd_t cmd;
+GRUB_MOD_INIT (plainmount)
+{
+  cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
+			      N_("-c cipher -s key-size [-h hash] [-S sector-size]"
+			      " [-o offset] [-p password] [-u uuid] "
+			      " [[-d keyfile] [-O keyfile offset]] <SOURCE>"),
+			      N_("Open partition encrypted in plain mode."),
+			      options);
+}
+
+GRUB_MOD_FINI (plainmount)
+{
+  grub_unregister_extcmd (cmd);
+}
--
2.37.1

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Glenn Washburn]

I noticed that you've not been including Daniel Kiper on these patches.
I don't expect him to look at these until I'm satisfied with the patch,
but its always good practice to include him. So please do on the next
version of this patch.

On Sun, 31 Jul 2022 17:32:50 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> From 683357e227467c05272facc7da534a82becc9d8a Mon Sep 17 00:00:00 2001
> From: Maxim Fomin <maxim@fomin.one>
> Date: Sun, 31 Jul 2022 14:06:42 +0100
> Subject: [PATCH v6 1/1] plainmount: Support plain encryption mode
> 
> This patch adds support for plain encryption mode (plain dm-crypt) via
> new module/command named 'plainmount'.
> 
> Signed-off-by: Maxim Fomin <maxim@fomin.one>
> ---
>  docs/grub.texi              |  80 +++++++
>  grub-core/Makefile.core.def |   5 +
>  grub-core/disk/plainmount.c | 450 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 535 insertions(+)
>  create mode 100644 grub-core/disk/plainmount.c
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index af119dea3..22c73580c 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4265,6 +4265,7 @@ you forget a command, you can run the command @command{help}
>  * parttool::                    Modify partition table entries
>  * password::                    Set a clear-text password
>  * password_pbkdf2::             Set a hashed password
> +* plainmount::                  Open device encrypted in plain mode
>  * play::                        Play a tune
>  * probe::                       Retrieve device info
>  * rdmsr::                       Read values from model-specific registers
> @@ -4552,6 +4553,14 @@ function is supported, as Argon2 is not yet supported.
> 
>  Also, note that, unlike filesystem UUIDs, UUIDs for encrypted devices must be
>  specified without dash separators.
> +
> +Successfully decrypted disks are named as (cryptoX) and have increasing numeration
> +suffix for each new decrypted disk. If the encrypted disk hosts some higher level
> +of abstraction (like LVM2 or MDRAID) it will be created under a separate device
> +namespace in addition to the cryptodisk namespace.
> +
> +Support for plain encryption mode (plain dm-crypt) is provided via separate
> +@command{@pxref{plainmount}} command.
>  @end deffn
> 
>  @node cutmem
> @@ -5060,6 +5069,77 @@ to generate password hashes.  @xref{Security}.
>  @end deffn
> 
> 
> +@node plainmount
> +@subsection plainmount
> +
> +@deffn Command plainmount device @option{-c} cipher @option{-s} key size [@option{-h} hash]
> +[@option{-S} sector size] [@option{-p} password] [@option{-u} uuid]
> +[[@option{-d} keyfile] [@option{-O} keyfile offset]]
> +
> +
> +Setup access to the encrypted device in plain mode. Offset of the encrypted
> +data at the device is specified in terms of 512 byte sectors with the blocklist
> +syntax and loopback device. The following example shows how to specify 1MiB
> +offset:
> +
> +@example
> +loopback node (hd0,gpt1)2048+
> +plainmount node
> +@end example
> +
> +The @command{plainmount} command can be used to open LUKS encrypted volume
> +if its master key and parameters (key size, cipher, offset, etc) are known.
> +
> +There are two ways to specify password: a keyfile and a secret passphrase.

s/password/a password/

> +Keyfile path parameter has higher priority than secret passphrase and is

s/Keyfile/The keyfile/
s/secret passphrase/the secret passphrase parameter/

> +specified with the option @option{-d}. Password data obtained from keyfiles
> +is not hashed and is used directly as a cipher key. Optional offset of password

s/Optional/An optional/

> +data in the keyfile can be specified with the option @option{-O} or directly
> +with the option @option{-d} and GRUB blocklist syntax. The following example
> +shows both methods to specify password data in the keyfile at offset 1MiB:
> +
> +@example
> +plainmount -d (hd0,gpt1)2048+
> +plainmount -d (hd0,gpt1) -O 1048576

I don't think this will work. I think it has to be something like:

  plainmount -d (hd0,gpt1)+ -O 1048576

> +@end example
> +
> +If no keyfile is specified then the password is set to data specified by

s/data/the string/

> +option @option{-p} or is requested interactively from the console. In both
> +cases the provided password is hashed with the algorithm specified by the
> +option @option{-h}. This option is mandatory if no keyfile is specified, but
> +it can be set to @code{plain} which means that no hashing is done and such

I was mistaken in my last suggestion to use @code here, instead @samp
should be used because this is sample input.

> +password is used directly as a key.
> +
> +Cipher @option{-c} and keysize @option{-s} options specify the cipher
> +algorithm and the key size respectively and are mandatory options. Cipher
> +must be specified with the mode separated by a dash (for example,
> +'aes-xts-plain64'). Key size option @option{-s} is the key size of the cipher,

s/'aes-xts-plain64'/@samp{aes-xts-plain64}/
s/cipher/cipher in bits/

> +not to be confused with the offset of the key data in a keyfile specified with
> +the @option{-O} option. It must not exceed 128 bytes and must be specified in
> +bits (so a 32 byte key would be specified as 256 bits).

Also, I'm seeing now that we need not talk about the maximum number of
bytes here since the user is inputting in bits, so I think this
sentence is better:

  It must not exceed 1024 bits, so a 32 byte key would be specified as
  256 bits.

> +
> +The optional parameter @option{-S} specifies encrypted device sector size. It
> +must be at least 512 bytes long (default value) and a power of 2. @footnote{Current
> +implementation of cryptsetup supports only 512/1024/2048/4096 byte sectors}.
> +Disk sector size is configured when creating the encrypted volume. Attempting
> +to decrypt volumes with a different sector size than it was created with will
> +not result in an error, but will decrypt to random bytes and thus prevent
> +accessing the volume (in some cases filesystem driver can detect the filesystem

s/filesystem driver/the filesystem driver/

s/filesystem presense/presence of a filesystem/

> +presense, but nevertheless will refuse to mount it).
> +
> +By default new plainmount devices will be given a UUID starting with
> +'109fea84-a6b7-34a8-4bd1-1c506305a401' where the last digits are incremented
> +by one for each plainmounted device beyond the first up to 2^10 devices.
> +
> +All encryption arguments (cipher, hash, key size, disk offset and disk sector
> +size) must match the parameters used to create the volume. If any of them does
> +not match the actual arguments used during the initial encryption, plainmount
> +will create virtual device with the garbage data and GRUB will report unknown
> +filesystem for such device. Writing data to such virtual device will result in
> +the data loss if the underlying partition contained desired data.
> +@end deffn
> +
> +
>  @node play
>  @subsection play
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 715994872..3910b7670 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1174,6 +1174,11 @@ module = {
>    common = disk/cryptodisk.c;
>  };
> 
> +module = {
> +  name = plainmount;
> +  common = disk/plainmount.c;
> +};
> +
>  module = {
>    name = json;
>    common = lib/json/json.c;
> diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
> new file mode 100644
> index 000000000..8b735a425
> --- /dev/null
> +++ b/grub-core/disk/plainmount.c
> @@ -0,0 +1,450 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/* plaimount.c - Open device encrypted in plain mode. */
> +
> +#include <grub/cryptodisk.h>
> +#include <grub/dl.h>
> +#include <grub/err.h>
> +#include <grub/extcmd.h>
> +#include <grub/partition.h>
> +#include <grub/file.h>
> +
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
> +#define PLAINMOUNT_DEFAULT_UUID        "109fea84-a6b7-34a8-4bd1-1c506305a400"
> +
> +
> +enum PLAINMOUNT_OPTION
> +  {
> +    OPTION_HASH,
> +    OPTION_CIPHER,
> +    OPTION_KEY_SIZE,
> +    OPTION_SECTOR_SIZE,
> +    OPTION_PASSWORD,
> +    OPTION_KEYFILE,
> +    OPTION_KEYFILE_OFFSET,
> +    OPTION_UUID
> +  };
> +
> +
> +static const struct grub_arg_option options[] =
> +  {
> +    /* TRANSLATORS: It's still restricted to this module only.  */
> +    {"hash", 'h', 0, N_("Password hash"), 0, ARG_TYPE_STRING},
> +    {"cipher", 'c', 0, N_("Password cipher"), 0, ARG_TYPE_STRING},
> +    {"key-size", 's', 0, N_("Key size (in bits)"), 0, ARG_TYPE_INT},
> +    {"sector-size", 'S', 0, N_("Device sector size"), 0, ARG_TYPE_INT},
> +    {"password", 'p', 0, N_("Password (key)"), 0, ARG_TYPE_STRING},
> +    {"keyfile", 'd', 0, N_("Keyfile path"), 0, ARG_TYPE_STRING},
> +    {"keyfile-offset", 'O', 0, N_("Keyfile offset"), 0, ARG_TYPE_INT},
> +    {"uuid", 'u', 0, N_("Set device UUID"), 0, ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +
> +/* Cryptodisk setkey() function wrapper */
> +static grub_err_t
> +plainmount_setkey (grub_cryptodisk_t dev, grub_uint8_t *key,
> +		   grub_size_t size)
> +{
> +  gcry_err_code_t code = grub_cryptodisk_setkey (dev, key, size);
> +  if (code != GPG_ERR_NO_ERROR)
> +    {
> +      grub_dprintf ("plainmount", "failed to set cipher key with code: %d\n", code);
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot set specified key"));
> +    }
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Configure cryptodisk uuid */
> +static void plainmount_set_uuid (grub_cryptodisk_t dev, const char *user_uuid)
> +{
> +  grub_size_t pos = 0;
> +
> +  /* Size of user_uuid is checked in main func */
> +  if (user_uuid != NULL)
> +      grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
> +  else
> +    {
> +      /*
> +       * Set default UUID. Last digits start from 1 and are incremented for
> +       * each new plainmount device by snprintf().
> +       */
> +      grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%36lx", dev->id+1);
> +      while (dev->uuid[++pos] == ' ');
> +      grub_memcpy (dev->uuid, PLAINMOUNT_DEFAULT_UUID, pos);
> +    }
> +  COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (PLAINMOUNT_DEFAULT_UUID));
> +}
> +
> +
> +/* Configure cryptodevice sector size (-S option) */
> +static grub_err_t
> +plainmount_configure_sectors (grub_cryptodisk_t dev, grub_disk_t disk,
> +			      grub_size_t sector_size)
> +{
> +  dev->total_sectors = grub_disk_native_sectors (disk);
> +  if (dev->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> +    return grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot determine disk %s size"),
> +		       disk->name);
> +
> +  /* Convert size to sectors */
> +  dev->log_sector_size = grub_log2ull (sector_size);
> +  dev->total_sectors = grub_convert_sector (dev->total_sectors,
> +					    GRUB_DISK_SECTOR_BITS,
> +					    dev->log_sector_size);
> +  if (dev->total_sectors == 0)
> +    return grub_error (GRUB_ERR_BAD_DEVICE,
> +		       N_("cannot set specified sector size on disk %s"),
> +		       disk->name);
> +
> +  grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"
> +		PRIuGRUB_SIZE"\n", dev->log_sector_size, dev->total_sectors);
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Hashes a password into a key and stores it with cipher. */
> +static grub_uint8_t*
> +plainmount_configure_password (grub_cryptodisk_t dev, const char *hash,
> +			       grub_uint8_t *key_data, grub_size_t key_size)

Why does the return type changed from v5? I like it was better before,
and I'm thinking the signature should be more like hash() in
cryptsetup, that is have password and password_size arguments, to get
rid of the non-NULL byte assumption. Moving the password asking code
out of this function is fine though.

> +{
> +  grub_uint8_t *derived_hash, *dh;
> +  char *p;
> +  unsigned int round, i;
> +  unsigned int len, size;
> +
> +  /* Support none (plain) hash */
> +  if (grub_strcmp (hash, "plain") == 0)
> +    {
> +      dev->hash = NULL;
> +      return key_data;
> +    }
> +
> +  /* Hash argument was checked at main func */

It was? I'm not seeing the check below...

> +  dev->hash = grub_crypto_lookup_md_by_name (hash);
> +  len = dev->hash->mdlen;
> +  p = grub_malloc (key_size + 2 + (key_size / len));

This should password_size + (key_size / len). I forget what the 2 above
should be from (NULL byte and something else?), but I'm not sure its
needed. The hash() function in cryptsetup allows for NULL bytes in the
password string. I think we should also, so p doesn't need to be NULL
terminated. 

> +  derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> +  if (p == NULL || derived_hash == NULL)
> +    {
> +      grub_free (p);
> +      grub_free (derived_hash);
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      return NULL;
> +    }
> +  dh = derived_hash;
> +
> +  /*
> +   * Hash password. Adapted from cryptsetup.
> +   * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> +   */
> +  for (round = 0, size = key_size; size; round++, dh += len, size -= len)
> +    {
> +      for (i = 0; i < round; i++)
> +	p[i] = 'A';
> +
> +      grub_strcpy (p + i, (char*) key_data);

This assumes that there are no NULL bytes in key_data.

> +
> +      if (len > size)
> +	len = size;
> +
> +      grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));

This also has a non-NULL byte assumption.

> +    }
> +  grub_free (p);
> +  return derived_hash;
> +}
> +
> +
> +/* Read key material from keyfile */
> +static grub_err_t
> +plainmount_configure_keyfile (char *keyfile, grub_uint8_t *key_data,
> +			      grub_size_t key_size, grub_size_t keyfile_offset)
> +{
> +  grub_file_t g_keyfile = grub_file_open (keyfile, GRUB_FILE_TYPE_NONE);
> +  if (g_keyfile == NULL)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile %s"),
> +		       keyfile);
> +
> +  if (grub_file_seek (g_keyfile, keyfile_offset) == (grub_off_t)-1)
> +    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +		       N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
> +		       keyfile_offset);
> +
> +  if (key_size > (g_keyfile->size - keyfile_offset))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Specified key size (%"
> +		       PRIuGRUB_SIZE") is too small for keyfile size (%"
> +		       PRIuGRUB_SIZE") and offset (%"PRIuGRUB_SIZE")"),
> +		       key_size, g_keyfile->size, keyfile_offset);
> +
> +  if (grub_file_read (g_keyfile, key_data, key_size) != (grub_ssize_t) key_size)
> +    return grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading key file"));
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Plainmount command entry point */
> +static grub_err_t
> +grub_cmd_plainmount (grub_extcmd_context_t ctxt, int argc, char **args)
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  grub_cryptodisk_t dev = NULL;
> +  grub_disk_t disk = NULL;
> +  const gcry_md_spec_t *gcry_hash;
> +  char *diskname, *disklast = NULL, *cipher, *mode, *hash, *keyfile, *uuid;
> +  grub_size_t len, key_size, sector_size, keyfile_offset = 0;
> +  grub_err_t err;
> +  const char *p;
> +  grub_uint8_t *key_data;
> +  grub_uint8_t *hashed_key_data;
> +
> +  if (argc < 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
> +
> +  /* Check whether required arguments are specified */
> +  if (!state[OPTION_CIPHER].set || !state[OPTION_KEY_SIZE].set)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, "cipher and key size must be set");
> +  if (!state[OPTION_HASH].set && !state[OPTION_KEYFILE].set)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, "hash algorithm must be set");
> +
> +  /* Check hash */
> +  if (!state[OPTION_KEYFILE].set)
> +  {
> +    gcry_hash = grub_crypto_lookup_md_by_name (state[OPTION_HASH].arg);
> +    if (!gcry_hash)
> +      return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("couldn't load hash %s"),
> +			 state[OPTION_HASH].arg);
> +
> +    if (gcry_hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +			 N_("hash length %"PRIuGRUB_SIZE" exceeds maximum %d bits"),
> +			 gcry_hash->mdlen * GRUB_CHAR_BIT,
> +			 GRUB_CRYPTODISK_MAX_KEYLEN * GRUB_CHAR_BIT);
> +   }
> +
> +  /* Check cipher mode */
> +  if (!grub_strchr (state[OPTION_CIPHER].arg,'-'))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		       N_("invalid cipher mode, must be of format cipher-mode"));
> +
> +  /* Check password size */
> +  if (state[OPTION_PASSWORD].set && grub_strlen (state[OPTION_PASSWORD].arg) >
> +		                                 GRUB_CRYPTODISK_MAX_PASSPHRASE)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		       N_("password exceeds maximium size"));
> +
> +  /* Check uuid length */
> +  if (state[OPTION_UUID].set && grub_strlen (state[OPTION_UUID].arg) >
> +				sizeof (PLAINMOUNT_DEFAULT_UUID))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		       N_("specified UUID exceeds maximum size"));
> +
> +  /* Parse plainmount arguments */
> +  grub_errno = GRUB_ERR_NONE;
> +  keyfile_offset = state[OPTION_KEYFILE_OFFSET].set ?
> +		   grub_strtoull (state[OPTION_KEYFILE_OFFSET].arg, &p, 0) : 0;
> +  if (state[OPTION_KEYFILE_OFFSET].set &&
> +     (state[OPTION_KEYFILE_OFFSET].arg[0] == '\0' || *p != '\0' ||
> +      grub_errno != GRUB_ERR_NONE))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized keyfile offset"));
> +
> +  sector_size = state[OPTION_SECTOR_SIZE].set ?
> +		grub_strtoull (state[OPTION_SECTOR_SIZE].arg, &p, 0) :
> +		PLAINMOUNT_DEFAULT_SECTOR_SIZE;
> +  if (state[OPTION_SECTOR_SIZE].set && (state[OPTION_SECTOR_SIZE].arg[0] == '\0' ||
> +					*p != '\0' || grub_errno != GRUB_ERR_NONE))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector size"));
> +
> +  /* Check key size */
> +  key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0);
> +  if (state[OPTION_KEY_SIZE].arg[0] == '\0' || *p != '\0' ||
> +      grub_errno != GRUB_ERR_NONE)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
> +  if (key_size % GRUB_CHAR_BIT != 0)

I think parenthesis around "key_size % GRUB_CHAR_BIT" would be good.

> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		       N_("key size is not multiple of %d bits"), GRUB_CHAR_BIT);
> +  key_size = key_size / GRUB_CHAR_BIT;
> +  if (key_size > GRUB_CRYPTODISK_MAX_KEYLEN)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		       N_("key size %"PRIuGRUB_SIZE" exceeds maximum %d bits"),
> +		       key_size * GRUB_CHAR_BIT,
> +		       GRUB_CRYPTODISK_MAX_KEYLEN * GRUB_CHAR_BIT);
> +
> +  /* Check disk sector size */
> +  if (sector_size < GRUB_DISK_SECTOR_SIZE)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		       N_("sector size -S must be at least %d"),
> +		       GRUB_DISK_SECTOR_SIZE);
> +  if ((sector_size & (sector_size - 1)) != 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		       N_("sector size -S %"PRIuGRUB_SIZE" is not power of 2"),
> +		       sector_size);
> +
> +  /* Allocate all stuff here */
> +  hash =  state[OPTION_HASH].set ? grub_strdup (state[OPTION_HASH].arg) : NULL;
> +  cipher = grub_strdup (state[OPTION_CIPHER].arg);
> +  keyfile = state[OPTION_KEYFILE].set ?
> +	    grub_strdup (state[OPTION_KEYFILE].arg) : NULL;
> +  dev = grub_zalloc (sizeof *dev);
> +  key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> +  uuid = state[OPTION_UUID].set ? grub_strdup (state[OPTION_UUID].arg) : NULL;
> +  if ((hash == NULL && state[OPTION_HASH].set) || cipher == NULL || dev == NULL ||
> +      (keyfile == NULL && state[OPTION_KEYFILE].set) || key_data == NULL ||
> +      (uuid == NULL && state[OPTION_UUID].set))
> +    {
> +      err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      goto exit;
> +    }
> +
> +  /* Copy user password from -p option */
> +  if (state[OPTION_PASSWORD].set)
> +    grub_memcpy (key_data, state[OPTION_PASSWORD].arg,
> +		 grub_strlen (state[OPTION_PASSWORD].arg));
> +
> +  /* Copy user UUID from -u option */
> +  if (state[OPTION_UUID].set)
> +    grub_memcpy (uuid, state[OPTION_UUID].arg,
> +		 grub_strlen (state[OPTION_UUID].arg));
> +
> +  /* Set cipher mode (tested above) */
> +  mode = grub_strchr (cipher,'-');
> +  *mode++ = '\0';
> +
> +  /* Check cipher */
> +  if (grub_cryptodisk_setcipher (dev, cipher, mode) != GRUB_ERR_NONE)
> +    {
> +      err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher %s"), cipher);
> +      goto exit;
> +    }
> +
> +  /* Open SOURCE disk */
> +  diskname = args[0];
> +  len = grub_strlen (diskname);
> +  if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> +    {
> +      disklast = &diskname[len - 1];
> +      *disklast = '\0';
> +      diskname++;
> +    }
> +  disk = grub_disk_open (diskname);
> +  if (disk == NULL)
> +    {
> +      if (disklast)
> +        *disklast = ')';
> +      err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot open disk %s"), diskname);
> +      goto exit;
> +    }
> +
> +  /* Get password from console */
> +  if (keyfile == NULL && key_data[0] == '\0')
> +  {
> +    char *part = grub_partition_get_name (disk->partition);
> +    grub_printf_ (N_("Enter passphrase for %s%s%s: "), disk->name,
> +		  disk->partition != NULL ? "," : "",
> +		  part != NULL ? part : N_("UNKNOWN"));
> +    grub_free (part);
> +
> +    if (!grub_password_get ((char*)key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE-1))
> +        grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error reading password"));
> +  }
> +
> +  /* Warn if hash and keyfile are both provided */
> +  if (keyfile != NULL && state[OPTION_HASH].arg)

Seems inconsistent to use "keyfile != NULL" instead of
state[OPTION_KEYFILE].set, like the ifs below. Is there a reason for
this that I'm not thinking of?

> +    grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
> +
> +  /* Warn if -p option is specified with keyfile */
> +  if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
> +    grub_printf_ (N_("warning: password specified with -p option "
> +		     "is ignored if keyfile is provided\n"));
> +
> +  /* Warn of -O is provided without keyfile */
> +  if (state[OPTION_KEYFILE_OFFSET].set && !state[OPTION_KEYFILE].set)
> +    grub_printf_ (N_("warning: keyfile offset option -O "
> +		     "specified without keyfile option -d\n"));
> +
> +  grub_dprintf ("plainmount", "parameters: cipher=%s, hash=%s, key_size=%"
> +		PRIuGRUB_SIZE", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
> +		cipher, hash, key_size, keyfile, keyfile_offset);
> +
> +  err = plainmount_configure_sectors (dev, disk, sector_size);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  /* Configure keyfile or password */
> +  if (keyfile != NULL)
> +    {
> +      err = plainmount_configure_keyfile (keyfile, key_data, key_size, keyfile_offset);
> +      if (err != GRUB_ERR_NONE)
> +        goto exit;
> +      err = plainmount_setkey (dev, key_data, key_size);
> +      if (err != GRUB_ERR_NONE)
> +        goto exit;
> +    }
> +  else
> +    {
> +      hashed_key_data = plainmount_configure_password (dev, hash, key_data, key_size);

It looks like you're limiting key_data, which could be a string from
-p, to key_size. The cryptsetup code does not appear to do this, so I
think this does not work for passwords longer than the hash length.

> +      if (hashed_key_data == NULL)
> +        goto exit;
> +      err = plainmount_setkey (dev, hashed_key_data, key_size);
> +      if (err != GRUB_ERR_NONE)
> +      {
> +        grub_free (hashed_key_data);
> +        goto exit;
> +      }
> +    }

I was hoping that when moving plainmount_setkey() out of the
plainmount_configure_*() functions that it could be only called once in
the code, instead of twice as done here. Why can't we refactor and have
this code here:

  err = plainmount_setkey (dev, key_data, key_size);
  if (err != GRUB_ERR_NONE)
    goto exit;

Glenn

> +
> +  err = grub_cryptodisk_insert (dev, diskname, disk);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  dev->modname = "plainmount";
> +  dev->source_disk = disk;
> +  plainmount_set_uuid (dev, uuid);
> +
> +exit:
> +  grub_free (hash);
> +  grub_free (cipher);
> +  grub_free (keyfile);
> +  grub_free (key_data);
> +  grub_free (uuid);
> +  if (err != GRUB_ERR_NONE && disk)
> +    grub_disk_close (disk);
> +  if (err != GRUB_ERR_NONE && dev)
> +    grub_free (dev);
> +  return err;
> +}
> +
> +static grub_extcmd_t cmd;
> +GRUB_MOD_INIT (plainmount)
> +{
> +  cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
> +			      N_("-c cipher -s key-size [-h hash] [-S sector-size]"
> +			      " [-o offset] [-p password] [-u uuid] "
> +			      " [[-d keyfile] [-O keyfile offset]] <SOURCE>"),
> +			      N_("Open partition encrypted in plain mode."),
> +			      options);
> +}
> +
> +GRUB_MOD_FINI (plainmount)
> +{
> +  grub_unregister_extcmd (cmd);
> +}
> --
> 2.37.1
> 
> 

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Maxim Fomin]

------- Original Message -------
On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn <development@efficientek.com> wrote:
> > +/ Hashes a password into a key and stores it with cipher. /
> > +static grub_uint8_t
> > +plainmount_configure_password (grub_cryptodisk_t dev, const char *hash,
> > + grub_uint8_t *key_data, grub_size_t key_size)
>
>
> Why does the return type changed from v5? I like it was better before,
> and I'm thinking the signature should be more like hash() in
> cryptsetup, that is have password and password_size arguments, to get
> rid of the non-NULL byte assumption. Moving the password asking code
> out of this function is fine though.

I changed signature because configure_password() modifies password data sent from the caller.
It does so in a such way, that new pointer must be returned (that's what I was thinking when
changing function signature). This does not happen with the configure_keyfile() because it
does not modify the buffer. So, moving the call to setkey() into the main func (out from
configure_password() and configure_keyfile()) required changing one of the function signatures.
I will reconsider this issue to make signatures look like hash() in cryptsetup and also will
check the issue with NULL byte.

>
> > + grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
> > +
> > + /* Warn if -p option is specified with keyfile /
> > + if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
> > + grub_printf_ (N_("warning: password specified with -p option "
> > + "is ignored if keyfile is provided\n"));
> > +
> > + / Warn of -O is provided without keyfile /
> > + if (state[OPTION_KEYFILE_OFFSET].set && !state[OPTION_KEYFILE].set)
> > + grub_printf_ (N_("warning: keyfile offset option -O "
> > + "specified without keyfile option -d\n"));
> > +
> > + grub_dprintf ("plainmount", "parameters: cipher=%s, hash=%s, key_size=%"
> > + PRIuGRUB_SIZE", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
> > + cipher, hash, key_size, keyfile, keyfile_offset);
> > +
> > + err = plainmount_configure_sectors (dev, disk, sector_size);
> > + if (err != GRUB_ERR_NONE)
> > + goto exit;
> > +
> > + / Configure keyfile or password */
> > + if (keyfile != NULL)
> > + {
> > + err = plainmount_configure_keyfile (keyfile, key_data, key_size, keyfile_offset);
> > + if (err != GRUB_ERR_NONE)
> > + goto exit;
> > + err = plainmount_setkey (dev, key_data, key_size);
> > + if (err != GRUB_ERR_NONE)
> > + goto exit;
> > + }
> > + else
> > + {
> > + hashed_key_data = plainmount_configure_password (dev, hash, key_data, key_size);
>
>
> It looks like you're limiting key_data, which could be a string from
> -p, to key_size. The cryptsetup code does not appear to do this, so I
> think this does not work for passwords longer than the hash length.

In one of the old versions of the patch I removed the option which allowed to set key size
from password or keyfile. I considered it is strange to specify key size option (-s 512,
for example) and then implicitly specify different key size from password, hash or keyfile
argument. I think it was that version of the patch where you pointed on possible buffer
overflow attack because of this feature.

Also, I am confused about maximum key data and password size allowed by cryptomount.h. It
limits GRUB_CRYPTODISK_MAX_KEYLEN to 128, GRUB_CRYPTODISK_MAX_PASSPHRASE to 256 and
GRUB_CRYPTODISK_MAX_KEYFILE_SIZE to 8192. So, it looks like the passphrase (before hashing)
is limited to 256 bytes, when it is hashed - it is limited to hash size, which cannot be
larger than 128 bytes. Why then GRUB_CRYPTODISK_MAX_KEYFILE_SIZE is limited to 8192 bytes
(or bits?)? Also, if password is not hashed (-h plain) then 129 byte password becomes illegal,
because it is used directly as a key, which is limited to 128 bytes. Am I correct?

> > + if (hashed_key_data == NULL)
> > + goto exit;
> > + err = plainmount_setkey (dev, hashed_key_data, key_size);
> > + if (err != GRUB_ERR_NONE)
> > + {
> > + grub_free (hashed_key_data);
> > + goto exit;
> > + }
> > + }
>
>
> I was hoping that when moving plainmount_setkey() out of the
> plainmount_configure_*() functions that it could be only called once in
> the code, instead of twice as done here. Why can't we refactor and have
> this code here:
>
> err = plainmount_setkey (dev, key_data, key_size);
> if (err != GRUB_ERR_NONE)
> goto exit;
>
> Glenn

I will check this in the next version (see my comment above about changing data buffer
in one of configure_*() function explaining why I changed the function signature).

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Glenn Washburn]

On Sat, 27 Aug 2022 12:06:30 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn <development@efficientek.com> wrote:
> > > +/ Hashes a password into a key and stores it with cipher. /
> > > +static grub_uint8_t
> > > +plainmount_configure_password (grub_cryptodisk_t dev, const char *hash,
> > > + grub_uint8_t *key_data, grub_size_t key_size)
> >
> >
> > Why does the return type changed from v5? I like it was better before,
> > and I'm thinking the signature should be more like hash() in
> > cryptsetup, that is have password and password_size arguments, to get
> > rid of the non-NULL byte assumption. Moving the password asking code
> > out of this function is fine though.
> 
> I changed signature because configure_password() modifies password data sent from the caller.
> It does so in a such way, that new pointer must be returned (that's what I was thinking when
> changing function signature). This does not happen with the configure_keyfile() because it
> does not modify the buffer. So, moving the call to setkey() into the main func (out from
> configure_password() and configure_keyfile()) required changing one of the function signatures.
> I will reconsider this issue to make signatures look like hash() in cryptsetup and also will
> check the issue with NULL byte.
> 
> >
> > > + grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
> > > +
> > > + /* Warn if -p option is specified with keyfile /
> > > + if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
> > > + grub_printf_ (N_("warning: password specified with -p option "
> > > + "is ignored if keyfile is provided\n"));
> > > +
> > > + / Warn of -O is provided without keyfile /
> > > + if (state[OPTION_KEYFILE_OFFSET].set && !state[OPTION_KEYFILE].set)
> > > + grub_printf_ (N_("warning: keyfile offset option -O "
> > > + "specified without keyfile option -d\n"));
> > > +
> > > + grub_dprintf ("plainmount", "parameters: cipher=%s, hash=%s, key_size=%"
> > > + PRIuGRUB_SIZE", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
> > > + cipher, hash, key_size, keyfile, keyfile_offset);
> > > +
> > > + err = plainmount_configure_sectors (dev, disk, sector_size);
> > > + if (err != GRUB_ERR_NONE)
> > > + goto exit;
> > > +
> > > + / Configure keyfile or password */
> > > + if (keyfile != NULL)
> > > + {
> > > + err = plainmount_configure_keyfile (keyfile, key_data, key_size, keyfile_offset);
> > > + if (err != GRUB_ERR_NONE)
> > > + goto exit;
> > > + err = plainmount_setkey (dev, key_data, key_size);
> > > + if (err != GRUB_ERR_NONE)
> > > + goto exit;
> > > + }
> > > + else
> > > + {
> > > + hashed_key_data = plainmount_configure_password (dev, hash, key_data, key_size);
> >
> >
> > It looks like you're limiting key_data, which could be a string from
> > -p, to key_size. The cryptsetup code does not appear to do this, so I
> > think this does not work for passwords longer than the hash length.
> 
> In one of the old versions of the patch I removed the option which allowed to set key size
> from password or keyfile. I considered it is strange to specify key size option (-s 512,
> for example) and then implicitly specify different key size from password, hash or keyfile
> argument. I think it was that version of the patch where you pointed on possible buffer
> overflow attack because of this feature.

The password nor should keyfile does not set the keysize, implicitly or
otherwise.

> 
> Also, I am confused about maximum key data and password size allowed by cryptomount.h. It
> limits GRUB_CRYPTODISK_MAX_KEYLEN to 128, GRUB_CRYPTODISK_MAX_PASSPHRASE to 256 and
> GRUB_CRYPTODISK_MAX_KEYFILE_SIZE to 8192. So, it looks like the passphrase (before hashing)
> is limited to 256 bytes, when it is hashed - it is limited to hash size, which cannot be
> larger than 128 bytes. Why then GRUB_CRYPTODISK_MAX_KEYFILE_SIZE is limited to 8192 bytes
> (or bits?)? Also, if password is not hashed (-h plain) then 129 byte password becomes illegal,
> because it is used directly as a key, which is limited to 128 bytes. Am I correct?

The max keyfile size 4096 bytes, and I believe that is the cryptsetup
default compiled in maximum. Yes, an unhashed 129 byte password will be
longer than the max key length, this is okay because it should just be
truncated to keysize. Cryptsetup looks like it fails when the password
using the plain hash is of length _less_ than key size. If it is _more_
than keysize, then its fine, it just copies up to keysize bytes.

As far as keyfiles, plainmount is different than cryptomount, because
keyfile data in plainmount is used as the encryption key, but in
cryptomount its used as the password (ie the keyfile is hashed). I'm
actually not sure if this conforms with cryptsetup, can you verify
this? The documentation doesn't seem to specify.

> 
> > > + if (hashed_key_data == NULL)
> > > + goto exit;
> > > + err = plainmount_setkey (dev, hashed_key_data, key_size);
> > > + if (err != GRUB_ERR_NONE)
> > > + {
> > > + grub_free (hashed_key_data);
> > > + goto exit;
> > > + }
> > > + }
> >
> >
> > I was hoping that when moving plainmount_setkey() out of the
> > plainmount_configure_*() functions that it could be only called once in
> > the code, instead of twice as done here. Why can't we refactor and have
> > this code here:
> >
> > err = plainmount_setkey (dev, key_data, key_size);
> > if (err != GRUB_ERR_NONE)
> > goto exit;
> >
> > Glenn
> 
> I will check this in the next version (see my comment above about changing data buffer
> in one of configure_*() function explaining why I changed the function signature).

Sure that makes sense, but I think having the signature more like hash()
in cryptsetup doesn't have this issue.

Glenn

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Maxim Fomin]

------- Original Message -------
On Saturday, August 27th, 2022 at 5:05 PM, Glenn Washburn <development@efficientek.com> wrote:
> 
> 
> On Sat, 27 Aug 2022 12:06:30 +0000
> Maxim Fomin maxim@fomin.one wrote:
> 
> > Also, I am confused about maximum key data and password size allowed by cryptomount.h. It
> > limits GRUB_CRYPTODISK_MAX_KEYLEN to 128, GRUB_CRYPTODISK_MAX_PASSPHRASE to 256 and
> > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE to 8192. So, it looks like the passphrase (before hashing)
> > is limited to 256 bytes, when it is hashed - it is limited to hash size, which cannot be
> > larger than 128 bytes. Why then GRUB_CRYPTODISK_MAX_KEYFILE_SIZE is limited to 8192 bytes
> > (or bits?)? Also, if password is not hashed (-h plain) then 129 byte password becomes illegal,
> > because it is used directly as a key, which is limited to 128 bytes. Am I correct?
> 
> 
> The max keyfile size 4096 bytes, and I believe that is the cryptsetup
> default compiled in maximum. Yes, an unhashed 129 byte password will be
> longer than the max key length, this is okay because it should just be
> truncated to keysize. Cryptsetup looks like it fails when the password
> using the plain hash is of length less than key size. If it is more
> than keysize, then its fine, it just copies up to keysize bytes.
> 
> As far as keyfiles, plainmount is different than cryptomount, because
> keyfile data in plainmount is used as the encryption key, but in
> cryptomount its used as the password (ie the keyfile is hashed). I'm
> actually not sure if this conforms with cryptsetup, can you verify
> this? The documentation doesn't seem to specify.
> 

Yes, it specified in man cryptsetup in this section:

NOTES
   Passphrase processing for PLAIN mode
       Note that no iterated hashing or salting is done in plain mode. If hashing is done, it is a single direct hash. This means
       that low-entropy passphrases are easy to attack in plain mode.

       From a terminal: The passphrase is read until the first newline, i.e. '\n'. The input without the newline character is
       processed with the default hash or the hash specified with --hash. The hash result will be truncated to the key size of the
       used cipher, or the size specified with -s.

       From stdin: Reading will continue until a newline (or until the maximum input size is reached), with the trailing newline
       stripped. The maximum input size is defined by the same compiled-in default as for the maximum key file size and can be
       overwritten using --keyfile-size option.

       The data read will be hashed with the default hash or the hash specified with --hash. The hash result will be truncated to
       the key size of the used cipher, or the size specified with -s.

       Note that if --key-file=- is used for reading the key from stdin, trailing newlines are not stripped from the input.

       If "plain" is used as argument to --hash, the input data will not be hashed. Instead, it will be zero padded (if shorter than
       the key size) or truncated (if longer than the key size) and used directly as the binary key. This is useful for directly
       specifying a binary key. No warning will be given if the amount of data read from stdin is less than the key size.

       From a key file: It will be truncated to the key size of the used cipher or the size given by -s and directly used as a
       binary key.

       WARNING: The --hash argument is being ignored. The --hash option is usable only for stdin input in plain mode.

       If the key file is shorter than the key, cryptsetup will quit with an error. The maximum input size is defined by the same
       compiled-in default as for the maximum key file size and can be overwritten using --keyfile-size option.

Btw, it says also about keydata being truncated.

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Maxim Fomin]

------- Original Message -------
On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn <development@efficientek.com> wrote:

> Why does the return type changed from v5? I like it was better before,
> and I'm thinking the signature should be more like hash() in
> cryptsetup, that is have password and password_size arguments, to get
> rid of the non-NULL byte assumption. Moving the password asking code
> out of this function is fine though.
>
> > +{
> > + grub_uint8_t *derived_hash, *dh;
> > + char p;
> > + unsigned int round, i;
> > + unsigned int len, size;
> > +
> > + / Support none (plain) hash /
> > + if (grub_strcmp (hash, "plain") == 0)
> > + {
> > + dev->hash = NULL;
> > + return key_data;
> > + }
> > +
> > + / Hash argument was checked at main func */
>
> This should password_size + (key_size / len). I forget what the 2 above
> should be from (NULL byte and something else?), but I'm not sure its
> needed. The hash() function in cryptsetup allows for NULL bytes in the
> password string. I think we should also, so p doesn't need to be NULL
> terminated.
>
> > + derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> > + if (p == NULL || derived_hash == NULL)
> > + {
> > + grub_free (p);
> > + grub_free (derived_hash);
> > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > + return NULL;
> > + }
> > + dh = derived_hash;
> > +
> > + /*
> > + * Hash password. Adapted from cryptsetup.
> > + * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> > + /
> > + for (round = 0, size = key_size; size; round++, dh += len, size -= len)
> > + {
> > + for (i = 0; i < round; i++)
> > + p[i] = 'A';
> > +
> > + grub_strcpy (p + i, (char) key_data);
>
>
> This assumes that there are no NULL bytes in key_data.
>
> > +
> > + if (len > size)
> > + len = size;
> > +
> > + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
>
>
> This also has a non-NULL byte assumption.
>

Regarding NULL byte assumption. You mention it in the context of supplying password
from grub terminal via interactive console or via '-p' option. How user is supposed
to input NULL character? I couldn't find how to do it. If supplying NULL byte is not
possible, then supporting this feature is useless. In cryptsetup 'password size' is
not used to support NULL byte in password, it is used for different purpose: the key
size (-s in terms of plainmount syntax) parameter is optional, so 'password size'
parameter keeps password size. In plainmount key size is mandatory and should match
the actual password size, so 'password size' parameter is not needed.

Note that keyfile method of providing key material supports any character, including
NULL byte, and its current implementation does not depend on strcpy() function.

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Glenn Washburn]

On Wed, 14 Sep 2022 16:15:56 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn <development@efficientek.com> wrote:
> 
> > Why does the return type changed from v5? I like it was better before,
> > and I'm thinking the signature should be more like hash() in
> > cryptsetup, that is have password and password_size arguments, to get
> > rid of the non-NULL byte assumption. Moving the password asking code
> > out of this function is fine though.
> >
> > > +{
> > > + grub_uint8_t *derived_hash, *dh;
> > > + char p;
> > > + unsigned int round, i;
> > > + unsigned int len, size;
> > > +
> > > + / Support none (plain) hash /
> > > + if (grub_strcmp (hash, "plain") == 0)
> > > + {
> > > + dev->hash = NULL;
> > > + return key_data;
> > > + }
> > > +
> > > + / Hash argument was checked at main func */
> >
> > This should password_size + (key_size / len). I forget what the 2 above
> > should be from (NULL byte and something else?), but I'm not sure its
> > needed. The hash() function in cryptsetup allows for NULL bytes in the
> > password string. I think we should also, so p doesn't need to be NULL
> > terminated.
> >
> > > + derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> > > + if (p == NULL || derived_hash == NULL)
> > > + {
> > > + grub_free (p);
> > > + grub_free (derived_hash);
> > > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > > + return NULL;
> > > + }
> > > + dh = derived_hash;
> > > +
> > > + /*
> > > + * Hash password. Adapted from cryptsetup.
> > > + * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> > > + /
> > > + for (round = 0, size = key_size; size; round++, dh += len, size -= len)
> > > + {
> > > + for (i = 0; i < round; i++)
> > > + p[i] = 'A';
> > > +
> > > + grub_strcpy (p + i, (char) key_data);
> >
> >
> > This assumes that there are no NULL bytes in key_data.
> >
> > > +
> > > + if (len > size)
> > > + len = size;
> > > +
> > > + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> >
> >
> > This also has a non-NULL byte assumption.
> >
> 
> Regarding NULL byte assumption. You mention it in the context of supplying password
> from grub terminal via interactive console or via '-p' option. How user is supposed
> to input NULL character? I couldn't find how to do it. If supplying NULL byte is not
> possible, then supporting this feature is useless. In cryptsetup 'password size' is

Yes, I don't think there is currently a way to add NULL bytes into the
password string. This could change in the future (eg. patch to allow
\xXX or \oOOO character escapes). Making this NULL byte agnostic will
future proof this code. Do you think it will take much effort to make
the change? If you are really against this, I can accept that, but in
that case there should be at a minimum a comment above the function
stating that it does not handle passwords containing NULL bytes.

> not used to support NULL byte in password, it is used for different purpose: the key
> size (-s in terms of plainmount syntax) parameter is optional, so 'password size'
> parameter keeps password size. In plainmount key size is mandatory and should match
> the actual password size, so 'password size' parameter is not needed.

No, the 'password size' is not used to determine the 'key size' when
key size is not given, nor is it to support NULL bytes in the password
string. It is to support passwords that are a different size from the
key size. What I was saying is that this allows the use of NULL bytes
in the password hashing code (I've not tried sending a password with
NULL bytes from the command line to see if there is a way for a user
to put NULL bytes in the password string).

Why should key size match password size? Why shouldn't I be able to
have a password not equal to key size? The hash function should hash
the password of any size to a string of bytes that is key size long.
And that is what the cryptsetup code does.
 
> Note that keyfile method of providing key material supports any character, including
> NULL byte, and its current implementation does not depend on strcpy() function.

Yep, this is good.

Glenn

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Maxim Fomin]

------- Original Message -------
On Thursday, September 15th, 2022 at 12:54 AM, Glenn Washburn <development@efficientek.com> wrote:
> 
> On Wed, 14 Sep 2022 16:15:56 +0000
> Maxim Fomin maxim@fomin.one wrote:
> 
> > ------- Original Message -------
> > On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn development@efficientek.com wrote:
> > 
> > > Why does the return type changed from v5? I like it was better before,
> > > and I'm thinking the signature should be more like hash() in
> > > cryptsetup, that is have password and password_size arguments, to get
> > > rid of the non-NULL byte assumption. Moving the password asking code
> > > out of this function is fine though.
> > > 
> > > > +{
> > > > + grub_uint8_t *derived_hash, *dh;
> > > > + char p;
> > > > + unsigned int round, i;
> > > > + unsigned int len, size;
> > > > +
> > > > + / Support none (plain) hash /
> > > > + if (grub_strcmp (hash, "plain") == 0)
> > > > + {
> > > > + dev->hash = NULL;
> > > > + return key_data;
> > > > + }
> > > > +
> > > > + / Hash argument was checked at main func */
> > > 
> > > This should password_size + (key_size / len). I forget what the 2 above
> > > should be from (NULL byte and something else?), but I'm not sure its
> > > needed. The hash() function in cryptsetup allows for NULL bytes in the
> > > password string. I think we should also, so p doesn't need to be NULL
> > > terminated.
> > > 
> > > > + derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> > > > + if (p == NULL || derived_hash == NULL)
> > > > + {
> > > > + grub_free (p);
> > > > + grub_free (derived_hash);
> > > > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > > > + return NULL;
> > > > + }
> > > > + dh = derived_hash;
> > > > +
> > > > + /*
> > > > + * Hash password. Adapted from cryptsetup.
> > > > + * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> > > > + /
> > > > + for (round = 0, size = key_size; size; round++, dh += len, size -= len)
> > > > + {
> > > > + for (i = 0; i < round; i++)
> > > > + p[i] = 'A';
> > > > +
> > > > + grub_strcpy (p + i, (char) key_data);
> > > 
> > > This assumes that there are no NULL bytes in key_data.
> > > 
> > > > +
> > > > + if (len > size)
> > > > + len = size;
> > > > +
> > > > + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> > > 
> > > This also has a non-NULL byte assumption.
> > 
> > Regarding NULL byte assumption. You mention it in the context of supplying password
> > from grub terminal via interactive console or via '-p' option. How user is supposed
> > to input NULL character? I couldn't find how to do it. If supplying NULL byte is not
> > possible, then supporting this feature is useless. In cryptsetup 'password size' is
> 
> 
> Yes, I don't think there is currently a way to add NULL bytes into the
> password string. This could change in the future (eg. patch to allow
> \xXX or \oOOO character escapes). Making this NULL byte agnostic will
> future proof this code. Do you think it will take much effort to make
> the change? If you are really against this, I can accept that, but in
> that case there should be at a minimum a comment above the function
> stating that it does not handle passwords containing NULL bytes.

I am not against this change, actually I already implemented it. I just wanted to clatify
this issue - I was not sure that currently there is no way to type NULL byte. I will make
the code NULL agnostic and write a big comment explaining current situation.

> 
> > not used to support NULL byte in password, it is used for different purpose: the key
> > size (-s in terms of plainmount syntax) parameter is optional, so 'password size'
> > parameter keeps password size. In plainmount key size is mandatory and should match
> > the actual password size, so 'password size' parameter is not needed.
> 
> 
> No, the 'password size' is not used to determine the 'key size' when
> key size is not given, nor is it to support NULL bytes in the password
> string. It is to support passwords that are a different size from the
> key size. What I was saying is that this allows the use of NULL bytes
> in the password hashing code (I've not tried sending a password with
> NULL bytes from the command line to see if there is a way for a user
> to put NULL bytes in the password string).
> 
> Why should key size match password size? Why shouldn't I be able to
> have a password not equal to key size? The hash function should hash
> the password of any size to a string of bytes that is key size long.
> And that is what the cryptsetup code does.
> 
> Glenn

I was incorrect when saying that key size must match password size. Current plainmount
implementaion truncates hashed password if key size < len(hash(password)) and zero-padds
hash if key size > len(hash(password)). This happens if key size != len(hash()). This
behavior matches cryptsetup. However, I believe most users choose key size length that
matches len(hash_algorithm), so those 'corner cases' do not arise in practice.

Best regards,
Maxim

Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Glenn Washburn]

On Thu, 15 Sep 2022 05:28:40 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> On Thursday, September 15th, 2022 at 12:54 AM, Glenn Washburn <development@efficientek.com> wrote:
> > 
> > On Wed, 14 Sep 2022 16:15:56 +0000
> > Maxim Fomin maxim@fomin.one wrote:
> > 
> > > ------- Original Message -------
> > > On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn development@efficientek.com wrote:
> > > 
> > > > Why does the return type changed from v5? I like it was better before,
> > > > and I'm thinking the signature should be more like hash() in
> > > > cryptsetup, that is have password and password_size arguments, to get
> > > > rid of the non-NULL byte assumption. Moving the password asking code
> > > > out of this function is fine though.
> > > > 
> > > > > +{
> > > > > + grub_uint8_t *derived_hash, *dh;
> > > > > + char p;
> > > > > + unsigned int round, i;
> > > > > + unsigned int len, size;
> > > > > +
> > > > > + / Support none (plain) hash /
> > > > > + if (grub_strcmp (hash, "plain") == 0)
> > > > > + {
> > > > > + dev->hash = NULL;
> > > > > + return key_data;
> > > > > + }
> > > > > +
> > > > > + / Hash argument was checked at main func */
> > > > 
> > > > This should password_size + (key_size / len). I forget what the 2 above
> > > > should be from (NULL byte and something else?), but I'm not sure its
> > > > needed. The hash() function in cryptsetup allows for NULL bytes in the
> > > > password string. I think we should also, so p doesn't need to be NULL
> > > > terminated.
> > > > 
> > > > > + derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> > > > > + if (p == NULL || derived_hash == NULL)
> > > > > + {
> > > > > + grub_free (p);
> > > > > + grub_free (derived_hash);
> > > > > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > > > > + return NULL;
> > > > > + }
> > > > > + dh = derived_hash;
> > > > > +
> > > > > + /*
> > > > > + * Hash password. Adapted from cryptsetup.
> > > > > + * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> > > > > + /
> > > > > + for (round = 0, size = key_size; size; round++, dh += len, size -= len)
> > > > > + {
> > > > > + for (i = 0; i < round; i++)
> > > > > + p[i] = 'A';
> > > > > +
> > > > > + grub_strcpy (p + i, (char) key_data);
> > > > 
> > > > This assumes that there are no NULL bytes in key_data.
> > > > 
> > > > > +
> > > > > + if (len > size)
> > > > > + len = size;
> > > > > +
> > > > > + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> > > > 
> > > > This also has a non-NULL byte assumption.
> > > 
> > > Regarding NULL byte assumption. You mention it in the context of supplying password
> > > from grub terminal via interactive console or via '-p' option. How user is supposed
> > > to input NULL character? I couldn't find how to do it. If supplying NULL byte is not
> > > possible, then supporting this feature is useless. In cryptsetup 'password size' is
> > 
> > 
> > Yes, I don't think there is currently a way to add NULL bytes into the
> > password string. This could change in the future (eg. patch to allow
> > \xXX or \oOOO character escapes). Making this NULL byte agnostic will
> > future proof this code. Do you think it will take much effort to make
> > the change? If you are really against this, I can accept that, but in
> > that case there should be at a minimum a comment above the function
> > stating that it does not handle passwords containing NULL bytes.
> 
> I am not against this change, actually I already implemented it. I just wanted to clatify
> this issue - I was not sure that currently there is no way to type NULL byte. I will make
> the code NULL agnostic and write a big comment explaining current situation.

Thanks!

> > 
> > > not used to support NULL byte in password, it is used for different purpose: the key
> > > size (-s in terms of plainmount syntax) parameter is optional, so 'password size'
> > > parameter keeps password size. In plainmount key size is mandatory and should match
> > > the actual password size, so 'password size' parameter is not needed.
> > 
> > 
> > No, the 'password size' is not used to determine the 'key size' when
> > key size is not given, nor is it to support NULL bytes in the password
> > string. It is to support passwords that are a different size from the
> > key size. What I was saying is that this allows the use of NULL bytes
> > in the password hashing code (I've not tried sending a password with
> > NULL bytes from the command line to see if there is a way for a user
> > to put NULL bytes in the password string).
> > 
> > Why should key size match password size? Why shouldn't I be able to
> > have a password not equal to key size? The hash function should hash
> > the password of any size to a string of bytes that is key size long.
> > And that is what the cryptsetup code does.
> > 
> > Glenn
> 
> I was incorrect when saying that key size must match password size. Current plainmount
> implementaion truncates hashed password if key size < len(hash(password)) and zero-padds
> hash if key size > len(hash(password)). This happens if key size != len(hash()). This
> behavior matches cryptsetup. However, I believe most users choose key size length that
> matches len(hash_algorithm), so those 'corner cases' do not arise in practice.

Yes, but this isn't really what I was speaking to. Even when the hash
size is equal to the key size, the password can be longer or shorter
than the key size. I think its probably pretty common that the user
chooses a password that does not equal key size. And in this case all
bits of the password should be fed to the hash to create the key (which
is just a hash of the password bits).

Glenn