Re: [PATCH v2 0/1] plainmount: Support plain encryption mode.

[Glenn Washburn]

On Tue, 10 May 2022 19:16:06 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> On Thursday, May 5th, 2022 at 6:09, Glenn Washburn 
> <development@efficientek.com> wrote:
> 
> > Hi Maxim,
> >
> > I just want to reiterate Daniel, for v3, please use --threaded when
> > using format-patch and send-email. It'll help reviewers see all the
> > emails in the patch series together.
> 
> It is complicated for me because I write from email service which a) does not 
> have simple
> SMTP connection and b) is blocked in my country (I access it through VPN). I 
> think I need to
> change my email provider for git commit author to send patches (because grub 
> does not accept
> patches when commit author email differs from email sender, right?). I will 
> try to fix these
> problems in the next patch version.

Another option is to create a single patch, with no cover letter. You
can add extra information that would have gone in the cover letter in
the single patch after the first line containing only '---'. Of course,
this won't work with a multiple patch series, but this one should work.

> > On Sat, 02 Apr 2022 20:02:17 +0000
> > Maxim Fomin maxim@fomin.one wrote:
> > > +Setup access to encrypted in plain mode device. Password can be 
> > > specified in
> > > +two ways: as a secret passphrase or as a byte array from keyfile or 
> > > device
> > > +block with the option @option{-d}. Passphrase can be provided 
> > > interactively
> > > +from console or with option @option{-p}. Keyfile/device block can be a 
> > > regular
> > > +file or some partition/disk. The length of the key from keyfile/device 
> > > block
> >
> >
> > I think instead of "device block" you mean "block device" here. I think
> > the above should be rewritten as:
> >
> > Setup access to encrypted in plain mode device. Password can be
> > specified in two ways: as a secret passphrase or as a byte array from
> > a keyfile with the option @option{-d}. Passphrase can be provided
> > interactively from console or with option @option{-p}. The keyfile
> > can be either a path to a regular file or a block device. The length
> > of the key from keyfile
> 
> When I wrote 'device block' I meant to say 'piece' of 'block device' because
> writing just 'block device' could be wrongly intepreted that the whole 
> partition
> acts as passphrase which is strange. I think specifying 'segment' in the 
> sentence
> 'path to a regular file or a block device segment' will be more clear.

You could mention using blocklist syntax to use a specified range of
sectors. I think you should just get rid of the support for sending a
disk device as an argument. This can be supported just as well with the
blocklist syntax.

