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

[Maxim Fomin]

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

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

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

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

>
> > + 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).

> > + 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).

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

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

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

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

Thanks for the review!

Maxim Fomin