Re: [PATCH v6 1/1] plainmount: Support plain encryption mode
--- Original Message ---
On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn
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).
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v6 1/1] plainmount: Support plain encryption mode
On Sat, 27 Aug 2022 12:06:30 +
Maxim Fomin wrote:
> --- Original Message ---
> On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn
> 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
Re: [PATCH] Add a link to environment variables inside docs
> Andrea, I allowed myself to add "Signed-off-by: Andrea G. Monaco > " to the commit message on your > behalf. I hope it is not a problem. Thanks! Andrea Monaco ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v6 1/1] plainmount: Support plain encryption mode
--- Original Message --- On Saturday, August 27th, 2022 at 5:05 PM, Glenn Washburn wrote: > > > On Sat, 27 Aug 2022 12:06:30 + > Maxim Fomin ma...@fomin.one wrote: > > > 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. > Yes, it specified in man cryptsetup in this section: NOTES Passphrase processing for PLAIN mode Note that no iterated hashing or salting is done in plain mode. If hashing is done, it is a single direct hash. This means that low-entropy passphrases are easy to attack in plain mode. From a terminal: The passphrase is read until the first newline, i.e. '\n'. The input without the newline character is processed with the default hash or the hash specified with --hash. The hash result will be truncated to the key size of the used cipher, or the size specified with -s. From stdin: Reading will continue until a newline (or until the maximum input size is reached), with the trailing newline stripped. The maximum input size is defined by the same compiled-in default as for the maximum key file size and can be overwritten using --keyfile-size option. The data read will be hashed with the default hash or the hash specified with --hash. The hash result will be truncated to the key size of the used cipher, or the size specified with -s. Note that if --key-file=- is used for reading the key from stdin, trailing newlines are not stripped from the input. If "plain" is used as argument to --hash, the input data will not be hashed. Instead, it will be zero padded (if shorter than the key size) or truncated (if longer than the key size) and used directly as the binary key. This is useful for directly specifying a binary key. No warning will be given if the amount of data read from stdin is less than the key size. From a key file: It will be truncated to the key size of the used cipher or the size given by -s and directly used as a binary key. WARNING: The --hash argument is being ignored. The --hash option is usable only for stdin input in plain mode. If the key file is shorter than the key, cryptsetup will quit with an error. The maximum input size is defined by the same compiled-in default as for the maximum key file size and can be overwritten using --keyfile-size option. Btw, it says also about keydata being truncated. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
