Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.

[Maxim Fomin]

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