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

[Maxim Fomin <maxim@fomin.one>]

------- Original Message -------
On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn <development@efficientek.com> wrote:
> > +/ Hashes a password into a key and stores it with cipher. /
> > +static grub_uint8_t
> > +plainmount_configure_password (grub_cryptodisk_t dev, const char *hash,
> > + grub_uint8_t *key_data, grub_size_t key_size)
>
>
> Why does the return type changed from v5? I like it was better before,
> and I'm thinking the signature should be more like hash() in
> cryptsetup, that is have password and password_size arguments, to get
> rid of the non-NULL byte assumption. Moving the password asking code
> out of this function is fine though.

I changed signature because configure_password() modifies password data sent from the caller.
It does so in a such way, that new pointer must be returned (that's what I was thinking when
changing function signature). This does not happen with the configure_keyfile() because it
does not modify the buffer. So, moving the call to setkey() into the main func (out from
configure_password() and configure_keyfile()) required changing one of the function signatures.
I will reconsider this issue to make signatures look like hash() in cryptsetup and also will
check the issue with NULL byte.

>
> > + grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
> > +
> > + /* Warn if -p option is specified with keyfile /
> > + if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
> > + grub_printf_ (N_("warning: password specified with -p option "
> > + "is ignored if keyfile is provided\n"));
> > +
> > + / Warn of -O is provided without keyfile /
> > + if (state[OPTION_KEYFILE_OFFSET].set && !state[OPTION_KEYFILE].set)
> > + grub_printf_ (N_("warning: keyfile offset option -O "
> > + "specified without keyfile option -d\n"));
> > +
> > + grub_dprintf ("plainmount", "parameters: cipher=%s, hash=%s, key_size=%"
> > + PRIuGRUB_SIZE", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
> > + cipher, hash, key_size, keyfile, keyfile_offset);
> > +
> > + err = plainmount_configure_sectors (dev, disk, sector_size);
> > + if (err != GRUB_ERR_NONE)
> > + goto exit;
> > +
> > + / Configure keyfile or password */
> > + if (keyfile != NULL)
> > + {
> > + err = plainmount_configure_keyfile (keyfile, key_data, key_size, keyfile_offset);
> > + if (err != GRUB_ERR_NONE)
> > + goto exit;
> > + err = plainmount_setkey (dev, key_data, key_size);
> > + if (err != GRUB_ERR_NONE)
> > + goto exit;
> > + }
> > + else
> > + {
> > + hashed_key_data = plainmount_configure_password (dev, hash, key_data, key_size);
>
>
> It looks like you're limiting key_data, which could be a string from
> -p, to key_size. The cryptsetup code does not appear to do this, so I
> think this does not work for passwords longer than the hash length.

In one of the old versions of the patch I removed the option which allowed to set key size
from password or keyfile. I considered it is strange to specify key size option (-s 512,
for example) and then implicitly specify different key size from password, hash or keyfile
argument. I think it was that version of the patch where you pointed on possible buffer
overflow attack because of this feature.

Also, I am confused about maximum key data and password size allowed by cryptomount.h. It
limits GRUB_CRYPTODISK_MAX_KEYLEN to 128, GRUB_CRYPTODISK_MAX_PASSPHRASE to 256 and
GRUB_CRYPTODISK_MAX_KEYFILE_SIZE to 8192. So, it looks like the passphrase (before hashing)
is limited to 256 bytes, when it is hashed - it is limited to hash size, which cannot be
larger than 128 bytes. Why then GRUB_CRYPTODISK_MAX_KEYFILE_SIZE is limited to 8192 bytes
(or bits?)? Also, if password is not hashed (-h plain) then 129 byte password becomes illegal,
because it is used directly as a key, which is limited to 128 bytes. Am I correct?

> > + if (hashed_key_data == NULL)
> > + goto exit;
> > + err = plainmount_setkey (dev, hashed_key_data, key_size);
> > + if (err != GRUB_ERR_NONE)
> > + {
> > + grub_free (hashed_key_data);
> > + goto exit;
> > + }
> > + }
>
>
> I was hoping that when moving plainmount_setkey() out of the
> plainmount_configure_*() functions that it could be only called once in
> the code, instead of twice as done here. Why can't we refactor and have
> this code here:
>
> err = plainmount_setkey (dev, key_data, key_size);
> if (err != GRUB_ERR_NONE)
> goto exit;
>
> Glenn

I will check this in the next version (see my comment above about changing data buffer
in one of configure_*() function explaining why I changed the function signature).