grub-devel
[Top][All Lists]
Advanced

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

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


From: Glenn Washburn
Subject: Re: [PATCH v6 1/1] plainmount: Support plain encryption mode
Date: Sat, 27 Aug 2022 12:05:19 -0500

On Sat, 27 Aug 2022 12:06:30 +0000
Maxim Fomin <maxim@fomin.one> wrote:

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

The password nor should keyfile does not set the keysize, implicitly or
otherwise.

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

The max keyfile size 4096 bytes, and I believe that is the cryptsetup
default compiled in maximum. Yes, an unhashed 129 byte password will be
longer than the max key length, this is okay because it should just be
truncated to keysize. Cryptsetup looks like it fails when the password
using the plain hash is of length _less_ than key size. If it is _more_
than keysize, then its fine, it just copies up to keysize bytes.

As far as keyfiles, plainmount is different than cryptomount, because
keyfile data in plainmount is used as the encryption key, but in
cryptomount its used as the password (ie the keyfile is hashed). I'm
actually not sure if this conforms with cryptsetup, can you verify
this? The documentation doesn't seem to specify.

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

Sure that makes sense, but I think having the signature more like hash()
in cryptsetup doesn't have this issue.

Glenn




reply via email to

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