> > > +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"), 0, ARG_TYPE_STRING},
> > > + {"disk-size", 'b', 0, N_("Device size"), 0, ARG_TYPE_STRING},
> >
> >
> > I'm failing to see why this option is needed. What's the reasoning
> > behind wanting this? Would it be sufficient to create a loopback device
> > of the required size using the blocklist syntax?
> 
> Probably yes, I didn't think about loopback device. This option allows to
> set disk size explicitly when for some reason disk firmware reports wrong
> size to grub. If such feature can be provided with loopback device, than it
> can be removed from here. I will check this in the next version.
> 
> > > + {"key-size", 's', 0, N_("Key size (in bits)"), 0, ARG_TYPE_INT},
> > > + {"sector-size", 'S', 0, N_("Device sector size"), 0, ARG_TYPE_INT},
> > > + {"password", 'p', 0, N_("Password (key)"), 0, ARG_TYPE_STRING},
> > > + {"keyfile", 'd', 0, N_("Keyfile/disk path"), 0, ARG_TYPE_STRING},
> > > + {"keyfile-offset", 'O', 0, N_("Keyfile offset."), 0, ARG_TYPE_STRING},
> > > + {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;
> > > + grub_disk_t disk;
> > > +};
> > > +typedef struct grub_plainmount_args *grub_plainmount_args_t;
> >
> >
> > The reason that I created the cargs code in cryptomount was to not have
> > to change the backend function interfaces when new features were added.
> > I don't think this quite applies here, since there are not backends. I
> > don't really like that this struct is being used instead of decent
> > function signatures. I'm not adamantly opposed to using this struct,
> > but I would prefer that it not be used and only pass in needed data to
> > each function.
> 
> Yes, initially I wrote the module as a backend to cryptomount.
> I will fix it in the next version.
> 
> > > + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > + N_("cannot set key from password. "
> > > + "Check keysize/hash/cipher options."));
> > > + }
> > > + return GRUB_ERR_NONE;
> > > +}
> > > +
> > > +
> > > +/* Parse disk size suffix */
> > > +static grub_size_t plainmount_parse_suffix (char arg)
> > > +{
> > > + const char p = NULL;
> > > + grub_errno = GRUB_ERR_NONE;
> > > + grub_size_t val = grub_strtoull (arg, &p, 0);
> > > + switch (p)
> > > + {
> > > + case 'K':
> > > + case 'k':
> > > + val = val * 1024;
> > > + break;
> > > + case 'M':
> > > + case 'm':
> > > + val = val * 10241024;
> > > + break;
> > > + case 'G':
> > > + case 'g':
> > > + val = val * 102410241024;
> > > + break;
> > > + case '\0':
> > > + break;
> > > + default:
> > > + val = (grub_size_t) -1;
> > > + }
> >
> >
> > If there is are non-NULL characters after p[0], they will be silently
> > ignored. Its not the most terrible thing, and it would allow for 'KB'
> > and other cases where the user didn't read the documentation closely. On
> > the otherhand, it doesn't sit well with me that "100GiB" is parsed fine
> > and will not error nor do what the user expects.
> 
> Yes, if user types 100GB assuming base 10, it will not match his/her 
> expectation.

Glad to see you understood my point despite my errors in making it. I
think there should be better handing of undesired user-input here.

> > > + return val;
> > > +}
> > > +
> > > +/* Configure cryptodisk uuid */
> > > +static void plainmount_set_uuid (grub_cryptodisk_t dev)
> > > +{
> > > + grub_size_t pos = 0;
> > > + static const char *uuid = "00000000-0000-0000-0000-000000000000";
> > > + grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%32lu", dev->id+1);
> >
> >
> > So this generates a UUID based on the cryptodisk device id. This means
> > it will be potentially different on different boots where there are
> > multiple cryptodisk devices and they are opened in differing orders.
> > That doesn't seem like a great idea. On the otherhand, I'm not coming
> > up with a better solution right now. Any ideas?
> >
> > Also what do you think about creating a random 16 byte prefix to use
> > for the UUID for plain mounts (this you would generate once and put in
> > the source). It seems more likely (although still unlikely) that a user
> > might created a LUKS volume and set a UUID in the range used above
> > (since its not very random).
> 
> Yes, current UUID is not random and can clash with those UUIDs which are
> created by user manually, and UUID can change during boot. On the other hand,
> creating random UUID each boot defeats the purpose of using such UUID in
> grub.cfg. Besides, does grub have random generator? Perhaps drop this idea
> altogether and leave blank UUID?

I didn't explain well enough. I'm not suggesting that the UUID will be
random on each boot. I'm saying to use a random UUID prefix defined at
compile time. But it perhaps doesn't even need to be randomly
generated, for instance using the non-randomly generated prefix
'506c6169-6e55-5549-44'. The rest of the UUID would be zero-filled
except for the last digits being from the GRUB device id.

As for potentially just having the UUID be blank, it doesn't sit well
with me. First of all, it means that specifying the device as
(cryptouuid/xxxx...) won't work, and I think that's a nice feature to
have. Also, I think having the UUID space be unique sounds like a good
idea, even if it doesn't matter right now and even if the UUID is not
preserved across boots. Unfortunately GRUB doesn't have a device
uniqueness property on all/any targets, otherwise, I'd say using some
id of the underlying device. For EFI platforms, which are the majority
(in my uneducated estimation), we could use the EFI device path as a
source of uniqueness. Anyway, just some ideas that are not really in
scope for this patch.

Another idea just came to me, let the user specify a UUID via
the "-U" or "--set-uuid" parameters. There should be a default UUID
generated if this option is not given. But this would allow a UUID that
is stable across reboots.

> >
> > > + while (dev->uuid[pos++] == ' ');
> >
> >
> > I'm failing to see where dev->uuid gets filled with spaces. It seems to
> >
> > me that it should be NULL filled from when dev was created using
> > grub_zalloc().
> 
> It is the result of grub_snprintf when the size of the 'buf' is larger than
> specified number (in particular, when size is 32 and number of digits is less
> than 32).

Ah right. Referencing above, I was saying that the line two above the
while loop could be:

 static const char *uuid = "506c6169-6e55-5549-4400-000000000000";

> > > + grub_memcpy (dev->uuid, uuid, pos-1);
> > > + COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (uuid));
> > > +}
> > > +
> > > +
> > > +/* Configure cryptodevice sector size (-S option) /
> > > +static grub_err_t
> > > +plainmount_configure_sectors (grub_cryptodisk_t dev, 
> > > grub_plainmount_args_t cargs)
> > > +{
> > > + grub_disk_addr_t total_sectors;
> > > +
> > > + / cryptsetup allows only 512/1024/2048/4096 byte sectors */
> > > + 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 -S %"PRIuGRUB_SIZE
> > > + ", only 512/1024/2048/4096 are allowed"),
> > > + cargs->sector_size);
> >
> >
> > This check is true for LUKS2, but is it true here? Shouldn't plainmount
> > support arbitrary power of 2 sector sizes? It may be the case that
> > cryptsetup only supports these sector sizes, but do we care? Someone
> > could have a plain mount not created by cryptsetup. I do think we
> > should check for power of 2.
> 
> Technically you are correct, although I would bet cryptsetup is 99%
> implementation which was used for initial encryption. Bitlocker does not
> support plain mode, so it leaves only BSD tools (if they support plain mode
> with more types of disk sector than cryptsetup).

