Re: [PATCH v4 1/1] plainmount: Support plain encryption mode

[Glenn Washburn]

On Mon, 11 Jul 2022 19:35:47 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> On Sunday, July 10th, 2022 at 9:07 PM, Glenn Washburn 
> <development@efficientek.com> wrote:
> 
> > > +
> > > +plainmount hd0,gpt1 -o 1048576
> > > +
> > > +
> > > +both create virtual devices with 1MiB offset on top of the specified 
> > > partition. The
> > > +option @option{-o} is useful to specify offset which is not aligned to 
> > > 512 byte
> >
> >
> > I just noticed that the code takes this parameter and turns it into a
> > number of sectors that the data is offset from the start of the device.
> > Thus internally this gets rounded down to be sector aligned anyway. In
> > fact, its aligned to the size of the sector specified with the -S
> > option. So we should get rid of the -o entirely. In fact, -o will not
> > work if it is not a multiple of the sector size specified with -S.
> 
> Option -o is taken as a byte number, not as a sector number, it is not 
> multiplied
> by 512 or by sector size. However later, indeed it is truncated to a sector 
> size,
> loosing its meaning. I agree with the rest of the comment, this option is 
> irrelevant
> and should be removed.

I never did say above that the -o value gets multiplied, it gets
aligned. Anyway, I think we agree here.

> 
> >
> > > +sector size. Note: current cryptsetup implementation of plain mode and 
> > > LUKS v1 restricts
> > > +alignment to 512 byte sector size. Specifying arbitrary byte alignment 
> > > is useful only to
> > > +open LUKS v2 volume if master key is known or to open the volume 
> > > encrypted by other
> > > +cryptographic implementation. Note: in LUKS revealing master key is not 
> > > recommended
> > > +because it allows to open encrypted device directly bypassing the header 
> > > and LUKS
> > > +security features.
> >
> >
> > These "Notes" should probably be reorganized as @footnote{Put note
> > inside of this.} You have two notes here, so perhaps two foot notes,
> > although it looks like here you have a nested note. I'm not sure if you
> > can have nested footnotes.
> 
> I can change them to be just a single note.
> 
> > > +
> > > +Password can be specified in two ways: as a password data from a keyfile 
> > > or as a secret
> > > +passphrase. The keyfile parameter @option{-d} specifies path to the 
> > > keyfile containing
> > > +password data and has higher priority than the other password method. 
> > > Specified password
> > > +data in this mode is not hashed. The option @option{-O} specifies offset 
> > > in terms of bytes
> >
> >
> > Hmm its not hashed? I thought a keyfile that contains 4 characters
> > "pass" would be the same as entering "pass" as the secret passphrase.
> > If the keyfile is not hashed, then the derived master key is different.
> > Are you Am I mistaken or missing something?
> 
> I have taken this behavior from cryptsetup. Having binary key file data 
> directly allows
> to have arbitrary password (not necessarily typable with a keyboard). Such 
> feature is not
> very popular, but it exists. This feature is connected with use cases where 
> user types
> password in non-English keyboard (possibly, a letter which is not easily 
> encoded in UTF,
> or which encoding depends on keyboard layout), which cannot be typed at 
> another computer.

Ok, yes, I was mistaken. Perhaps we should say after "not hashed", ",
and and will be used as the cipher key."

