* [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
@ 2022-01-30 19:40 Maxim Fomin
2022-01-31 11:15 ` Milan Broz
2022-02-01 2:30 ` Glenn Washburn
0 siblings, 2 replies; 8+ messages in thread
From: Maxim Fomin @ 2022-01-30 19:40 UTC (permalink / raw)
To: grub-devel
This patch introduces support for plain encryption mode (plain dm-crypt) via
new module and command named 'plainmount'. The command allows to open devices
encrypted in plain mode (without LUKS) with following syntax:
plainmount <SOURCE> -h hash -c cipher -o offset -s size -k key-size
-z sector-size -d keyfile -O keyfile offset -l keyfile-size
<SOURCE> can be some partition or GPT UUID, keyfile can be <UUID>/some/path.
Despite moving plain mode code into separate module, it still depends on
cryptodisk.c because cryptodisk module defines whole grub infrastructure
of encrypted devices.
Principal feature of this patch is supporting GPT partition UUID to point to
specific partition. The patch also includes two other minor features: using
disk block as a key (instead of keyfile) and support for 1024/2048/4096 byte
blocks in plain mode (cryptsetup added large block support in plain mode after
introducing them in LUKS2 mode - since circa version 2.4).
This is fully reworked version of '0004-Cryptomount-support-plain-dm-crypt'
patch which is member of John Lane crypto enhancement patch series. It also
includes '0007-Add-support-for-using-a-whole-device-as-a-keyfile' patch. They
were sent in grub-devel mailing list as:
From a8f9e3dcece89c179e89414abe89985c7ab1e03f Mon Sep 17 00:00:00 2001
From: John Lane <john@lane.uk.net>
Date: Fri, 26 Jun 2015 22:09:52 +0100
Subject: [PATCH 4/7] Cryptomount support plain dm-crypt
From ef720d0d44b8d97a83950ced0df1ce1bcf8cd988 Mon Sep 17 00:00:00 2001
From: Paul Gideon Dann <pdgiddie@gmail.com>
Date: Tue, 19 Jul 2016 12:36:37 +0100
Subject: [PATCH 7/7] Add support for using a whole device as a keyfile
---
grub-core/Makefile.core.def | 5 +
grub-core/disk/plainmount.c | 678 ++++++++++++++++++++++++++++++++++++
2 files changed, 683 insertions(+)
create mode 100644 grub-core/disk/plainmount.c
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 8022e1c0a..ce8478055 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1184,6 +1184,11 @@ module = {
common = disk/luks.c;
};
+module = {
+ name = plainmount;
+ common = disk/plainmount.c;
+};
+
module = {
name = luks2;
common = disk/luks2.c;
diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
new file mode 100644
index 000000000..4654cec96
--- /dev/null
+++ b/grub-core/disk/plainmount.c
@@ -0,0 +1,678 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2003,2007,2010,2011,2019 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/>.
+ */
+
+#include <grub/cryptodisk.h>
+#include <grub/dl.h>
+#include <grub/err.h>
+#include <grub/extcmd.h>
+#include <grub/partition.h>
+#include <grub/file.h>
+#include <grub/gpt_partition.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+#define GRUB_PLAINMOUNT_UUID "00000000000000000000000000000000"
+#define GRUB_PLAINMOUNT_CIPHER "aes-cbc-essiv:sha256"
+#define GRUB_PLAINMOUNT_DIGEST "ripemd160"
+#define GRUB_PLAINMOUNT_KEY_SIZE 256
+#define GRUB_PLAINMOUNT_SECTOR_SIZE 512
+
+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},
+ {"offset", 'o', 0, N_("Device offset (512 bit blocks)."), 0, ARG_TYPE_INT},
+ {"size", 'b', 0, N_("Size of device (512 byte blocks)."), 0, ARG_TYPE_INT},
+ {"key-size", 's', 0, N_("Key size (in bits)."), 0, ARG_TYPE_INT},
+ {"sector-size", 'z', 0, N_("Device sector size."), 0, ARG_TYPE_INT},
+ {"keyfile", 'd', 0, N_("Keyfile/disk path."), 0, ARG_TYPE_STRING},
+ {"keyfile-offset", 'O', 0, N_("Keyfile offset (512 bit blocks)."), 0, ARG_TYPE_INT},
+ {"keyfile-size", 'l', 0, N_("Keyfile data size (in bits)."), 0, ARG_TYPE_INT},
+ {0, 0, 0, 0, 0, 0}
+ };
+
+struct grub_plainmount_args
+{
+ char *key_data, *cipher, *mode, *hash, *keyfile;
+ grub_size_t offset, size, key_size, sector_size, keyfile_offset, keyfile_size;
+ grub_disk_t disk;
+};
+typedef struct grub_plainmount_args *grub_plainmount_args_t;
+
+struct grub_plainmount_iterate_args
+{
+ char *uuid, *diskname;
+};
+
+
+/* Disk iterate callback */
+static int grub_plainmount_scan_real (const char *name, void *data)
+{
+ int ret = 0;
+ struct grub_plainmount_iterate_args *args = data;
+ grub_disk_t source = NULL, disk = NULL;
+ struct grub_partition *partition;
+ struct grub_gpt_partentry entry;
+ grub_gpt_part_guid_t *guid;
+ /* UUID format: AAAABBBB-CCCC-DDDD-EEEE-FFFFFFFFFFFF + '\0' */
+ char uuid[37] = "";
+
+ source = grub_disk_open (name);
+ if (!source)
+ goto exit;
+ if (!source->partition)
+ goto exit;
+ partition = source->partition;
+ if (grub_strcmp (partition->partmap->name, "gpt") != 0)
+ goto exit;
+ disk = grub_disk_open (source->name);
+ if (!disk)
+ goto exit;
+ if (grub_disk_read (disk, partition->offset, partition->index,
+ sizeof(entry), &entry) != GRUB_ERR_NONE)
+ goto exit;
+ guid = &entry.guid;
+ grub_snprintf (uuid, sizeof(uuid),
+ "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
+ grub_le_to_cpu32 (guid->data1),
+ grub_le_to_cpu16 (guid->data2),
+ grub_le_to_cpu16 (guid->data3),
+ guid->data4[0], guid->data4[1], guid->data4[2],
+ guid->data4[3], guid->data4[4], guid->data4[5],
+ guid->data4[6], guid->data4[7]);
+ if (grub_strcasecmp (args->uuid, uuid) == 0)
+ {
+ args->diskname = grub_strdup (name);
+ ret = 1;
+ }
+
+exit:
+ if (source)
+ grub_disk_close (source);
+ if (disk)
+ grub_disk_close (disk);
+ return ret;
+}
+
+
+/* Get partition name from UUID */
+static char* plainmount_get_diskname_from_uuid (char *uuid)
+{
+ struct grub_plainmount_iterate_args args = {uuid, NULL};
+ if (grub_device_iterate (&grub_plainmount_scan_real, &args) == 1
+ && args.diskname)
+ return args.diskname;
+ else
+ return NULL;
+}
+
+
+/* Support use case: -d <UUID>/dir/keyfile */
+static char*
+plainmount_uuid_path_to_disk_path (char *uuid_path)
+{
+ char *slash = grub_strchr (uuid_path, '/');
+ if (slash)
+ {
+ *slash = '\0';
+ char *diskname = plainmount_get_diskname_from_uuid (uuid_path);
+ if (!diskname)
+ {
+ *slash = '/';
+ return NULL;
+ }
+
+ /* "(" + diskname + ")/" + path_after_first_slash + '\0' */
+ int str_size = grub_strlen ("(") +
+ grub_strlen (diskname) +
+ grub_strlen (")/") +
+ grub_strlen (slash+1) + 1; /* "some/path" */
+ char *new_diskname = grub_zalloc (str_size);
+ if (!new_diskname)
+ {
+ grub_free (diskname);
+ *slash = '/';
+ return NULL;
+ }
+ grub_snprintf (new_diskname, str_size, "(%s)/%s", diskname, slash+1);
+ *slash = '/';
+ return new_diskname;
+ }
+ else
+ return plainmount_get_diskname_from_uuid (uuid_path);
+}
+
+
+/* Configure cryptodevice sector size (-z option), default - 512 byte */
+static grub_err_t
+plainmount_configure_sectors (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
+{
+ grub_disk_addr_t total_sectors;
+
+ /* Check whether disk can be accessed */
+ if (!cargs->size &&
+ grub_disk_native_sectors (cargs->disk) == GRUB_DISK_SIZE_UNKNOWN)
+ return grub_error (GRUB_ERR_BAD_DEVICE,
+ N_("cannot determine disk %s size"),
+ cargs->disk->name);
+
+ /* cryptsetup allows only 512/1024/2048/4096 byte sectors */
+ switch (cargs->sector_size)
+ {
+ case 512:
+ dev->log_sector_size = 9;
+ break;
+ case 1024:
+ dev->log_sector_size = 10;
+ break;
+ case 2048:
+ dev->log_sector_size = 11;
+ break;
+ case 4096:
+ dev->log_sector_size = 12;
+ break;
+ default:
+ grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("invalid sector size -z %"PRIuGRUB_SIZE
+ ", only 512/1024/2048/4096 are allowed"),
+ cargs->sector_size);
+ grub_print_error ();
+ return GRUB_ERR_BAD_ARGUMENT;
+ }
+
+ /* Offset is always given in terms of number of 512 byte sectors. */
+ dev->offset_sectors = grub_divmod64 (cargs->offset*512,
+ cargs->sector_size, NULL);
+
+ if (cargs->size)
+ total_sectors = cargs->size;
+ else
+ total_sectors = grub_disk_native_sectors (cargs->disk);
+
+ /* Calculate disk sectors in terms of log_sector_size */
+ total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
+ dev->log_sector_size);
+ dev->total_sectors = total_sectors - dev->offset_sectors;
+ grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"PRIuGRUB_SIZE
+ ", offset_sectors=%"PRIuGRUB_SIZE"\n", dev->log_sector_size,
+ dev->total_sectors, dev->offset_sectors);
+ return GRUB_ERR_NONE;
+}
+
+
+/* Hashes a password into a key and stores it with cipher. */
+static grub_err_t
+plainmount_configure_password (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
+{
+ const gcry_md_spec_t *hash = NULL;
+ grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = derived_hash;
+ char *p;
+ unsigned int round, i;
+ unsigned int len, size;
+ char *part = NULL;
+ gcry_err_code_t code;
+
+ /* Check hash */
+ hash = grub_crypto_lookup_md_by_name (cargs->hash);
+ if (!hash)
+ return grub_error (GRUB_ERR_FILE_NOT_FOUND,
+ N_("couldn't load %s hash (perhaps a typo?)"),
+ cargs->hash);
+
+ /* Check key size */
+ if (cargs->key_size > GRUB_CRYPTODISK_MAX_KEYLEN ||
+ hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("invalid key size %"PRIuGRUB_SIZE
+ " (exceeds maximum %d bits)"),
+ cargs->key_size, GRUB_CRYPTODISK_MAX_KEYLEN * 8);
+ dev->hash = hash;
+
+ grub_disk_t source = cargs->disk;
+ part = grub_partition_get_name (source->partition);
+ grub_printf_ (N_("Enter passphrase for %s%s%s: "), source->name,
+ source->partition != NULL ? "," : "",
+ part != NULL ? part : N_("UNKNOWN"));
+ grub_free (part);
+
+ if (!grub_password_get (cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
+ grub_error (GRUB_ERR_BAD_ARGUMENT, N_("password not supplied"));
+
+ /* Hack to support the "none" hash */
+ if (dev->hash)
+ len = dev->hash->mdlen;
+ else
+ len = cargs->key_size;
+
+ p = grub_malloc (cargs->key_size + 2 + cargs->key_size / len);
+ if (!p)
+ return GRUB_ERR_OUT_OF_MEMORY;
+
+ for (round = 0, size = cargs->key_size; size; round++, dh += len, size -= len)
+ {
+ for (i = 0; i < round; i++)
+ p[i] = 'A';
+
+ grub_strcpy (p + i, cargs->key_data);
+
+ if (len > size)
+ len = size;
+
+ grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
+ }
+ grub_free (p);
+ code = grub_cryptodisk_setkey (dev, derived_hash, cargs->key_size);
+ grub_dprintf ("plainmount", "password crypto status is %d\n", code);
+ if (code != GPG_ERR_NO_ERROR)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("cannot set key from password. "
+ "Check keysize/hash/cipher options."));
+ else
+ return GRUB_ERR_NONE;
+}
+
+
+/* Read keyfile as a file */
+static grub_err_t
+plainmount_configure_keyfile (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
+{
+ grub_file_t keyfile;
+ grub_err_t err;
+ gcry_err_code_t code;
+
+ keyfile = grub_file_open (cargs->keyfile, GRUB_FILE_TYPE_NONE);
+ if (!keyfile)
+ {
+ /* Try to parse keyfile path as UUID path */
+ char *real_path = plainmount_uuid_path_to_disk_path (cargs->keyfile);
+ if (!real_path)
+ {
+ err = grub_error (GRUB_ERR_FILE_NOT_FOUND,
+ N_("cannot open keyfile %s as UUID or real path"),
+ cargs->keyfile);
+ goto error;
+ }
+ grub_dprintf ("plainmount", "UUID %s converted to %s\n",
+ cargs->keyfile, real_path);
+ keyfile = grub_file_open (real_path, GRUB_FILE_TYPE_NONE);
+ if (!keyfile)
+ {
+ err = grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile %s"),
+ real_path);
+ goto error;
+ }
+ }
+
+ if (grub_file_seek (keyfile, cargs->keyfile_offset) == (grub_off_t)-1)
+ {
+ err = grub_error (GRUB_ERR_FILE_READ_ERROR,
+ N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
+ cargs->keyfile_offset);
+ goto error;
+ }
+
+ if (cargs->keyfile_size)
+ {
+ if (cargs->keyfile_size > (keyfile->size - cargs->keyfile_offset))
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("Specified key size (%"PRIuGRUB_SIZE") is too small "
+ "for keyfile size (%"PRIuGRUB_SIZE") and offset (%"
+ PRIuGRUB_SIZE")"),
+ cargs->keyfile_size, keyfile->size,
+ cargs->keyfile_offset);
+ goto error;
+ }
+
+ cargs->key_size = cargs->keyfile_size;
+ }
+ else
+ cargs->key_size = keyfile->size - cargs->keyfile_offset;
+
+ if (grub_file_read (keyfile, cargs->key_data, cargs->key_size) !=
+ (grub_ssize_t) cargs->key_size)
+ {
+ err = grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading key file"));
+ goto error;
+ }
+
+ code = grub_cryptodisk_setkey (dev, (grub_uint8_t*) cargs->key_data,
+ cargs->key_size);
+ grub_dprintf ("plainmount", "keyfile: setkey() status %d\n", code);
+ if (code != GPG_ERR_NO_ERROR)
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("cannot set key from keyfile %s. "
+ "Check keysize/cipher/hash options."),
+ cargs->keyfile);
+ goto error;
+ }
+ else
+ return GRUB_ERR_NONE;
+
+error:
+ grub_print_error ();
+ return err;
+}
+
+
+/* Read keyfile as a disk segment */
+static grub_err_t
+plainmount_configure_keydisk (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
+{
+ grub_err_t err;
+ grub_disk_t keydisk = NULL;
+ char* keydisk_name = NULL;
+ gcry_err_code_t code;
+ grub_uint64_t total_sectors;
+
+ keydisk_name = grub_file_get_device_name (cargs->keyfile);
+ keydisk = keydisk_name ? grub_disk_open (keydisk_name) : NULL;
+ if (!keydisk)
+ {
+ /* Try to parse keyfile path as UUID path */
+ keydisk_name = plainmount_uuid_path_to_disk_path (cargs->keyfile);
+ if (!keydisk_name)
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("unable to open disk %s as UUID or real path"),
+ cargs->keyfile);
+ goto error;
+ }
+ keydisk = grub_disk_open (keydisk_name);
+ if (!keydisk)
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unable to open disk %s"),
+ keydisk_name);
+ goto error;
+ }
+ }
+
+ total_sectors = grub_disk_native_sectors (keydisk);
+ if (total_sectors == GRUB_DISK_SIZE_UNKNOWN)
+ {
+ err = grub_error (GRUB_ERR_BAD_DEVICE,
+ N_("unable to determine size of disk %s"),
+ keydisk_name);
+ goto error;
+ }
+ total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
+ keydisk->log_sector_size);
+
+ if (GRUB_ERR_NONE != grub_disk_read (keydisk, 0, cargs->keyfile_offset,
+ cargs->keyfile_size, cargs->key_data))
+ {
+ err = grub_error (GRUB_ERR_READ_ERROR, N_("failed to read from disk %s"),
+ keydisk_name);
+ goto error;
+ }
+ code = grub_cryptodisk_setkey (dev, (grub_uint8_t*) cargs->key_data,
+ cargs->key_size);
+ grub_dprintf ("plainmount", "keydisk: setkey() status %d\n", code);
+ if (code != GPG_ERR_NO_ERROR)
+ {
+ grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("cannot set key from keydisk %s. "
+ "Check keysize/cipher/hash options."),
+ cargs->keyfile);
+ goto error;
+ }
+ err = GRUB_ERR_NONE;
+ goto cleanup;
+
+error:
+ grub_print_error ();
+
+cleanup:
+ grub_free (keydisk_name);
+ if (keydisk)
+ grub_disk_close (keydisk);
+ return err;
+}
+
+
+/* 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;
+ struct grub_plainmount_args cargs = {0};
+ grub_cryptodisk_t dev = NULL;
+ char *diskname = NULL, *disklast = NULL;
+ grub_size_t len;
+ grub_err_t err = GRUB_ERR_BUG;
+ const char *p = NULL;
+
+ if (argc < 1)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
+
+ /* Open SOURCE disk */
+ diskname = args[0];
+ len = grub_strlen (diskname);
+ if (len && diskname[0] == '(' && diskname[len - 1] == ')')
+ {
+ disklast = &diskname[len - 1];
+ *disklast = '\0';
+ diskname++;
+ }
+ cargs.disk = grub_disk_open (diskname);
+ if (!cargs.disk)
+ {
+ if (disklast)
+ *disklast = ')';
+ char *real_name = plainmount_get_diskname_from_uuid (diskname);
+ if (real_name)
+ {
+ /* diskname must point to hdX,gptY, not to UUID */
+ diskname = real_name;
+ grub_dprintf ("plainmount", "deduced partition %s from UUID %s\n",
+ real_name, args[0]);
+ cargs.disk = grub_disk_open (diskname);
+ if (!cargs.disk)
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("cannot open disk %s specified as UUID %s"),
+ diskname, args[0]);
+ goto error;
+ }
+ }
+ else
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("cannot open disk %s by name or by UUID"), diskname);
+ goto error;
+ }
+ }
+
+ /* Process plainmount command arguments */
+ cargs.hash = grub_strdup (state[0].set ? state[0].arg : GRUB_PLAINMOUNT_DIGEST);
+ cargs.cipher = grub_strdup (state[1].set ? state[1].arg : GRUB_PLAINMOUNT_CIPHER);
+ cargs.keyfile = state[6].set ? grub_strdup (state[6].arg) : NULL;
+ if (!cargs.hash || !cargs.cipher || (!cargs.keyfile && state[6].set))
+ {
+ err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
+ goto error;
+ }
+ cargs.offset = state[2].set ? grub_strtoul (state[2].arg, &p, 0) : 0;
+ if (state[2].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized disk offset"));
+ goto error;
+ }
+ cargs.size = (state[3].set ? grub_strtoul (state[3].arg, &p, 0) : 0) * 512;
+ if (state[3].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized disk size"));
+ goto error;
+ }
+ cargs.key_size = (state[4].set ? grub_strtoul (state[4].arg, &p, 0) :
+ GRUB_PLAINMOUNT_KEY_SIZE) / 8;
+ if (state[4].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
+ goto error;
+ }
+ cargs.sector_size = state[5].set ? grub_strtoul (state[5].arg, &p, 0) :
+ GRUB_PLAINMOUNT_SECTOR_SIZE;
+ if (state[5].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector size"));
+ goto error;
+ }
+ cargs.keyfile_offset = (state[7].set ? grub_strtoul (state[7].arg, &p, 0) : 0) * 512;
+ if (state[7].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized keyfile offset"));
+ goto error;
+ }
+ cargs.keyfile_size = (state[8].set ? grub_strtoul (state[8].arg, &p, 0) : 0) / 8;
+ if (state[8].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized keyfile size"));
+ goto error;
+ }
+
+ /* Check cipher mode */
+ cargs.mode = grub_strchr (cargs.cipher,'-');
+ if (!cargs.mode)
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher mode"));
+ goto error;
+ }
+ else
+ *cargs.mode++ = 0;
+
+ /* Check keyfile size */
+ if (cargs.keyfile && cargs.keyfile_size > GRUB_CRYPTODISK_MAX_KEYLEN)
+ {
+ err = grub_error (GRUB_ERR_OUT_OF_RANGE,
+ N_("key file size exceeds maximum size (%d)"),
+ GRUB_CRYPTODISK_MAX_KEYLEN);
+ goto error;
+ }
+
+ /* Create cryptodisk object and test cipher */
+ dev = grub_zalloc (sizeof *dev);
+ if (!dev)
+ {
+ err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
+ goto error;
+ }
+
+ /* Check cipher */
+ if (grub_cryptodisk_setcipher (dev, cargs.cipher, cargs.mode) != GRUB_ERR_NONE)
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher %s"), cargs.cipher);
+ goto error;
+ }
+
+ /* Warn if hash and keyfile are both provided */
+ if (cargs.keyfile && state[0].arg)
+ grub_printf_ (N_("warning: hash parameter is ignored if keyfile is specified\n"));
+
+ /* Warn if key file args are provided without key file */
+ if (!state[6].set && (state[7].set || state[8].set))
+ grub_printf_ (N_("warning: keyfile offset (-O) and size (-l) arguments "
+ "are ignored without keyfile (-d)\n"));
+
+ /* Warn if hash was not set */
+ if (!state[0].set && !cargs.keyfile)
+ grub_printf_ (N_("warning: using password and hash is not set, using default %s\n"),
+ cargs.hash);
+
+ /* Warn if cipher was not set */
+ if (!state[1].set)
+ grub_printf_ (N_("warning: cipher not set, using default %s\n"),
+ GRUB_PLAINMOUNT_CIPHER);
+
+ /* Warn if key size was not set */
+ if (!state[4].set)
+ grub_printf_ (N_("warning: key size not set, using default %"PRIuGRUB_SIZE" bits\n"),
+ cargs.key_size * 8);
+
+ err = plainmount_configure_sectors (dev, &cargs);
+ if (err != GRUB_ERR_NONE)
+ goto error;
+
+ grub_dprintf ("plainmount",
+ "parameters: cipher=%s, hash=%s, key_size=%"PRIuGRUB_SIZE", keyfile=%s, "
+ "keyfile offset=%"PRIuGRUB_SIZE", key file size=%"PRIuGRUB_SIZE"\n",
+ cargs.cipher, cargs.hash, cargs.key_size,
+ cargs.keyfile ? cargs.keyfile : NULL,
+ cargs.keyfile_offset, cargs.keyfile_size);
+
+ dev->modname = "plainmount";
+ dev->source_disk = cargs.disk;
+ grub_memcpy (dev->uuid, GRUB_PLAINMOUNT_UUID, sizeof (dev->uuid));
+ COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (GRUB_PLAINMOUNT_UUID));
+
+ /* For password or keyfile */
+ cargs.key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+ if (!cargs.key_data)
+ {
+ err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
+ goto error;
+ }
+
+ /* Configure keyfile/keydisk/password */
+ if (cargs.keyfile)
+ if (grub_strchr (cargs.keyfile, '/'))
+ err = plainmount_configure_keyfile (dev, &cargs);
+ else
+ err = plainmount_configure_keydisk (dev, &cargs);
+ else /* password */
+ err = plainmount_configure_password (dev, &cargs);
+ if (err != GRUB_ERR_NONE)
+ goto error;
+
+ err = grub_cryptodisk_insert (dev, diskname, cargs.disk);
+ if (err == GRUB_ERR_NONE)
+ {
+ grub_printf_ ("disk %s mounted as crypto%"PRIuGRUB_SIZE" in plain mode.\n",
+ dev->source, dev->id);
+ return err;
+ }
+ else
+ grub_printf_ (N_("cannot initialize cryptodisk. "
+ "Check cipher/key size/hash arguments\n"));
+
+error:
+ grub_free (cargs.hash);
+ grub_free (cargs.cipher);
+ grub_free (cargs.keyfile);
+ grub_free (cargs.key_data);
+ if (cargs.disk)
+ grub_disk_close (cargs.disk);
+ return err;
+}
+
+static grub_extcmd_t cmd;
+GRUB_MOD_INIT (plainmount)
+{
+ cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
+ N_("[-h hash] [-c cipher] [-o offset] [-s size] "
+ "[-k key-size] [-z sector-size] [-d keyfile] "
+ "[-O keyfile offset] [-l keyfile-size] <SOURCE>"),
+ N_("Open partition encrypted in plain mode."), options);
+}
+
+GRUB_MOD_FINI (plainmount)
+{
+ grub_unregister_extcmd (cmd);
+}
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
2022-01-30 19:40 [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode Maxim Fomin
@ 2022-01-31 11:15 ` Milan Broz
2022-01-31 17:40 ` Maxim Fomin
2022-02-01 2:30 ` Glenn Washburn
1 sibling, 1 reply; 8+ messages in thread
From: Milan Broz @ 2022-01-31 11:15 UTC (permalink / raw)
To: The development of GNU GRUB, Maxim Fomin
On 30/01/2022 20:40, Maxim Fomin wrote:
> This patch introduces support for plain encryption mode (plain dm-crypt) via
> new module and command named 'plainmount'. The command allows to open devices
> encrypted in plain mode (without LUKS) with following syntax:
> +
...
> +#define GRUB_PLAINMOUNT_UUID "00000000000000000000000000000000"
> +#define GRUB_PLAINMOUNT_CIPHER "aes-cbc-essiv:sha256"
> +#define GRUB_PLAINMOUNT_DIGEST "ripemd160"
> +#define GRUB_PLAINMOUNT_KEY_SIZE 256
> +#define GRUB_PLAINMOUNT_SECTOR_SIZE 512
Sooner or later we will have to change this default in cryptsetup
(as ripemd and CBC mode are no longer the best options) and you
you will create data corruption here (as there is no way in plain
mode to check that the mode is set correctly).
Not sure if it is possible, but in normal system it should be required
that these parameters are set in /etc/crypttab, grub should perhaps
require explicit setting them in config too?
Milan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
2022-01-31 11:15 ` Milan Broz
@ 2022-01-31 17:40 ` Maxim Fomin
2022-02-01 2:45 ` Glenn Washburn
0 siblings, 1 reply; 8+ messages in thread
From: Maxim Fomin @ 2022-01-31 17:40 UTC (permalink / raw)
To: grub-devel
------- Original Message -------
On Monday, January 31st, 2022 at 14:15, Milan Broz <gmazyland@gmail.com> wrote:
> On 30/01/2022 20:40, Maxim Fomin wrote:
>
> > This patch introduces support for plain encryption mode (plain dm-crypt) via
> >
> > new module and command named 'plainmount'. The command allows to open devices
> >
> > encrypted in plain mode (without LUKS) with following syntax:
> >
> > +
>
> ...
>
> > +#define GRUB_PLAINMOUNT_UUID "00000000000000000000000000000000"
> >
> > +#define GRUB_PLAINMOUNT_CIPHER "aes-cbc-essiv:sha256"
> >
> > +#define GRUB_PLAINMOUNT_DIGEST "ripemd160"
> >
> > +#define GRUB_PLAINMOUNT_KEY_SIZE 256
> >
> > +#define GRUB_PLAINMOUNT_SECTOR_SIZE 512
>
> Sooner or later we will have to change this default in cryptsetup
>
> (as ripemd and CBC mode are no longer the best options) and you
>
> you will create data corruption here (as there is no way in plain
>
> mode to check that the mode is set correctly).
>
> Not sure if it is possible, but in normal system it should be required
>
> that these parameters are set in /etc/crypttab, grub should perhaps
>
> require explicit setting them in config too?
>
> Milan
These default values can be fixed or removed altogether. Some additional points.
1. Just to clarify - mounting device in plain mode with wrong parameters (it is irrelevant
whether default values or explicitly set arguments are wrong) does not case data corruption
per se, data corruption requires writing to such device. And writing to unformatted device
is complicated by the fact that no command operating on files will work with such device.
User can corrupt data only if he uses some grub command which has functionality to write
arbitrary byte data to arbitrary device (like 'dd' command) at arbitrary device offset.
2. Still, solving this problem may be possible. After decryption additional check can be added -
test whether some fs is recognized on target device (or some additional virtual device like
'lvm' appeared). I will investigate whether this is technically possible and add such checks in
the next version of the patch.
3. The question of how to keep settings (on live system) is still open because plainmount
command is not supported by grub-mkconfig yet. I agree that support in grub-mkconfig must
require explicit parameter setting. These parameters can be specified in /etc/default/grub
like GRUB_CRYPTODISK_PLAINMOUNT_CIPHER=xxx (and similar for additional parameters). Then
grub-mkconfig must be changed to recognize such variables.
Currently plainmount just processes arguments - whether they were generated by mkconfig and
came from /etc/default/grub (hypothetically) or user directly edited grub.cfg is not in scope.
What can be done at this step is removing default arguments for cipher/hash/key size
- this will stimulate user to check arguments correctness. Under current implementation plainmount
will silently create incorrect disk when these parameters are omiited, so such change will
impove data safety. Also, from my understanding these cipher/hash types are used rarely nowadays,
(meaning these default values will likely not be used anyway) so this is additional argument
to remove them.
Best regards,
Maxim Fomin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
2022-01-30 19:40 [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode Maxim Fomin
2022-01-31 11:15 ` Milan Broz
@ 2022-02-01 2:30 ` Glenn Washburn
2022-02-02 14:55 ` Maxim Fomin
1 sibling, 1 reply; 8+ messages in thread
From: Glenn Washburn @ 2022-02-01 2:30 UTC (permalink / raw)
To: Maxim Fomin; +Cc: The development of GNU GRUB
On Sun, 30 Jan 2022 19:40:43 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> This patch introduces support for plain encryption mode (plain dm-crypt) via
> new module and command named 'plainmount'. The command allows to open devices
> encrypted in plain mode (without LUKS) with following syntax:
>
> plainmount <SOURCE> -h hash -c cipher -o offset -s size -k key-size
> -z sector-size -d keyfile -O keyfile offset -l keyfile-size
>
> <SOURCE> can be some partition or GPT UUID, keyfile can be <UUID>/some/path.
I'm not in favor of the keyfile UUID prefix for paths. Why not just use
the normal device syntax?
> Despite moving plain mode code into separate module, it still depends on
> cryptodisk.c because cryptodisk module defines whole grub infrastructure
> of encrypted devices.
>
> Principal feature of this patch is supporting GPT partition UUID to point to
> specific partition. The patch also includes two other minor features: using
> disk block as a key (instead of keyfile) and support for 1024/2048/4096 byte
> blocks in plain mode (cryptsetup added large block support in plain mode after
> introducing them in LUKS2 mode - since circa version 2.4).
>
> This is fully reworked version of '0004-Cryptomount-support-plain-dm-crypt'
> patch which is member of John Lane crypto enhancement patch series. It also
> includes '0007-Add-support-for-using-a-whole-device-as-a-keyfile' patch. They
> were sent in grub-devel mailing list as:
>
> From a8f9e3dcece89c179e89414abe89985c7ab1e03f Mon Sep 17 00:00:00 2001
> From: John Lane <john@lane.uk.net>
> Date: Fri, 26 Jun 2015 22:09:52 +0100
> Subject: [PATCH 4/7] Cryptomount support plain dm-crypt
>
> From ef720d0d44b8d97a83950ced0df1ce1bcf8cd988 Mon Sep 17 00:00:00 2001
> From: Paul Gideon Dann <pdgiddie@gmail.com>
> Date: Tue, 19 Jul 2016 12:36:37 +0100
> Subject: [PATCH 7/7] Add support for using a whole device as a keyfile
> ---
> grub-core/Makefile.core.def | 5 +
> grub-core/disk/plainmount.c | 678 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 683 insertions(+)
> create mode 100644 grub-core/disk/plainmount.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8022e1c0a..ce8478055 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1184,6 +1184,11 @@ module = {
> common = disk/luks.c;
> };
>
> +module = {
> + name = plainmount;
> + common = disk/plainmount.c;
> +};
> +
> module = {
> name = luks2;
> common = disk/luks2.c;
> diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
> new file mode 100644
> index 000000000..4654cec96
> --- /dev/null
> +++ b/grub-core/disk/plainmount.c
> @@ -0,0 +1,678 @@
> +/*
> + * GRUB -- GRand Unified Bootloader
> + * Copyright (C) 2003,2007,2010,2011,2019 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/>.
> + */
> +
> +#include <grub/cryptodisk.h>
> +#include <grub/dl.h>
> +#include <grub/err.h>
> +#include <grub/extcmd.h>
> +#include <grub/partition.h>
> +#include <grub/file.h>
> +#include <grub/gpt_partition.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define GRUB_PLAINMOUNT_UUID "00000000000000000000000000000000"
> +#define GRUB_PLAINMOUNT_CIPHER "aes-cbc-essiv:sha256"
> +#define GRUB_PLAINMOUNT_DIGEST "ripemd160"
> +#define GRUB_PLAINMOUNT_KEY_SIZE 256
> +#define GRUB_PLAINMOUNT_SECTOR_SIZE 512
> +
> +static const struct grub_arg_option options[] =
> + {
> + /* TRANSLATORS: It's still restricted to this module only. */
It would be nice to allow specifying a password as an argument (-p)
like in cryptomount for consistency. It'll allow tests to no need to
write a keyfile also.
> + {"hash", 'h', 0, N_("Password hash."), 0, ARG_TYPE_STRING},
> + {"cipher", 'c', 0, N_("Password cipher."), 0, ARG_TYPE_STRING},
> + {"offset", 'o', 0, N_("Device offset (512 bit blocks)."), 0, ARG_TYPE_INT},
s/bit/byte/
And why 512-byte blocks? The LUKS2 header stores offset as bytes,
perhaps bytes should be used here too.
> + {"size", 'b', 0, N_("Size of device (512 byte blocks)."), 0, ARG_TYPE_INT},
Same as above.
> + {"key-size", 's', 0, N_("Key size (in bits)."), 0, ARG_TYPE_INT},
> + {"sector-size", 'z', 0, N_("Device sector size."), 0, ARG_TYPE_INT},
'S' instead of 'z' makes more sense to me.
> + {"keyfile", 'd', 0, N_("Keyfile/disk path."), 0, ARG_TYPE_STRING},
> + {"keyfile-offset", 'O', 0, N_("Keyfile offset (512 bit blocks)."), 0, ARG_TYPE_INT},
s/bit/byte/
> + {"keyfile-size", 'l', 0, N_("Keyfile data size (in bits)."), 0, ARG_TYPE_INT},
> + {0, 0, 0, 0, 0, 0}
> + };
> +
> +struct grub_plainmount_args
> +{
> + char *key_data, *cipher, *mode, *hash, *keyfile;
> + grub_size_t offset, size, key_size, sector_size, keyfile_offset, keyfile_size;
> + grub_disk_t disk;
> +};
> +typedef struct grub_plainmount_args *grub_plainmount_args_t;
> +
> +struct grub_plainmount_iterate_args
> +{
> + char *uuid, *diskname;
> +};
> +
> +
> +/* Disk iterate callback */
> +static int grub_plainmount_scan_real (const char *name, void *data)
> +{
> + int ret = 0;
> + struct grub_plainmount_iterate_args *args = data;
> + grub_disk_t source = NULL, disk = NULL;
> + struct grub_partition *partition;
> + struct grub_gpt_partentry entry;
> + grub_gpt_part_guid_t *guid;
> + /* UUID format: AAAABBBB-CCCC-DDDD-EEEE-FFFFFFFFFFFF + '\0' */
> + char uuid[37] = "";
> +
> + source = grub_disk_open (name);
> + if (!source)
> + goto exit;
> + if (!source->partition)
> + goto exit;
> + partition = source->partition;
> + if (grub_strcmp (partition->partmap->name, "gpt") != 0)
> + goto exit;
As noted elsewhere, I'm not in favor of these checks, nor the forcing
of the volume to be on a partition.
> + disk = grub_disk_open (source->name);
> + if (!disk)
> + goto exit;
> + if (grub_disk_read (disk, partition->offset, partition->index,
> + sizeof(entry), &entry) != GRUB_ERR_NONE)
> + goto exit;
> + guid = &entry.guid;
> + grub_snprintf (uuid, sizeof(uuid),
> + "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> + grub_le_to_cpu32 (guid->data1),
> + grub_le_to_cpu16 (guid->data2),
> + grub_le_to_cpu16 (guid->data3),
> + guid->data4[0], guid->data4[1], guid->data4[2],
> + guid->data4[3], guid->data4[4], guid->data4[5],
> + guid->data4[6], guid->data4[7]);
> + if (grub_strcasecmp (args->uuid, uuid) == 0)
> + {
> + args->diskname = grub_strdup (name);
> + ret = 1;
> + }
> +
> +exit:
> + if (source)
> + grub_disk_close (source);
> + if (disk)
> + grub_disk_close (disk);
> + return ret;
> +}
> +
> +
> +/* Get partition name from UUID */
> +static char* plainmount_get_diskname_from_uuid (char *uuid)
> +{
> + struct grub_plainmount_iterate_args args = {uuid, NULL};
> + if (grub_device_iterate (&grub_plainmount_scan_real, &args) == 1
> + && args.diskname)
> + return args.diskname;
> + else
> + return NULL;
> +}
> +
> +
> +/* Support use case: -d <UUID>/dir/keyfile */
> +static char*
> +plainmount_uuid_path_to_disk_path (char *uuid_path)
> +{
> + char *slash = grub_strchr (uuid_path, '/');
> + if (slash)
> + {
> + *slash = '\0';
> + char *diskname = plainmount_get_diskname_from_uuid (uuid_path);
> + if (!diskname)
> + {
> + *slash = '/';
> + return NULL;
> + }
> +
> + /* "(" + diskname + ")/" + path_after_first_slash + '\0' */
> + int str_size = grub_strlen ("(") +
> + grub_strlen (diskname) +
> + grub_strlen (")/") +
> + grub_strlen (slash+1) + 1; /* "some/path" */
> + char *new_diskname = grub_zalloc (str_size);
> + if (!new_diskname)
> + {
> + grub_free (diskname);
> + *slash = '/';
> + return NULL;
> + }
> + grub_snprintf (new_diskname, str_size, "(%s)/%s", diskname, slash+1);
> + *slash = '/';
> + return new_diskname;
> + }
> + else
> + return plainmount_get_diskname_from_uuid (uuid_path);
> +}
> +
> +
> +/* Configure cryptodevice sector size (-z option), default - 512 byte */
> +static grub_err_t
> +plainmount_configure_sectors (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
> +{
> + grub_disk_addr_t total_sectors;
> +
> + /* Check whether disk can be accessed */
> + if (!cargs->size &&
> + grub_disk_native_sectors (cargs->disk) == GRUB_DISK_SIZE_UNKNOWN)
> + return grub_error (GRUB_ERR_BAD_DEVICE,
> + N_("cannot determine disk %s size"),
> + cargs->disk->name);
> +
> + /* cryptsetup allows only 512/1024/2048/4096 byte sectors */
I prefer using code like in luks2.c, something like (without the bad
formatting):
/* Sector size should be one of 512, 1024, 2048, or 4096. */
if (!(cargs->sector_size == 512 || cargs->sector_size == 1024 ||
cargs->sector_size == 2048 || cargs->sector_size == 4096))
{
return grub_error (GRUB_ERR_BAD_ARGUMENT,
N_("invalid sector size -z %"PRIuGRUB_SIZE
", only 512/1024/2048/4096 are allowed"),
cargs->sector_size);
}
dev->log_sector_size = grub_log2ull(cargs->sector_size);
> + switch (cargs->sector_size)
> + {
> + case 512:
> + dev->log_sector_size = 9;
> + break;
> + case 1024:
> + dev->log_sector_size = 10;
> + break;
> + case 2048:
> + dev->log_sector_size = 11;
> + break;
> + case 4096:
> + dev->log_sector_size = 12;
> + break;
> + default:
> + grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("invalid sector size -z %"PRIuGRUB_SIZE
> + ", only 512/1024/2048/4096 are allowed"),
> + cargs->sector_size);
> + grub_print_error ();
> + return GRUB_ERR_BAD_ARGUMENT;
Should just set the error and return. Further up the call stack the
error should be printed. So do something like
return grub_error (GRUB_ERR_BAD_ARGUMENT,
N_("invalid sector size -z %"PRIuGRUB_SIZE
", only 512/1024/2048/4096 are allowed"),
cargs->sector_size);
> + }
> +
> + /* Offset is always given in terms of number of 512 byte sectors. */
> + dev->offset_sectors = grub_divmod64 (cargs->offset*512,
512 should be a constant, assuming we still want to have input in
512-byte sectors.
> + cargs->sector_size, NULL);
> +
> + if (cargs->size)
> + total_sectors = cargs->size;
> + else
> + total_sectors = grub_disk_native_sectors (cargs->disk);
> +
> + /* Calculate disk sectors in terms of log_sector_size */
> + total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
> + dev->log_sector_size);
> + dev->total_sectors = total_sectors - dev->offset_sectors;
> + grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"PRIuGRUB_SIZE
> + ", offset_sectors=%"PRIuGRUB_SIZE"\n", dev->log_sector_size,
> + dev->total_sectors, dev->offset_sectors);
> + return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Hashes a password into a key and stores it with cipher. */
> +static grub_err_t
> +plainmount_configure_password (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
> +{
> + const gcry_md_spec_t *hash = NULL;
> + grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = derived_hash;
> + char *p;
> + unsigned int round, i;
> + unsigned int len, size;
> + char *part = NULL;
> + gcry_err_code_t code;
> +
> + /* Check hash */
> + hash = grub_crypto_lookup_md_by_name (cargs->hash);
> + if (!hash)
> + return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> + N_("couldn't load %s hash (perhaps a typo?)"),
> + cargs->hash);
> +
> + /* Check key size */
> + if (cargs->key_size > GRUB_CRYPTODISK_MAX_KEYLEN ||
> + hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
Should this check for the message digest length of the hash function be
a different error? And why is it needed anyway?
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("invalid key size %"PRIuGRUB_SIZE
> + " (exceeds maximum %d bits)"),
> + cargs->key_size, GRUB_CRYPTODISK_MAX_KEYLEN * 8);
> + dev->hash = hash;
> +
> + grub_disk_t source = cargs->disk;
> + part = grub_partition_get_name (source->partition);
> + grub_printf_ (N_("Enter passphrase for %s%s%s: "), source->name,
> + source->partition != NULL ? "," : "",
> + part != NULL ? part : N_("UNKNOWN"));
> + grub_free (part);
> +
> + if (!grub_password_get (cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> + grub_error (GRUB_ERR_BAD_ARGUMENT, N_("password not supplied"));
It would be nice if this was refactored to look like the recent changes
to cryptodisk (ie. not asking for password here, but passing in the
password/keyfile data).
> +
> + /* Hack to support the "none" hash */
> + if (dev->hash)
> + len = dev->hash->mdlen;
> + else
> + len = cargs->key_size;
> +
> + p = grub_malloc (cargs->key_size + 2 + cargs->key_size / len);
> + if (!p)
> + return GRUB_ERR_OUT_OF_MEMORY;
> +
> + for (round = 0, size = cargs->key_size; size; round++, dh += len, size -= len)
> + {
> + for (i = 0; i < round; i++)
> + p[i] = 'A';
> +
> + grub_strcpy (p + i, cargs->key_data);
> +
> + if (len > size)
> + len = size;
> +
> + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> + }
> + grub_free (p);
> + code = grub_cryptodisk_setkey (dev, derived_hash, cargs->key_size);
> + grub_dprintf ("plainmount", "password crypto status is %d\n", code);
This should be done in the if block below, no one cares if the code is
success.
> + if (code != GPG_ERR_NO_ERROR)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("cannot set key from password. "
> + "Check keysize/hash/cipher options."));
I don't like that the GPG error is getting swallowed. Perhaps reutrn
grub_crypto_gcry_error (code) here.
> + else
> + return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Read keyfile as a file */
> +static grub_err_t
> +plainmount_configure_keyfile (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
> +{
> + grub_file_t keyfile;
> + grub_err_t err;
> + gcry_err_code_t code;
> +
> + keyfile = grub_file_open (cargs->keyfile, GRUB_FILE_TYPE_NONE);
> + if (!keyfile)
> + {
> + /* Try to parse keyfile path as UUID path */
> + char *real_path = plainmount_uuid_path_to_disk_path (cargs->keyfile);
> + if (!real_path)
> + {
> + err = grub_error (GRUB_ERR_FILE_NOT_FOUND,
> + N_("cannot open keyfile %s as UUID or real path"),
> + cargs->keyfile);
> + goto error;
> + }
> + grub_dprintf ("plainmount", "UUID %s converted to %s\n",
> + cargs->keyfile, real_path);
> + keyfile = grub_file_open (real_path, GRUB_FILE_TYPE_NONE);
> + if (!keyfile)
> + {
> + err = grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile %s"),
> + real_path);
> + goto error;
> + }
> + }
> +
> + if (grub_file_seek (keyfile, cargs->keyfile_offset) == (grub_off_t)-1)
> + {
> + err = grub_error (GRUB_ERR_FILE_READ_ERROR,
> + N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
> + cargs->keyfile_offset);
> + goto error;
> + }
> +
> + if (cargs->keyfile_size)
> + {
> + if (cargs->keyfile_size > (keyfile->size - cargs->keyfile_offset))
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("Specified key size (%"PRIuGRUB_SIZE") is too small "
> + "for keyfile size (%"PRIuGRUB_SIZE") and offset (%"
> + PRIuGRUB_SIZE")"),
> + cargs->keyfile_size, keyfile->size,
> + cargs->keyfile_offset);
> + goto error;
> + }
> +
> + cargs->key_size = cargs->keyfile_size;
> + }
> + else
> + cargs->key_size = keyfile->size - cargs->keyfile_offset;
> +
> + if (grub_file_read (keyfile, cargs->key_data, cargs->key_size) !=
> + (grub_ssize_t) cargs->key_size)
> + {
> + err = grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading key file"));
> + goto error;
> + }
> +
> + code = grub_cryptodisk_setkey (dev, (grub_uint8_t*) cargs->key_data,
> + cargs->key_size);
> + grub_dprintf ("plainmount", "keyfile: setkey() status %d\n", code);
> + if (code != GPG_ERR_NO_ERROR)
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("cannot set key from keyfile %s. "
> + "Check keysize/cipher/hash options."),
> + cargs->keyfile);
Ditto.
> + goto error;
> + }
> + else
> + return GRUB_ERR_NONE;
> +
> +error:
> + grub_print_error ();
Probabl shouldn't do this either and let it happen further up the call
stack.
> + return err;
> +}
> +
> +
> +/* Read keyfile as a disk segment */
> +static grub_err_t
> +plainmount_configure_keydisk (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
> +{
> + grub_err_t err;
> + grub_disk_t keydisk = NULL;
> + char* keydisk_name = NULL;
> + gcry_err_code_t code;
> + grub_uint64_t total_sectors;
> +
> + keydisk_name = grub_file_get_device_name (cargs->keyfile);
> + keydisk = keydisk_name ? grub_disk_open (keydisk_name) : NULL;
> + if (!keydisk)
> + {
> + /* Try to parse keyfile path as UUID path */
> + keydisk_name = plainmount_uuid_path_to_disk_path (cargs->keyfile);
> + if (!keydisk_name)
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("unable to open disk %s as UUID or real path"),
> + cargs->keyfile);
> + goto error;
> + }
> + keydisk = grub_disk_open (keydisk_name);
> + if (!keydisk)
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unable to open disk %s"),
> + keydisk_name);
> + goto error;
> + }
> + }
> +
> + total_sectors = grub_disk_native_sectors (keydisk);
> + if (total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> + {
> + err = grub_error (GRUB_ERR_BAD_DEVICE,
> + N_("unable to determine size of disk %s"),
> + keydisk_name);
> + goto error;
> + }
> + total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
> + keydisk->log_sector_size);
> +
> + if (GRUB_ERR_NONE != grub_disk_read (keydisk, 0, cargs->keyfile_offset,
> + cargs->keyfile_size, cargs->key_data))
> + {
> + err = grub_error (GRUB_ERR_READ_ERROR, N_("failed to read from disk %s"),
> + keydisk_name);
> + goto error;
> + }
> + code = grub_cryptodisk_setkey (dev, (grub_uint8_t*) cargs->key_data,
> + cargs->key_size);
> + grub_dprintf ("plainmount", "keydisk: setkey() status %d\n", code);
> + if (code != GPG_ERR_NO_ERROR)
> + {
> + grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("cannot set key from keydisk %s. "
> + "Check keysize/cipher/hash options."),
> + cargs->keyfile);
Here also.
> + goto error;
> + }
> + err = GRUB_ERR_NONE;
> + goto cleanup;
> +
> +error:
> + grub_print_error ();
Also, let's not.
> +
> +cleanup:
> + grub_free (keydisk_name);
> + if (keydisk)
> + grub_disk_close (keydisk);
> + return err;
> +}
> +
> +
> +/* 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;
> + struct grub_plainmount_args cargs = {0};
> + grub_cryptodisk_t dev = NULL;
> + char *diskname = NULL, *disklast = NULL;
> + grub_size_t len;
> + grub_err_t err = GRUB_ERR_BUG;
> + const char *p = NULL;
> +
> + if (argc < 1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
> +
> + /* Open SOURCE disk */
> + diskname = args[0];
> + len = grub_strlen (diskname);
> + if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> + {
> + disklast = &diskname[len - 1];
> + *disklast = '\0';
> + diskname++;
> + }
> + cargs.disk = grub_disk_open (diskname);
> + if (!cargs.disk)
> + {
> + if (disklast)
> + *disklast = ')';
> + char *real_name = plainmount_get_diskname_from_uuid (diskname);
> + if (real_name)
> + {
> + /* diskname must point to hdX,gptY, not to UUID */
> + diskname = real_name;
> + grub_dprintf ("plainmount", "deduced partition %s from UUID %s\n",
> + real_name, args[0]);
> + cargs.disk = grub_disk_open (diskname);
> + if (!cargs.disk)
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("cannot open disk %s specified as UUID %s"),
> + diskname, args[0]);
> + goto error;
> + }
> + }
> + else
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("cannot open disk %s by name or by UUID"), diskname);
> + goto error;
> + }
> + }
> +
> + /* Process plainmount command arguments */
> + cargs.hash = grub_strdup (state[0].set ? state[0].arg : GRUB_PLAINMOUNT_DIGEST);
> + cargs.cipher = grub_strdup (state[1].set ? state[1].arg : GRUB_PLAINMOUNT_CIPHER);
> + cargs.keyfile = state[6].set ? grub_strdup (state[6].arg) : NULL;
> + if (!cargs.hash || !cargs.cipher || (!cargs.keyfile && state[6].set))
> + {
> + err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> + goto error;
> + }
> + cargs.offset = state[2].set ? grub_strtoul (state[2].arg, &p, 0) : 0;
Why not use grub_strtoull? And its a good idea to se grub_errno =
GRUB_ERR_NONE before this so you don't get false positives.
> + if (state[2].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized disk offset"));
> + goto error;
> + }
> + cargs.size = (state[3].set ? grub_strtoul (state[3].arg, &p, 0) : 0) * 512;
Ditto.
> + if (state[3].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized disk size"));
> + goto error;
> + }
> + cargs.key_size = (state[4].set ? grub_strtoul (state[4].arg, &p, 0) :
> + GRUB_PLAINMOUNT_KEY_SIZE) / 8;
> + if (state[4].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
> + goto error;
> + }
> + cargs.sector_size = state[5].set ? grub_strtoul (state[5].arg, &p, 0) :
> + GRUB_PLAINMOUNT_SECTOR_SIZE;
> + if (state[5].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector size"));
> + goto error;
> + }
> + cargs.keyfile_offset = (state[7].set ? grub_strtoul (state[7].arg, &p, 0) : 0) * 512;
> + if (state[7].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized keyfile offset"));
> + goto error;
> + }
> + cargs.keyfile_size = (state[8].set ? grub_strtoul (state[8].arg, &p, 0) : 0) / 8;
> + if (state[8].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized keyfile size"));
> + goto error;
> + }
> +
> + /* Check cipher mode */
> + cargs.mode = grub_strchr (cargs.cipher,'-');
> + if (!cargs.mode)
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher mode"));
> + goto error;
> + }
> + else
> + *cargs.mode++ = 0;
s/0/'\0'/
> +
> + /* Check keyfile size */
> + if (cargs.keyfile && cargs.keyfile_size > GRUB_CRYPTODISK_MAX_KEYLEN)
> + {
> + err = grub_error (GRUB_ERR_OUT_OF_RANGE,
> + N_("key file size exceeds maximum size (%d)"),
> + GRUB_CRYPTODISK_MAX_KEYLEN);
> + goto error;
> + }
> +
> + /* Create cryptodisk object and test cipher */
> + dev = grub_zalloc (sizeof *dev);
> + if (!dev)
> + {
> + err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> + goto error;
> + }
> +
> + /* Check cipher */
> + if (grub_cryptodisk_setcipher (dev, cargs.cipher, cargs.mode) != GRUB_ERR_NONE)
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher %s"), cargs.cipher);
> + goto error;
> + }
> +
> + /* Warn if hash and keyfile are both provided */
> + if (cargs.keyfile && state[0].arg)
> + grub_printf_ (N_("warning: hash parameter is ignored if keyfile is specified\n"));
> +
> + /* Warn if key file args are provided without key file */
> + if (!state[6].set && (state[7].set || state[8].set))
> + grub_printf_ (N_("warning: keyfile offset (-O) and size (-l) arguments "
> + "are ignored without keyfile (-d)\n"));
> +
> + /* Warn if hash was not set */
> + if (!state[0].set && !cargs.keyfile)
> + grub_printf_ (N_("warning: using password and hash is not set, using default %s\n"),
> + cargs.hash);
> +
> + /* Warn if cipher was not set */
> + if (!state[1].set)
> + grub_printf_ (N_("warning: cipher not set, using default %s\n"),
> + GRUB_PLAINMOUNT_CIPHER);
> +
> + /* Warn if key size was not set */
> + if (!state[4].set)
> + grub_printf_ (N_("warning: key size not set, using default %"PRIuGRUB_SIZE" bits\n"),
> + cargs.key_size * 8);
> +
> + err = plainmount_configure_sectors (dev, &cargs);
> + if (err != GRUB_ERR_NONE)
> + goto error;
> +
> + grub_dprintf ("plainmount",
> + "parameters: cipher=%s, hash=%s, key_size=%"PRIuGRUB_SIZE", keyfile=%s, "
> + "keyfile offset=%"PRIuGRUB_SIZE", key file size=%"PRIuGRUB_SIZE"\n",
> + cargs.cipher, cargs.hash, cargs.key_size,
> + cargs.keyfile ? cargs.keyfile : NULL,
> + cargs.keyfile_offset, cargs.keyfile_size);
> +
> + dev->modname = "plainmount";
> + dev->source_disk = cargs.disk;
> + grub_memcpy (dev->uuid, GRUB_PLAINMOUNT_UUID, sizeof (dev->uuid));
I don't like this because this makes the collection of all dev uuids
not unique if there are more than one plainmount volume mounted. I
haven't thought about how this might cause related problems, but its a
concern. Would it better to create a ramdom prefix and allow for say
256 plainmounts? Maybe use a module level static global to keep track
of number of plain mounts.
> + COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (GRUB_PLAINMOUNT_UUID));
> +
> + /* For password or keyfile */
> + cargs.key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> + if (!cargs.key_data)
> + {
> + err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> + goto error;
> + }
> +
> + /* Configure keyfile/keydisk/password */
> + if (cargs.keyfile)
> + if (grub_strchr (cargs.keyfile, '/'))
> + err = plainmount_configure_keyfile (dev, &cargs);
> + else
> + err = plainmount_configure_keydisk (dev, &cargs);
> + else /* password */
> + err = plainmount_configure_password (dev, &cargs);
> + if (err != GRUB_ERR_NONE)
> + goto error;
> +
> + err = grub_cryptodisk_insert (dev, diskname, cargs.disk);
> + if (err == GRUB_ERR_NONE)
> + {
> + grub_printf_ ("disk %s mounted as crypto%"PRIuGRUB_SIZE" in plain mode.\n",
> + dev->source, dev->id);
This should be a grub_dprintf().
> + return err;
> + }
> + else
> + grub_printf_ (N_("cannot initialize cryptodisk. "
> + "Check cipher/key size/hash arguments\n"));
This isn't that useful, I would drop it. An error will get printed up
the call stack when a return value other than GRUB_ERR_NONE is returned.
> +
> +error:
> + grub_free (cargs.hash);
> + grub_free (cargs.cipher);
> + grub_free (cargs.keyfile);
> + grub_free (cargs.key_data);
These frees should be done even if no error, otherwise you're going to
have a memory leak in the successful case.
> + if (cargs.disk)
> + grub_disk_close (cargs.disk);
> + return err;
> +}
> +
> +static grub_extcmd_t cmd;
> +GRUB_MOD_INIT (plainmount)
> +{
> + cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
> + N_("[-h hash] [-c cipher] [-o offset] [-s size] "
> + "[-k key-size] [-z sector-size] [-d keyfile] "
> + "[-O keyfile offset] [-l keyfile-size] <SOURCE>"),
This might be more accurate if this
[-d keyfile] [-O keyfile offset] [-l keyfile-size]
were prelaced by
[-d keyfile [-O keyfile offset] [-l keyfile-size]]
> + N_("Open partition encrypted in plain mode."), options);
> +}
> +
> +GRUB_MOD_FINI (plainmount)
> +{
> + grub_unregister_extcmd (cmd);
> +}
> --
> 2.35.1
>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
2022-01-31 17:40 ` Maxim Fomin
@ 2022-02-01 2:45 ` Glenn Washburn
2022-02-01 7:16 ` Milan Broz
0 siblings, 1 reply; 8+ messages in thread
From: Glenn Washburn @ 2022-02-01 2:45 UTC (permalink / raw)
To: Maxim Fomin; +Cc: The development of GNU GRUB
On Mon, 31 Jan 2022 17:40:24 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> ------- Original Message -------
>
> On Monday, January 31st, 2022 at 14:15, Milan Broz <gmazyland@gmail.com> wrote:
>
> > On 30/01/2022 20:40, Maxim Fomin wrote:
> >
> > > This patch introduces support for plain encryption mode (plain dm-crypt) via
> > >
> > > new module and command named 'plainmount'. The command allows to open devices
> > >
> > > encrypted in plain mode (without LUKS) with following syntax:
> > >
> > > +
> >
> > ...
> >
> > > +#define GRUB_PLAINMOUNT_UUID "00000000000000000000000000000000"
> > >
> > > +#define GRUB_PLAINMOUNT_CIPHER "aes-cbc-essiv:sha256"
> > >
> > > +#define GRUB_PLAINMOUNT_DIGEST "ripemd160"
> > >
> > > +#define GRUB_PLAINMOUNT_KEY_SIZE 256
> > >
> > > +#define GRUB_PLAINMOUNT_SECTOR_SIZE 512
> >
> > Sooner or later we will have to change this default in cryptsetup
> >
> > (as ripemd and CBC mode are no longer the best options) and you
> >
> > you will create data corruption here (as there is no way in plain
> >
> > mode to check that the mode is set correctly).
> >
> > Not sure if it is possible, but in normal system it should be required
> >
> > that these parameters are set in /etc/crypttab, grub should perhaps
> >
> > require explicit setting them in config too?
> >
> > Milan
>
> These default values can be fixed or removed altogether. Some additional points.
>
> 1. Just to clarify - mounting device in plain mode with wrong parameters (it is irrelevant
> whether default values or explicitly set arguments are wrong) does not case data corruption
> per se, data corruption requires writing to such device. And writing to unformatted device
> is complicated by the fact that no command operating on files will work with such device.
> User can corrupt data only if he uses some grub command which has functionality to write
> arbitrary byte data to arbitrary device (like 'dd' command) at arbitrary device offset.
I agree. I don't think data corruption is a big issue because GRUB
doesn't (generally) write to disks.
> 2. Still, solving this problem may be possible. After decryption additional check can be added -
> test whether some fs is recognized on target device (or some additional virtual device like
> 'lvm' appeared). I will investigate whether this is technically possible and add such checks in
> the next version of the patch.
I thought about this too, but I don't think the lack of a filesystem or
structured data should indicate that the decryption key was bad. It
could be another plainmount or random key data.
> 3. The question of how to keep settings (on live system) is still open because plainmount
> command is not supported by grub-mkconfig yet. I agree that support in grub-mkconfig must
> require explicit parameter setting. These parameters can be specified in /etc/default/grub
> like GRUB_CRYPTODISK_PLAINMOUNT_CIPHER=xxx (and similar for additional parameters). Then
> grub-mkconfig must be changed to recognize such variables.
I think this can be a separate patch series and that grub-mkconfig
should not support plainmount until such a change is made. So it must
be configured by hand. I think this is fine for the niche audience that
will use this.
> Currently plainmount just processes arguments - whether they were generated by mkconfig and
> came from /etc/default/grub (hypothetically) or user directly edited grub.cfg is not in scope.
>
> What can be done at this step is removing default arguments for cipher/hash/key size
> - this will stimulate user to check arguments correctness. Under current implementation plainmount
> will silently create incorrect disk when these parameters are omiited, so such change will
> impove data safety. Also, from my understanding these cipher/hash types are used rarely nowadays,
> (meaning these default values will likely not be used anyway) so this is additional argument
> to remove them.
I'm fine with default arguments, _if_ the defaults are common (eg. if
cryptsetup has defaults use those). As a user of plainmount, I think
I'd want to always be explicit incase the defaults changed. Of course,
not having defaults is okay too.
Glenn
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
2022-02-01 2:45 ` Glenn Washburn
@ 2022-02-01 7:16 ` Milan Broz
0 siblings, 0 replies; 8+ messages in thread
From: Milan Broz @ 2022-02-01 7:16 UTC (permalink / raw)
To: The development of GNU GRUB, Glenn Washburn, Maxim Fomin
On 01/02/2022 03:45, Glenn Washburn wrote:
> On Mon, 31 Jan 2022 17:40:24 +0000
> Maxim Fomin <maxim@fomin.one> wrote:
>
>> ------- Original Message -------
>>
>> On Monday, January 31st, 2022 at 14:15, Milan Broz <gmazyland@gmail.com> wrote:
>>
>>> On 30/01/2022 20:40, Maxim Fomin wrote:
>>>
>>>> This patch introduces support for plain encryption mode (plain dm-crypt) via
>>>>
>>>> new module and command named 'plainmount'. The command allows to open devices
>>>>
>>>> encrypted in plain mode (without LUKS) with following syntax:
>>>>
>>>> +
>>>
>>> ...
>>>
>>>> +#define GRUB_PLAINMOUNT_UUID "00000000000000000000000000000000"
>>>>
>>>> +#define GRUB_PLAINMOUNT_CIPHER "aes-cbc-essiv:sha256"
>>>>
>>>> +#define GRUB_PLAINMOUNT_DIGEST "ripemd160"
>>>>
>>>> +#define GRUB_PLAINMOUNT_KEY_SIZE 256
>>>>
>>>> +#define GRUB_PLAINMOUNT_SECTOR_SIZE 512
>>>
>>> Sooner or later we will have to change this default in cryptsetup
>>>
>>> (as ripemd and CBC mode are no longer the best options) and you
>>>
>>> you will create data corruption here (as there is no way in plain
>>>
>>> mode to check that the mode is set correctly).
>>>
>>> Not sure if it is possible, but in normal system it should be required
>>>
>>> that these parameters are set in /etc/crypttab, grub should perhaps
>>>
>>> require explicit setting them in config too?
>>>
>>> Milan
>>
>> These default values can be fixed or removed altogether. Some additional points.
>>
>> 1. Just to clarify - mounting device in plain mode with wrong parameters (it is irrelevant
>> whether default values or explicitly set arguments are wrong) does not case data corruption
>> per se, data corruption requires writing to such device. And writing to unformatted device
>> is complicated by the fact that no command operating on files will work with such device.
>> User can corrupt data only if he uses some grub command which has functionality to write
>> arbitrary byte data to arbitrary device (like 'dd' command) at arbitrary device offset.
>
> I agree. I don't think data corruption is a big issue because GRUB
> doesn't (generally) write to disks.
Yes, what I meant is in some situations like when the encryption use 4k sectors while
default here is 512Bytes. Then you can have the first 512B decrypted correctly
(the rest 7 512B sectors is garbage, as it fits in 4k larger sector - depends on IV setting).
This is enough to detect FS signature but obviously not correct data.
(In most real cases you get garbage in the whole sector.)
It is perhaps ok to ignore it, though.
...
> I'm fine with default arguments, _if_ the defaults are common (eg. if
> cryptsetup has defaults use those). As a user of plainmount, I think
> I'd want to always be explicit incase the defaults changed. Of course,
> not having defaults is okay too.
FYI: The defaults from patch are default for plain device in cryptsetup since
version 1.1.0 (2010).
But we have to change it soon, perhaps in 2.5 release.
Milan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
2022-02-01 2:30 ` Glenn Washburn
@ 2022-02-02 14:55 ` Maxim Fomin
2022-02-04 22:07 ` Glenn Washburn
0 siblings, 1 reply; 8+ messages in thread
From: Maxim Fomin @ 2022-02-02 14:55 UTC (permalink / raw)
To: grub-devel
------- Original Message -------
On Tuesday, February 1st, 2022 at 5:30, Glenn Washburn <development@efficientek.com> wrote:
> On Sun, 30 Jan 2022 19:40:43 +0000
>
> Maxim Fomin maxim@fomin.one wrote:
>
> > This patch introduces support for plain encryption mode (plain dm-crypt) via
> >
> > new module and command named 'plainmount'. The command allows to open devices
> >
> > encrypted in plain mode (without LUKS) with following syntax:
> >
> > plainmount <SOURCE> -h hash -c cipher -o offset -s size -k key-size
> >
> > -z sector-size -d keyfile -O keyfile offset -l keyfile-size
> >
> > <SOURCE> can be some partition or GPT UUID, keyfile can be <UUID>/some/path.
>
> I'm not in favor of the keyfile UUID prefix for paths. Why not just use
>
> the normal device syntax?
Normal syntax can be used, it is base case syntax. Not supporting UUID for
keyfile (but supporting for device parameter) significantly limits possible
use cases because keyfile can be located on another drive. For example,
several linux wiki/manuals present one of several setup scenarios where
keyfile is located on a separate disk. This makes having support for UUID
for keyfile is almost surely necessary. However, support for UUID in
plainmount can be temporarily disabled untill the situation with search
command patch is resolved.
> >
> > +static const struct grub_arg_option options[] =
> >
> > - {
> > - /* TRANSLATORS: It's still restricted to this module only. */
>
> It would be nice to allow specifying a password as an argument (-p)
>
> like in cryptomount for consistency. It'll allow tests to no need to
>
> write a keyfile also.
I agree if this option is alternative way of requesting password and the
ability to type password remains.
>
> > - {"hash", 'h', 0, N_("Password hash."), 0, ARG_TYPE_STRING},
> > - {"cipher", 'c', 0, N_("Password cipher."), 0, ARG_TYPE_STRING},
> > - {"offset", 'o', 0, N_("Device offset (512 bit blocks)."), 0, ARG_TYPE_INT},
>
> s/bit/byte/
>
> And why 512-byte blocks? The LUKS2 header stores offset as bytes,
>
> perhaps bytes should be used here too.
I followed cryptsetup command syntax for plain mode - it asks offset in terms of
512 byte segments because it is easier to type such number than number in bytes.
In general offset can be large and also inconvenient to type, but in practice
offset starts from 1-100 MiB, so this number is easier to remember (why it is
done this way in cryptsetup is my guess). Anyway, specifying in this way is more
familiar for cryptsetup users, but this can be changed.
> >
> > +/* Disk iterate callback */
> >
> > +static int grub_plainmount_scan_real (const char *name, void *data)
> >
> > +{
> >
> > - int ret = 0;
> > - struct grub_plainmount_iterate_args *args = data;
> > - grub_disk_t source = NULL, disk = NULL;
> > - struct grub_partition *partition;
> > - struct grub_gpt_partentry entry;
> > - grub_gpt_part_guid_t *guid;
> > - /* UUID format: AAAABBBB-CCCC-DDDD-EEEE-FFFFFFFFFFFF + '\0' */
> > - char uuid[37] = "";
> >
> > - source = grub_disk_open (name);
> > - if (!source)
> > - goto exit;
> >
> >
> > - if (!source->partition)
> > - goto exit;
> >
> >
> > - partition = source->partition;
> > - if (grub_strcmp (partition->partmap->name, "gpt") != 0)
> > - goto exit;
> >
> >
>
> As noted elsewhere, I'm not in favor of these checks, nor the forcing
>
> of the volume to be on a partition.
I didn't consider the case for disk instead of partition, will think about it.
> >
> >
> > - default:
> >
> >
> > - grub_error (GRUB_ERR_BAD_ARGUMENT,
> >
> >
> > - N_("invalid sector size -z %"PRIuGRUB_SIZE
> >
> >
> > - ", only 512/1024/2048/4096 are allowed"),
> >
> >
> > - cargs->sector_size);
> >
> >
> > - grub_print_error ();
> >
> >
> > - return GRUB_ERR_BAD_ARGUMENT;
> >
> >
>
> Should just set the error and return. Further up the call stack the
>
> error should be printed. So do something like
>
> return grub_error (GRUB_ERR_BAD_ARGUMENT,
>
> N_("invalid sector size -z %"PRIuGRUB_SIZE
>
> ", only 512/1024/2048/4096 are allowed"),
>
> cargs->sector_size);
Meaning move error printing from different places to the near of end of
grub_cmd_plainmount() (and possibly use grub_error_push/grub_error_pop)?
OK, I will take closer look at existing cryptodisk/luks behavior.
> > - cargs->sector_size, NULL);
> >
> >
> >
> > - if (cargs->size)
> > - total_sectors = cargs->size;
> > - else
> > - total_sectors = grub_disk_native_sectors (cargs->disk);
> >
> > - /* Calculate disk sectors in terms of log_sector_size */
> > - total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
> > - dev->log_sector_size);
> >
> >
> > - dev->total_sectors = total_sectors - dev->offset_sectors;
> > - grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"PRIuGRUB_SIZE
> > - ", offset_sectors=%"PRIuGRUB_SIZE"\\n", dev->log_sector_size,
> >
> >
> > - dev->total_sectors, dev->offset_sectors);
> >
> >
> > - return GRUB_ERR_NONE;
> >
> > +}
> >
> > +/* Hashes a password into a key and stores it with cipher. */
> >
> > +static grub_err_t
> >
> > +plainmount_configure_password (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
> >
> > +{
> >
> > - const gcry_md_spec_t *hash = NULL;
> > - grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = derived_hash;
> > - char *p;
> > - unsigned int round, i;
> > - unsigned int len, size;
> > - char *part = NULL;
> > - gcry_err_code_t code;
> >
> > - /* Check hash */
> > - hash = grub_crypto_lookup_md_by_name (cargs->hash);
> > - if (!hash)
> > - return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> > - N_("couldn't load %s hash (perhaps a typo?)"),
> >
> >
> > - cargs->hash);
> >
> >
> >
> > - /* Check key size */
> > - if (cargs->key_size > GRUB_CRYPTODISK_MAX_KEYLEN ||
> > - hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
> >
> >
>
> Should this check for the message digest length of the hash function be
>
> a different error? And why is it needed anyway?
Probably yes. This code hasn't been changed from John Lane patch (this function
is the only code which preserved itself well after reimplementing from scratch).
>
> > - return grub_error (GRUB_ERR_BAD_ARGUMENT,
> >
> >
> > - N_("invalid key size %"PRIuGRUB_SIZE
> >
> >
> > - " (exceeds maximum %d bits)"),
> >
> >
> > - cargs->key_size, GRUB_CRYPTODISK_MAX_KEYLEN * 8);
> >
> >
> > - dev->hash = hash;
> >
> > - grub_disk_t source = cargs->disk;
> > - part = grub_partition_get_name (source->partition);
> > - grub_printf_ (N_("Enter passphrase for %s%s%s: "), source->name,
> > - source->partition != NULL ? "," : "",
> >
> >
> > - part != NULL ? part : N_("UNKNOWN"));
> >
> >
> > - grub_free (part);
> >
> > - if (!grub_password_get (cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> > - grub_error (GRUB_ERR_BAD_ARGUMENT, N_("password not supplied"));
> >
> >
>
> It would be nice if this was refactored to look like the recent changes
>
> to cryptodisk (ie. not asking for password here, but passing in the
>
> password/keyfile data).
What exactly do you mean: add additional way to provide password via '-p'
option, remove ability to provide password interactively from console or
something else? Plainmount can receive key either from keyfile or from
user (via '-p' which can be implemented or via grub_password_get). There are
three separate functions which deal with each case (keyfile/keydisk are
separate cases). So, it is natural that function which deals with password
requests it.
>
> > - if (code != GPG_ERR_NO_ERROR)
> > - return grub_error (GRUB_ERR_BAD_ARGUMENT,
> >
> >
> > - N_("cannot set key from password. "
> >
> >
> > - "Check keysize/hash/cipher options."));
> >
> >
>
> I don't like that the GPG error is getting swallowed. Perhaps reutrn
>
> grub_crypto_gcry_error (code) here.
Agree that swallowing GPG error is bad, but grub_crypto_gcry_error (code) is not very helpful
because it returns GRUB_ACCESS_DENIED if gcry_error_t is not GCRY_ERR_NONE. This behavior
probably makes sense in LUKS where all crypto parameters are stored in header and cannot be
wrong, so failure to open means almost surely the password was wrong and 'access denined'
makes sense (although I consider such error message less informative than 'incorrect password').
In plain mode the situation is different. What makes plainmount to fail is not wrong password.
Providing wrong password will not make error, plainmount will silently create virtual device
full of random data. What makes set_cipher()/set_key(0 to fail is inconsistency between cipher
parameters (cipher, key size and hash). So, current grub_crypto_gcry_error implementation
definitely does not suit here.
I will think how this error can be improved. In any way passing information "Check keysize
/hash/cipher options" is reasonable because according to my experince and testing this
mismatch is the only case when set_key()/set_cipher() can return error.
> > +cleanup:
> >
> > - grub_free (keydisk_name);
> > - if (keydisk)
> > - grub_disk_close (keydisk);
> > - return err;
> >
> > +}
> >
> > +/* 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;
> > - struct grub_plainmount_args cargs = {0};
> > - grub_cryptodisk_t dev = NULL;
> > - char *diskname = NULL, *disklast = NULL;
> > - grub_size_t len;
> > - grub_err_t err = GRUB_ERR_BUG;
> > - const char *p = NULL;
> >
> > - if (argc < 1)
> > - return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
> >
> > - /* Open SOURCE disk */
> > - diskname = args[0];
> > - len = grub_strlen (diskname);
> > - if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > - {
> > - disklast = &diskname[len - 1];
> >
> >
> > - *disklast = '\\0';
> >
> >
> > - diskname++;
> >
> >
> > - }
> > - cargs.disk = grub_disk_open (diskname);
> > - if (!cargs.disk)
> > - {
> > - if (disklast)
> >
> >
> > - *disklast = ')';
> >
> >
> > - char *real_name = plainmount_get_diskname_from_uuid (diskname);
> >
> >
> > - if (real_name)
> >
> >
> > - {
> >
> >
> > - /* diskname must point to hdX,gptY, not to UUID */
> >
> >
> > - diskname = real_name;
> >
> >
> > - grub_dprintf ("plainmount", "deduced partition %s from UUID %s\\n",
> >
> >
> > - real_name, args[0]);
> >
> >
> > - cargs.disk = grub_disk_open (diskname);
> >
> >
> > - if (!cargs.disk)
> >
> >
> > - {
> >
> >
> > - err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> >
> >
> > - N_("cannot open disk %s specified as UUID %s"),
> >
> >
> > - diskname, args[0]);
> >
> >
> > - goto error;
> >
> >
> > - }
> >
> >
> > - }
> >
> >
> > - else
> >
> >
> > - {
> >
> >
> > - err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> >
> >
> > - N_("cannot open disk %s by name or by UUID"), diskname);
> >
> >
> > - goto error;
> >
> >
> > - }
> >
> >
> > - }
> >
> > - /* Process plainmount command arguments */
> > - cargs.hash = grub_strdup (state[0].set ? state[0].arg : GRUB_PLAINMOUNT_DIGEST);
> > - cargs.cipher = grub_strdup (state[1].set ? state[1].arg : GRUB_PLAINMOUNT_CIPHER);
> > - cargs.keyfile = state[6].set ? grub_strdup (state[6].arg) : NULL;
> > - if (!cargs.hash || !cargs.cipher || (!cargs.keyfile && state[6].set))
> > - {
> > - err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> >
> >
> > - goto error;
> >
> >
> > - }
> > - cargs.offset = state[2].set ? grub_strtoul (state[2].arg, &p, 0) : 0;
>
> Why not use grub_strtoull? And its a good idea to se grub_errno =
>
> GRUB_ERR_NONE before this so you don't get false positives.
I will check this in next version.
> > - /* Check keyfile size */
> > - if (cargs.keyfile && cargs.keyfile_size > GRUB_CRYPTODISK_MAX_KEYLEN)
> > - {
> > - err = grub_error (GRUB_ERR_OUT_OF_RANGE,
> >
> >
> > - N_("key file size exceeds maximum size (%d)"),
> >
> >
> > - GRUB_CRYPTODISK_MAX_KEYLEN);
> >
> >
> > - goto error;
> >
> >
> > - }
> >
> > - /* Create cryptodisk object and test cipher */
> > - dev = grub_zalloc (sizeof *dev);
> > - if (!dev)
> > - {
> > - err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> >
> >
> > - goto error;
> >
> >
> > - }
> >
> > - /* Check cipher */
> > - if (grub_cryptodisk_setcipher (dev, cargs.cipher, cargs.mode) != GRUB_ERR_NONE)
> > - {
> > - err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher %s"), cargs.cipher);
> >
> >
> > - goto error;
> >
> >
> > - }
> >
> > - /* Warn if hash and keyfile are both provided */
> > - if (cargs.keyfile && state[0].arg)
> > - grub_printf_ (N_("warning: hash parameter is ignored if keyfile is specified\n"));
> >
> > - /* Warn if key file args are provided without key file */
> > - if (!state[6].set && (state[7].set || state[8].set))
> > - grub_printf_ (N_("warning: keyfile offset (-O) and size (-l) arguments "
> > - "are ignored without keyfile (-d)\\n"));
> >
> >
> >
> > - /* Warn if hash was not set */
> > - if (!state[0].set && !cargs.keyfile)
> > - grub_printf_ (N_("warning: using password and hash is not set, using default %s\n"),
> > - cargs.hash);
> >
> >
> >
> > - /* Warn if cipher was not set */
> > - if (!state[1].set)
> > - grub_printf_ (N_("warning: cipher not set, using default %s\n"),
> > - GRUB_PLAINMOUNT_CIPHER);
> >
> >
> >
> > - /* Warn if key size was not set */
> > - if (!state[4].set)
> > - grub_printf_ (N_("warning: key size not set, using default %"PRIuGRUB_SIZE" bits\n"),
> > - cargs.key_size * 8);
> >
> >
> >
> > - err = plainmount_configure_sectors (dev, &cargs);
> > - if (err != GRUB_ERR_NONE)
> > - goto error;
> >
> > - grub_dprintf ("plainmount",
> > - "parameters: cipher=%s, hash=%s, key_size=%"PRIuGRUB_SIZE", keyfile=%s, "
> >
> >
> > - "keyfile offset=%"PRIuGRUB_SIZE", key file size=%"PRIuGRUB_SIZE"\\n",
> >
> >
> > - cargs.cipher, cargs.hash, cargs.key_size,
> >
> >
> > - cargs.keyfile ? cargs.keyfile : NULL,
> >
> >
> > - cargs.keyfile_offset, cargs.keyfile_size);
> >
> >
> >
> > - dev->modname = "plainmount";
> > - dev->source_disk = cargs.disk;
> > - grub_memcpy (dev->uuid, GRUB_PLAINMOUNT_UUID, sizeof (dev->uuid));
>
> I don't like this because this makes the collection of all dev uuids
>
> not unique if there are more than one plainmount volume mounted. I
>
> haven't thought about how this might cause related problems, but its a
>
> concern. Would it better to create a ramdom prefix and allow for say
>
> 256 plainmounts? Maybe use a module level static global to keep track
>
> of number of plain mounts.
I will think about this.
>
> > - COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (GRUB_PLAINMOUNT_UUID));
> >
> > - /* For password or keyfile */
> > - cargs.key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> > - if (!cargs.key_data)
> > - {
> > - err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> >
> >
> > - goto error;
> >
> >
> > - }
> >
> > - /* Configure keyfile/keydisk/password */
> > - if (cargs.keyfile)
> > - if (grub_strchr (cargs.keyfile, '/'))
> > - err = plainmount_configure_keyfile (dev, &cargs);
> >
> >
> > - else
> > - err = plainmount_configure_keydisk (dev, &cargs);
> >
> >
> > - else /* password */
> > - err = plainmount_configure_password (dev, &cargs);
> > - if (err != GRUB_ERR_NONE)
> > - goto error;
> >
> > - err = grub_cryptodisk_insert (dev, diskname, cargs.disk);
> > - if (err == GRUB_ERR_NONE)
> > - {
> > - grub_printf_ ("disk %s mounted as crypto%"PRIuGRUB_SIZE" in plain mode.\\n",
> >
> >
> > - dev->source, dev->id);
> >
> >
>
> This should be a grub_dprintf().
I think some text mesage should be printed to indicate the status, because if nothing is
printed the user will not know what happened (if we think about working in shell). LUKS
currently prints message 'Slot 0 opened'. I will try to add fs probe checks after creating
'crypto0' device and reconsider what should be printed (or not) in different cases.
> > +error:
> >
> > - grub_free (cargs.hash);
> > - grub_free (cargs.cipher);
> > - grub_free (cargs.keyfile);
> > - grub_free (cargs.key_data);
>
> These frees should be done even if no error, otherwise you're going to
>
> have a memory leak in the successful case.
Does key_data memory must be freed to? I run a test with freeing it and
received memory error like ('Alloc bad magic') when cryptdisk.c/disk.c were
reading disk. I looked at cryptodisk internals - it appeared cryptodisk saves
pointers to some of these parameters, so my impression was that this memory
is needed for cryptodisk, but may I got it wrong. I will recheck this issue.
> > - if (cargs.disk)
> > - grub_disk_close (cargs.disk);
> > - return err;
> >
> > +}
> >
> > +static grub_extcmd_t cmd;
> >
> > +GRUB_MOD_INIT (plainmount)
> >
> > +{
> >
> > - cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
> > - N_("[-h hash] [-c cipher] [-o offset] [-s size] "
> >
> >
> > - "[-k key-size] [-z sector-size] [-d keyfile] "
> >
> >
> > - "[-O keyfile offset] [-l keyfile-size] <SOURCE>"),
> >
> >
>
> This might be more accurate if this
>
> [-d keyfile] [-O keyfile offset] [-l keyfile-size]
>
> were prelaced by
>
> [-d keyfile [-O keyfile offset] [-l keyfile-size]]
Do you mean the entire list of arguments must be placed in []
brackets? Like [ ... [-d keyfile] ... [-l keyfile-size] ] ?
Best regards,
Maxim Fomin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
2022-02-02 14:55 ` Maxim Fomin
@ 2022-02-04 22:07 ` Glenn Washburn
0 siblings, 0 replies; 8+ messages in thread
From: Glenn Washburn @ 2022-02-04 22:07 UTC (permalink / raw)
To: Maxim Fomin; +Cc: The development of GNU GRUB
On Wed, 02 Feb 2022 14:55:46 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> ------- Original Message -------
>
> On Tuesday, February 1st, 2022 at 5:30, Glenn Washburn <development@efficientek.com> wrote:
>
> > On Sun, 30 Jan 2022 19:40:43 +0000
> >
> > Maxim Fomin maxim@fomin.one wrote:
> >
> > > This patch introduces support for plain encryption mode (plain dm-crypt) via
> > >
> > > new module and command named 'plainmount'. The command allows to open devices
> > >
> > > encrypted in plain mode (without LUKS) with following syntax:
> > >
> > > plainmount <SOURCE> -h hash -c cipher -o offset -s size -k key-size
> > >
> > > -z sector-size -d keyfile -O keyfile offset -l keyfile-size
> > >
> > > <SOURCE> can be some partition or GPT UUID, keyfile can be <UUID>/some/path.
> >
> > I'm not in favor of the keyfile UUID prefix for paths. Why not just use
> >
> > the normal device syntax?
>
> Normal syntax can be used, it is base case syntax. Not supporting UUID for
> keyfile (but supporting for device parameter) significantly limits possible
> use cases because keyfile can be located on another drive. For example,
> several linux wiki/manuals present one of several setup scenarios where
> keyfile is located on a separate disk. This makes having support for UUID
> for keyfile is almost surely necessary. However, support for UUID in
> plainmount can be temporarily disabled untill the situation with search
> command patch is resolved.
I think this code should assume that the user/script knows how to get
the full path needed.
>
> > >
> > > +static const struct grub_arg_option options[] =
> > >
> > > - {
> > > - /* TRANSLATORS: It's still restricted to this module only. */
> >
> > It would be nice to allow specifying a password as an argument (-p)
> >
> > like in cryptomount for consistency. It'll allow tests to no need to
> >
> > write a keyfile also.
>
> I agree if this option is alternative way of requesting password and the
> ability to type password remains.
Yes, in addition to. The password will still be prompted if no -k or -p
arguments are present.
>
> >
> > > - {"hash", 'h', 0, N_("Password hash."), 0, ARG_TYPE_STRING},
> > > - {"cipher", 'c', 0, N_("Password cipher."), 0, ARG_TYPE_STRING},
> > > - {"offset", 'o', 0, N_("Device offset (512 bit blocks)."), 0, ARG_TYPE_INT},
> >
> > s/bit/byte/
> >
> > And why 512-byte blocks? The LUKS2 header stores offset as bytes,
> >
> > perhaps bytes should be used here too.
>
> I followed cryptsetup command syntax for plain mode - it asks offset in terms of
> 512 byte segments because it is easier to type such number than number in bytes.
> In general offset can be large and also inconvenient to type, but in practice
> offset starts from 1-100 MiB, so this number is easier to remember (why it is
> done this way in cryptsetup is my guess). Anyway, specifying in this way is more
> familiar for cryptsetup users, but this can be changed.
I was misremembering that cryptsetup used a number in 512-byte sectors
for those arguments. In the LUKS header is stored in bytes. I like the
multiplicative suffixes that "dd" uses and bytes are assumed if no
suffix. Although it uses 'b' as the suffix for 512-bytes, which seems
non-obvious to me. Parted uses 's' for 512-byte sectors which I like
better.
> > >
> > > +/* Disk iterate callback */
> > >
> > > +static int grub_plainmount_scan_real (const char *name, void *data)
> > >
> > > +{
> > >
> > > - int ret = 0;
> > > - struct grub_plainmount_iterate_args *args = data;
> > > - grub_disk_t source = NULL, disk = NULL;
> > > - struct grub_partition *partition;
> > > - struct grub_gpt_partentry entry;
> > > - grub_gpt_part_guid_t *guid;
> > > - /* UUID format: AAAABBBB-CCCC-DDDD-EEEE-FFFFFFFFFFFF + '\0' */
> > > - char uuid[37] = "";
> > >
> > > - source = grub_disk_open (name);
> > > - if (!source)
> > > - goto exit;
> > >
> > >
> > > - if (!source->partition)
> > > - goto exit;
> > >
> > >
> > > - partition = source->partition;
> > > - if (grub_strcmp (partition->partmap->name, "gpt") != 0)
> > > - goto exit;
> > >
> > >
> >
> > As noted elsewhere, I'm not in favor of these checks, nor the forcing
> >
> > of the volume to be on a partition.
>
> I didn't consider the case for disk instead of partition, will think about it.
>
> > >
> > >
> > > - default:
> > >
> > >
> > > - grub_error (GRUB_ERR_BAD_ARGUMENT,
> > >
> > >
> > > - N_("invalid sector size -z %"PRIuGRUB_SIZE
> > >
> > >
> > > - ", only 512/1024/2048/4096 are allowed"),
> > >
> > >
> > > - cargs->sector_size);
> > >
> > >
> > > - grub_print_error ();
> > >
> > >
> > > - return GRUB_ERR_BAD_ARGUMENT;
> > >
> > >
> >
> > Should just set the error and return. Further up the call stack the
> >
> > error should be printed. So do something like
> >
> > return grub_error (GRUB_ERR_BAD_ARGUMENT,
> >
> > N_("invalid sector size -z %"PRIuGRUB_SIZE
> >
> > ", only 512/1024/2048/4096 are allowed"),
> >
> > cargs->sector_size);
>
> Meaning move error printing from different places to the near of end of
> grub_cmd_plainmount() (and possibly use grub_error_push/grub_error_pop)?
> OK, I will take closer look at existing cryptodisk/luks behavior.
You probably shouldn't be using grub_error_push, grub_error_pop, or
grub_print_error at all. When the plainmount command returns an error
code up the stack will print the set error message.
>
>
> > > - cargs->sector_size, NULL);
> > >
> > >
> > >
> > > - if (cargs->size)
> > > - total_sectors = cargs->size;
> > > - else
> > > - total_sectors = grub_disk_native_sectors (cargs->disk);
> > >
> > > - /* Calculate disk sectors in terms of log_sector_size */
> > > - total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
> > > - dev->log_sector_size);
> > >
> > >
> > > - dev->total_sectors = total_sectors - dev->offset_sectors;
> > > - grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"PRIuGRUB_SIZE
> > > - ", offset_sectors=%"PRIuGRUB_SIZE"\\n", dev->log_sector_size,
> > >
> > >
> > > - dev->total_sectors, dev->offset_sectors);
> > >
> > >
> > > - return GRUB_ERR_NONE;
> > >
> > > +}
> > >
> > > +/* Hashes a password into a key and stores it with cipher. */
> > >
> > > +static grub_err_t
> > >
> > > +plainmount_configure_password (grub_cryptodisk_t dev, grub_plainmount_args_t cargs)
> > >
> > > +{
> > >
> > > - const gcry_md_spec_t *hash = NULL;
> > > - grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = derived_hash;
> > > - char *p;
> > > - unsigned int round, i;
> > > - unsigned int len, size;
> > > - char *part = NULL;
> > > - gcry_err_code_t code;
> > >
> > > - /* Check hash */
> > > - hash = grub_crypto_lookup_md_by_name (cargs->hash);
> > > - if (!hash)
> > > - return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> > > - N_("couldn't load %s hash (perhaps a typo?)"),
> > >
> > >
> > > - cargs->hash);
> > >
> > >
> > >
> > > - /* Check key size */
> > > - if (cargs->key_size > GRUB_CRYPTODISK_MAX_KEYLEN ||
> > > - hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
> > >
> > >
> >
> > Should this check for the message digest length of the hash function be
> >
> > a different error? And why is it needed anyway?
>
> Probably yes. This code hasn't been changed from John Lane patch (this function
> is the only code which preserved itself well after reimplementing from scratch).
>
> >
> > > - return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > >
> > >
> > > - N_("invalid key size %"PRIuGRUB_SIZE
> > >
> > >
> > > - " (exceeds maximum %d bits)"),
> > >
> > >
> > > - cargs->key_size, GRUB_CRYPTODISK_MAX_KEYLEN * 8);
> > >
> > >
> > > - dev->hash = hash;
> > >
> > > - grub_disk_t source = cargs->disk;
> > > - part = grub_partition_get_name (source->partition);
> > > - grub_printf_ (N_("Enter passphrase for %s%s%s: "), source->name,
> > > - source->partition != NULL ? "," : "",
> > >
> > >
> > > - part != NULL ? part : N_("UNKNOWN"));
> > >
> > >
> > > - grub_free (part);
> > >
> > > - if (!grub_password_get (cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> > > - grub_error (GRUB_ERR_BAD_ARGUMENT, N_("password not supplied"));
> > >
> > >
> >
> > It would be nice if this was refactored to look like the recent changes
> >
> > to cryptodisk (ie. not asking for password here, but passing in the
> >
> > password/keyfile data).
>
> What exactly do you mean: add additional way to provide password via '-p'
> option, remove ability to provide password interactively from console or
> something else? Plainmount can receive key either from keyfile or from
> user (via '-p' which can be implemented or via grub_password_get). There are
> three separate functions which deal with each case (keyfile/keydisk are
> separate cases). So, it is natural that function which deals with password
> requests it.
Ignore my comment here.
>
> >
> > > - if (code != GPG_ERR_NO_ERROR)
> > > - return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > >
> > >
> > > - N_("cannot set key from password. "
> > >
> > >
> > > - "Check keysize/hash/cipher options."));
> > >
> > >
> >
> > I don't like that the GPG error is getting swallowed. Perhaps reutrn
> >
> > grub_crypto_gcry_error (code) here.
>
> Agree that swallowing GPG error is bad, but grub_crypto_gcry_error (code) is not very helpful
> because it returns GRUB_ACCESS_DENIED if gcry_error_t is not GCRY_ERR_NONE. This behavior
> probably makes sense in LUKS where all crypto parameters are stored in header and cannot be
> wrong, so failure to open means almost surely the password was wrong and 'access denined'
> makes sense (although I consider such error message less informative than 'incorrect password').
I haven't looked into the GPG errors and assumed they were more
informative. In this case, it sounds like the error message here is
better.
>
> In plain mode the situation is different. What makes plainmount to fail is not wrong password.
> Providing wrong password will not make error, plainmount will silently create virtual device
> full of random data. What makes set_cipher()/set_key(0 to fail is inconsistency between cipher
> parameters (cipher, key size and hash). So, current grub_crypto_gcry_error implementation
> definitely does not suit here.
>
> I will think how this error can be improved. In any way passing information "Check keysize
> /hash/cipher options" is reasonable because according to my experince and testing this
> mismatch is the only case when set_key()/set_cipher() can return error.
Perhaps grub_cryptodisk_setkey should be updated to set decent error
messages.
>
>
> > > +cleanup:
> > >
> > > - grub_free (keydisk_name);
> > > - if (keydisk)
> > > - grub_disk_close (keydisk);
> > > - return err;
> > >
> > > +}
> > >
> > > +/* 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;
> > > - struct grub_plainmount_args cargs = {0};
> > > - grub_cryptodisk_t dev = NULL;
> > > - char *diskname = NULL, *disklast = NULL;
> > > - grub_size_t len;
> > > - grub_err_t err = GRUB_ERR_BUG;
> > > - const char *p = NULL;
> > >
> > > - if (argc < 1)
> > > - return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
> > >
> > > - /* Open SOURCE disk */
> > > - diskname = args[0];
> > > - len = grub_strlen (diskname);
> > > - if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > > - {
> > > - disklast = &diskname[len - 1];
> > >
> > >
> > > - *disklast = '\\0';
> > >
> > >
> > > - diskname++;
> > >
> > >
> > > - }
> > > - cargs.disk = grub_disk_open (diskname);
> > > - if (!cargs.disk)
> > > - {
> > > - if (disklast)
> > >
> > >
> > > - *disklast = ')';
> > >
> > >
> > > - char *real_name = plainmount_get_diskname_from_uuid (diskname);
> > >
> > >
> > > - if (real_name)
> > >
> > >
> > > - {
> > >
> > >
> > > - /* diskname must point to hdX,gptY, not to UUID */
> > >
> > >
> > > - diskname = real_name;
> > >
> > >
> > > - grub_dprintf ("plainmount", "deduced partition %s from UUID %s\\n",
> > >
> > >
> > > - real_name, args[0]);
> > >
> > >
> > > - cargs.disk = grub_disk_open (diskname);
> > >
> > >
> > > - if (!cargs.disk)
> > >
> > >
> > > - {
> > >
> > >
> > > - err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> > >
> > >
> > > - N_("cannot open disk %s specified as UUID %s"),
> > >
> > >
> > > - diskname, args[0]);
> > >
> > >
> > > - goto error;
> > >
> > >
> > > - }
> > >
> > >
> > > - }
> > >
> > >
> > > - else
> > >
> > >
> > > - {
> > >
> > >
> > > - err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> > >
> > >
> > > - N_("cannot open disk %s by name or by UUID"), diskname);
> > >
> > >
> > > - goto error;
> > >
> > >
> > > - }
> > >
> > >
> > > - }
> > >
> > > - /* Process plainmount command arguments */
> > > - cargs.hash = grub_strdup (state[0].set ? state[0].arg : GRUB_PLAINMOUNT_DIGEST);
> > > - cargs.cipher = grub_strdup (state[1].set ? state[1].arg : GRUB_PLAINMOUNT_CIPHER);
> > > - cargs.keyfile = state[6].set ? grub_strdup (state[6].arg) : NULL;
> > > - if (!cargs.hash || !cargs.cipher || (!cargs.keyfile && state[6].set))
> > > - {
> > > - err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > >
> > >
> > > - goto error;
> > >
> > >
> > > - }
> > > - cargs.offset = state[2].set ? grub_strtoul (state[2].arg, &p, 0) : 0;
> >
> > Why not use grub_strtoull? And its a good idea to se grub_errno =
> >
> > GRUB_ERR_NONE before this so you don't get false positives.
>
> I will check this in next version.
>
>
> > > - /* Check keyfile size */
> > > - if (cargs.keyfile && cargs.keyfile_size > GRUB_CRYPTODISK_MAX_KEYLEN)
> > > - {
> > > - err = grub_error (GRUB_ERR_OUT_OF_RANGE,
> > >
> > >
> > > - N_("key file size exceeds maximum size (%d)"),
> > >
> > >
> > > - GRUB_CRYPTODISK_MAX_KEYLEN);
> > >
> > >
> > > - goto error;
> > >
> > >
> > > - }
> > >
> > > - /* Create cryptodisk object and test cipher */
> > > - dev = grub_zalloc (sizeof *dev);
> > > - if (!dev)
> > > - {
> > > - err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> > >
> > >
> > > - goto error;
> > >
> > >
> > > - }
> > >
> > > - /* Check cipher */
> > > - if (grub_cryptodisk_setcipher (dev, cargs.cipher, cargs.mode) != GRUB_ERR_NONE)
> > > - {
> > > - err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher %s"), cargs.cipher);
> > >
> > >
> > > - goto error;
> > >
> > >
> > > - }
> > >
> > > - /* Warn if hash and keyfile are both provided */
> > > - if (cargs.keyfile && state[0].arg)
> > > - grub_printf_ (N_("warning: hash parameter is ignored if keyfile is specified\n"));
> > >
> > > - /* Warn if key file args are provided without key file */
> > > - if (!state[6].set && (state[7].set || state[8].set))
> > > - grub_printf_ (N_("warning: keyfile offset (-O) and size (-l) arguments "
> > > - "are ignored without keyfile (-d)\\n"));
> > >
> > >
> > >
> > > - /* Warn if hash was not set */
> > > - if (!state[0].set && !cargs.keyfile)
> > > - grub_printf_ (N_("warning: using password and hash is not set, using default %s\n"),
> > > - cargs.hash);
> > >
> > >
> > >
> > > - /* Warn if cipher was not set */
> > > - if (!state[1].set)
> > > - grub_printf_ (N_("warning: cipher not set, using default %s\n"),
> > > - GRUB_PLAINMOUNT_CIPHER);
> > >
> > >
> > >
> > > - /* Warn if key size was not set */
> > > - if (!state[4].set)
> > > - grub_printf_ (N_("warning: key size not set, using default %"PRIuGRUB_SIZE" bits\n"),
> > > - cargs.key_size * 8);
> > >
> > >
> > >
> > > - err = plainmount_configure_sectors (dev, &cargs);
> > > - if (err != GRUB_ERR_NONE)
> > > - goto error;
> > >
> > > - grub_dprintf ("plainmount",
> > > - "parameters: cipher=%s, hash=%s, key_size=%"PRIuGRUB_SIZE", keyfile=%s, "
> > >
> > >
> > > - "keyfile offset=%"PRIuGRUB_SIZE", key file size=%"PRIuGRUB_SIZE"\\n",
> > >
> > >
> > > - cargs.cipher, cargs.hash, cargs.key_size,
> > >
> > >
> > > - cargs.keyfile ? cargs.keyfile : NULL,
> > >
> > >
> > > - cargs.keyfile_offset, cargs.keyfile_size);
> > >
> > >
> > >
> > > - dev->modname = "plainmount";
> > > - dev->source_disk = cargs.disk;
> > > - grub_memcpy (dev->uuid, GRUB_PLAINMOUNT_UUID, sizeof (dev->uuid));
> >
> > I don't like this because this makes the collection of all dev uuids
> >
> > not unique if there are more than one plainmount volume mounted. I
> >
> > haven't thought about how this might cause related problems, but its a
> >
> > concern. Would it better to create a ramdom prefix and allow for say
> >
> > 256 plainmounts? Maybe use a module level static global to keep track
> >
> > of number of plain mounts.
>
> I will think about this.
>
> >
> > > - COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (GRUB_PLAINMOUNT_UUID));
> > >
> > > - /* For password or keyfile */
> > > - cargs.key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> > > - if (!cargs.key_data)
> > > - {
> > > - err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> > >
> > >
> > > - goto error;
> > >
> > >
> > > - }
> > >
> > > - /* Configure keyfile/keydisk/password */
> > > - if (cargs.keyfile)
> > > - if (grub_strchr (cargs.keyfile, '/'))
> > > - err = plainmount_configure_keyfile (dev, &cargs);
> > >
> > >
> > > - else
> > > - err = plainmount_configure_keydisk (dev, &cargs);
> > >
> > >
> > > - else /* password */
> > > - err = plainmount_configure_password (dev, &cargs);
> > > - if (err != GRUB_ERR_NONE)
> > > - goto error;
> > >
> > > - err = grub_cryptodisk_insert (dev, diskname, cargs.disk);
> > > - if (err == GRUB_ERR_NONE)
> > > - {
> > > - grub_printf_ ("disk %s mounted as crypto%"PRIuGRUB_SIZE" in plain mode.\\n",
> > >
> > >
> > > - dev->source, dev->id);
> > >
> > >
> >
> > This should be a grub_dprintf().
>
> I think some text mesage should be printed to indicate the status, because if nothing is
> printed the user will not know what happened (if we think about working in shell). LUKS
> currently prints message 'Slot 0 opened'. I will try to add fs probe checks after creating
> 'crypto0' device and reconsider what should be printed (or not) in different cases.
Does cryptsetup print anything when doing plain mount? I don't really
think it makes sense to print a success message for plainmount (not
grub_dprintf either, like in my previous suggestion). Its not really
useful. If there's an error, then an error message will get printed so
the user will know something failed. So if nothing failed, then nothing
need be printed. Printing a success message is more likely to be
confusing when the key or password is incorrect because the the device
can be opened "sucessfully", but the decrypted data is garbage.
It kinda makes sense for luks2 to print "Slot X opened" because then
you know what slot is being used to decrypt the master key. But I think
that message in luks2 could be a grub_dprintf as well.
If there was a way of being sure that the device is decrypted
correctly, then this message makes more sense. For the common case,
checking for some filesystem being recognized would tell you that the
decryption was successful. But GRUB not detecting a filesystem does not
mean the decryption failed.
IMO, this is an advanced feature and users using it won't need a
success message that's potentially misleading.
>
> > > +error:
> > >
> > > - grub_free (cargs.hash);
> > > - grub_free (cargs.cipher);
> > > - grub_free (cargs.keyfile);
> > > - grub_free (cargs.key_data);
> >
> > These frees should be done even if no error, otherwise you're going to
> >
> > have a memory leak in the successful case.
>
> Does key_data memory must be freed to? I run a test with freeing it and
> received memory error like ('Alloc bad magic') when cryptdisk.c/disk.c were
> reading disk. I looked at cryptodisk internals - it appeared cryptodisk saves
> pointers to some of these parameters, so my impression was that this memory
> is needed for cryptodisk, but may I got it wrong. I will recheck this issue.
The parameters in cargs are only needed up until the device is fully
setup, which it should be by this point (or a failure). The crypto
device should not use any memory used for cargs members.
>
> > > - if (cargs.disk)
> > > - grub_disk_close (cargs.disk);
> > > - return err;
> > >
> > > +}
> > >
> > > +static grub_extcmd_t cmd;
> > >
> > > +GRUB_MOD_INIT (plainmount)
> > >
> > > +{
> > >
> > > - cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
> > > - N_("[-h hash] [-c cipher] [-o offset] [-s size] "
> > >
> > >
> > > - "[-k key-size] [-z sector-size] [-d keyfile] "
> > >
> > >
> > > - "[-O keyfile offset] [-l keyfile-size] <SOURCE>"),
> > >
> > >
> >
> > This might be more accurate if this
> >
> > [-d keyfile] [-O keyfile offset] [-l keyfile-size]
> >
> > were prelaced by
I meant "replaced" instead of "prelaced", not sure if that was
understood.
> >
> > [-d keyfile [-O keyfile offset] [-l keyfile-size]]
>
> Do you mean the entire list of arguments must be placed in []
> brackets? Like [ ... [-d keyfile] ... [-l keyfile-size] ] ?
No, I'm just referring to those arguments. So the help string should
contain that literal string.
Glenn
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-04 22:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-30 19:40 [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode Maxim Fomin
2022-01-31 11:15 ` Milan Broz
2022-01-31 17:40 ` Maxim Fomin
2022-02-01 2:45 ` Glenn Washburn
2022-02-01 7:16 ` Milan Broz
2022-02-01 2:30 ` Glenn Washburn
2022-02-02 14:55 ` Maxim Fomin
2022-02-04 22:07 ` Glenn Washburn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.