Ok fine, but still why have this check enforced? Just for sanity
checking so the user doesn't accidentally choose a wrong one? If we
limit the check to powers of two, I think that's enough sanity checking
to prevent a typo by the user. I don't think a user would enter 16384
when they really shouldn't put 1024.

> > > + if (!hash)
> > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> > > + N_("couldn't load %s hash (perhaps a typo?)"),
> > > + cargs->hash);
> > > +
> > > + if (hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
> > > + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > + N_("hash length %"PRIuGRUB_SIZE
> > > + " exceeds maximum %d bits"),
> > > + hash->mdlen, GRUB_CRYPTODISK_MAX_KEYLEN * 8);
> >
> >
> > s/8/BITS_PER_BYTE/
> >
> > Its odd here that the first number is hash length in bytes and the next
> > number, max hash size, is in bits. The first number should be in bits
> > and have "bits" after it.
> 
> I followed cryptsetup which specified key size in bits and key file size in
> bytes, I will change this in the next version.

I don't think that's what I was saying. "key size in bits and key file
size in bytes" is fine. I'm talking about the error message which, in
the above example, talks about hash length first in _bytes_, then its
maximum _bits_.

> > > + 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);
> > > + return plainmount_setkey (dev, derived_hash, cargs->key_size);
> > > +}
> > > +
> > > +
> > > +/* 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_file_open (cargs->keyfile, 
> > > GRUB_FILE_TYPE_NONE);
> > > + if (!keyfile)
> > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile 
> > > %s"),
> > > + cargs->keyfile);
> > > +
> > > + if (grub_file_seek (keyfile, cargs->keyfile_offset) == (grub_off_t)-1)
> > > + return grub_error (GRUB_ERR_FILE_READ_ERROR,
> > > + N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
> > > + cargs->keyfile_offset);
> > > +
> > > + if (cargs->key_size > (keyfile->size - cargs->keyfile_offset))
> > > + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > + N_("Specified key size (%"PRIuGRUB_SIZE") is too small"
> > > + " for keyfile size (%"PRIuGRUB_SIZE") and offset (%"
> > > + PRIuGRUB_SIZE")"),
> > > + cargs->key_size, keyfile->size,
> > > + cargs->keyfile_offset);
> > > + else
> > > + cargs->key_size = keyfile->size - cargs->keyfile_offset;
> >
> >
> > Why is cargs->key_size clobbered here? Shouldn't cargs->key_size only
> >
> > get set if its not already set? If I set -s 512 -k (dev)/some/file -O
> > 16, where (dev)/some/file is 4096 bytes, does this mean that
> > cargs->key_size is getting set to (4096 - 16)?
> >
> >
> > Its probably prudent to have cargs->key_size be set to
> >
> > grub_min(cargs->key_size, GRUB_CRYPTODISK_MAX_PASSPHRASE).
> 
> This code allows to omit key size when keyfile and offset is specified -
> in that case key size is deduced to be file size - offset, if it is less
> than cryptomount limit. In your example - yes, it will be 4096-16.
> 
> On the other hand, I have forgotten that in new patch version the '-s' option
> is required, so this feature is useless. I will remove this issue in the next
> version.

I think its better not to deduce key size. I'm not opposed to a default
key size (which should be 256 or 512).

> > > +
> > > + if (grub_file_read (keyfile, cargs->key_data, cargs->key_size) !=
> > > + (grub_ssize_t) cargs->key_size)
> >
> >
> > Since cargs->key_data is a buffer of size
> >
> > GRUB_CRYPTODISK_MAX_PASSPHRASE (128 bytes), but in the above example
> > cargs->key_size is 4080, I'm overflowing the buffer. Or am I missing
> >
> > something here?
> 
> Good catch! In previous version there was 'MAX_PASSPHRASE' condition, but it 
> was
> refactored to grub_cmd_plainmount() leaving this function unprotected. This 
> feature
> will be definitely removed.
> 
> > > +
> > > + /* Parse cmdline arguments */
> > > + if (state[2].set)
> > > + {
> > > + cargs.offset = plainmount_parse_suffix (state[2].arg);
> > > + if (cargs.offset == (grub_size_t)-1)
> > > + {
> > > + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > + N_("unrecognized offset suffix"));
> > > + goto exit;
> > > + }
> > > + }
> > > + if (state[3].set)
> > > + {
> > > + cargs.size = grub_strtoull (state[3].arg, &p, 0);
> > > + cargs.size = plainmount_parse_suffix (state[3].arg);
> > > + if (cargs.size == (grub_size_t)-1)
> > > + {
> > > + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > + N_("unrecognized disk size suffix"));
> > > + goto exit;
> > > + }
> > > + }
> > > + grub_errno = GRUB_ERR_NONE;
> > > + cargs.key_size = grub_strtoull (state[4].arg, &p, 0) / 8;
> >
> >
> > s/8/BITS_PER_BYTE/
> >
> > And define BITS_PER_BYTE at the top of the file. I considered using
> > GRUB_CHAR_BITS, but I don't think that's semantically correct.
> >
> > Also calling grub_strtoull() on state[4].arg when state[4].set == 0,
> > seems like not a good idea.
> 
> There is a check above that state[4].arg is set. Perhaps 'state[4].set'
> can be set, but 'state[4].arg' can be NULL - i didn't looked at grub
> internals. It appears that grub does not allow to set empty option like
> '-s -some_other_option', so if the state is set, than arg should point to
> at least one byte. I agree that for safety reasons this can be checked.

I missed that first check of state[4].set. I think its more readable to
separate out that check and process state[4].arg right after checking
state[4].set.

Also, currently, the if right after the grub_strtoull() has a
redundant check for state[4].set. I'm not sure we need a safety check
for state[4].arg==NULL, it doesn't seem to be common in other commands,
but I think its a good idea.

Also, something else I see that could be improved is to add an enum to
map states indexes to names. An example of this is as is done in
grub-core/term/serial.c. The cryptomount command does not do this, but
it should.

> > This was a fair amount to review, hopefully I didn't miss anything.
> >
> > Glenn
> 
> Thanks for the review!

Also, I was wrong in my previous review when I said you don't need "\n"
in grub_dprintf() calls. I was confusing that with grub_error() calls.

Glenn