> 
> > > +of the password data from the start of the keyfile. This option has no 
> > > effect without the
> > > +option @option{-d}. Offset of password data in the keyfile can be 
> > > specified directly with
> > > +option @option{-d} and GRUB blocklist syntax (for example: '-d 
> > > (hd0,gpt1)2048+').
> >
> >
> > Should this be -O instead of -d?
> 
> Why? The last part of quoted text says that alternative to option -O is the 
> blocklist syntax
> which is part of the -d option.

Ok, I got confused by the wording. I think it should use "using" instead
of "and", eg. "Offset of password data in the keyfile can be specified
directly with option @option{-d} using GRUB blocklist syntax"

> 
> > > +configured when creating the encrypted volume. Attempting to decrypt 
> > > such volume with
> > > +different sector size than it was created with will not result in an 
> > > error, but will
> > > +decrypt to random bytes and thus prevent accessing the volume.
> >
> >
> > Actually, some sectors will be decrypted correctly. So perhaps we
> > should say "but not all sectors will be properly decrypted, generally
> > causing the volume to be inaccessible"
> 
> I would not complicate explanation. The difference between decrypted in such 
> 'strange'
> scenario FS and normal FS is so large, that no fs will work. For example, I 
> have a BTRFS
> on top of -S 4096 encrypted partition. Once on live system (not GRUB) I 
> forgot to type
> -S 4096 and received internal BTRFS error in dmesg saying something about bad 
> internal
> metadata state and then general mount error. So yes, BTRFS can recognize that 
> this is a
> BTRFS partition, but will refuse to mount. In GRUB mode such device (wrong 
> sector size)
> is not recognized too. I think what can be added here is at most a line 
> saying that FS
> may ocassionally detect underlying FS, but will refuse to mount.

Your suggestion works for me.

> 
> > > +
> > > +Successfully decrypted disks are named as (cryptoX) and have linear 
> > > numeration
> > > +with other decrypted by cryptodisk devices. If the encrypted disk hosts 
> > > some higher
> > > +level abstraction (like LVM2 or MDRAID) it will be created under a 
> > > separate device
> > > +namespace in addition to the cryptodisk namespace.
> >
> >
> > This is standard cryptomount behavior. I think it makes more sense to
> > have some version of this paragraph in cryptomount and make a note here
> > that it behaves like cryptomount.
> 
> Do you mean to add this information in cryptomount doc as a part of this 
> patch?

I hadn't thought much about it, but I think its fine to add to this
patch.

> 
> > > +#include <grub/cryptodisk.h>
> > > +#include <grub/dl.h>
> > > +#include <grub/err.h>
> > > +#include <grub/extcmd.h>
> > > +#include <grub/partition.h>
> > > +#include <grub/file.h>
> > > +
> > > +
> > > +GRUB_MOD_LICENSE ("GPLv3+");
> > > +char PLAINMOUNT_DEFAULT_UUID[] = "109fea84-a6b7-34a8-4bd1-1c506305a400";
> > > +#define BITS_PER_BYTE 8
> > > +#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
> >
> >
> > Minor nit, I'd rather have these defines before the definition of
> > PLAINMOUNT_DEFAULT_UUID.
> 
> The comment later says about making it a #define instead of char[]. I made it
> char[] because it was more convenient for previous version of the code which
> is not true in current version. Yes, I can change that.
> 
> 
> >
> > > +{
> > > + grub_size_t pos = 0;
> > > +
> > > + /* Size of user_uuid is checked in main func /
> > > + if (user_uuid)
> > > + grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
> > > + else
> > > + {
> > > + /
> > > + * Set default UUID. Last digits start from 1 and are incremented for
> > > + * each new plainmount device.
> > > + * snprintf() sets uuid to ' ...x' where x starts from 1.
> > > + */
> > > + grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%36lx", dev->id+1);
> > > + while (dev->uuid[pos++] == ' ');
> > > + grub_memcpy (dev->uuid, PLAINMOUNT_DEFAULT_UUID, pos-1);
> >
> >
> > I'm wondering if there's a good reason to have PLAINMOUNT_DEFAULT_UUID
> > not be a #define, which I think makes more sense and would be expected
> > if just reading this part of the code.
> 
> OK.
> 
> > > + if (dev->total_sectors == 0)
> > > + return grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot set specified sector 
> > > size on disk %s"),
> > > + disk->name);
> > > +
> > > + /* Configure device offset */
> > > + dev->offset_sectors = grub_divmod64 (offset, sector_size, NULL);
> >
> >
> > Something I didn't catch before. Since there is only offset_sectors,
> > there can be no non-sector aligned offsets. Thus it really doesn't make
> > sense to have a '-o' option, blocklists will do everything that is
> > needed, and more actually. If you have a plainmount volume with sector
> > size 4096 at offset 2048 in (hd0), you can not access it using "-o
> > 2048". That will cause grub_divmod64() above to return 0 (2048/4096).
> > However creating a loopback device using "(hd0)+4" will work. This is
> > partly a limitation of the current cryptodisk code. Perhaps there
> > should be a dev->offset_partial that is the number of bytes to add to
> >
> > the offset_sectors to get the byte offset. That would be a different
> > patch series though. And I'm not convinced that its very useful (if you
> > have a convincing use-case I'm open to it).
> 
> I also think that -o option is useless in current conditions.
> 
> > > +
> > > + if (len > size)
> > > + len = size;
> > > +
> > > + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> > > + }
> > > + grub_free (p);
> > > + return plainmount_setkey (dev, derived_hash, key_size);
> > > +}
> > > +
> > > +
> > > +/* Read key material from keyfile */
> > > +static grub_err_t
> > > +plainmount_configure_keyfile (grub_cryptodisk_t dev, char *keyfile, 
> > > grub_uint8_t *key_data,
> >
> >
> > I don't think key_data needs to be passed here. Its only used to put
> > data read from the key file, but never used outside of this function.
> > My guess is that you're wanting to avoid allocating a buffer of
> > GRUB_CRYPTODISK_MAX_PASSPHRASE twice. But its strange to have a
> > function that takes a buffer that it unconditionally overwrites and
> > where that buffer is not used after the function.
> >
> > Thinking about it more, plainmount_configure_password and
> > plainmount_configure_keyfile should probably be refactored. I'm
> > thinking that neither function should call plainmount_setkey(), but
> > should have key_data as an out parameter and have plainmount_setkey()
> > called from grub_cmd_plainmount(). I'm also open to something other
> > than my above suggestion too.
> 
> I am OK with moving a call to setkey() outside of these two functions.

Glenn