grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Maxim Fomin
Subject: [PATCH v4 1/1] plainmount: Support plain encryption mode
Date: Mon, 11 Jul 2022 19:35:47 +0000

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

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

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

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


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


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]