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

[Glenn Washburn]

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