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

[Glenn Washburn]

On Thu, 15 Sep 2022 05:28:40 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> On Thursday, September 15th, 2022 at 12:54 AM, Glenn Washburn 
> <development@efficientek.com> wrote:
> > 
> > On Wed, 14 Sep 2022 16:15:56 +0000
> > Maxim Fomin maxim@fomin.one wrote:
> > 
> > > ------- Original Message -------
> > > On Tuesday, August 23rd, 2022 at 6:14 AM, Glenn Washburn 
> > > development@efficientek.com wrote:
> > > 
> > > > 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.
> > > > 
> > > > > +{
> > > > > + grub_uint8_t *derived_hash, *dh;
> > > > > + char p;
> > > > > + unsigned int round, i;
> > > > > + unsigned int len, size;
> > > > > +
> > > > > + / Support none (plain) hash /
> > > > > + if (grub_strcmp (hash, "plain") == 0)
> > > > > + {
> > > > > + dev->hash = NULL;
> > > > > + return key_data;
> > > > > + }
> > > > > +
> > > > > + / Hash argument was checked at main func */
> > > > 
> > > > This should password_size + (key_size / len). I forget what the 2 above
> > > > should be from (NULL byte and something else?), but I'm not sure its
> > > > needed. The hash() function in cryptsetup allows for NULL bytes in the
> > > > password string. I think we should also, so p doesn't need to be NULL
> > > > terminated.
> > > > 
> > > > > + derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> > > > > + if (p == NULL || derived_hash == NULL)
> > > > > + {
> > > > > + grub_free (p);
> > > > > + grub_free (derived_hash);
> > > > > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > > > > + return NULL;
> > > > > + }
> > > > > + dh = derived_hash;
> > > > > +
> > > > > + /*
> > > > > + * Hash password. Adapted from cryptsetup.
> > > > > + * 
> > > > > https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> > > > > + /
> > > > > + for (round = 0, size = key_size; size; round++, dh += len, size -= 
> > > > > len)
> > > > > + {
> > > > > + for (i = 0; i < round; i++)
> > > > > + p[i] = 'A';
> > > > > +
> > > > > + grub_strcpy (p + i, (char) key_data);
> > > > 
> > > > This assumes that there are no NULL bytes in key_data.
> > > > 
> > > > > +
> > > > > + if (len > size)
> > > > > + len = size;
> > > > > +
> > > > > + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> > > > 
> > > > This also has a non-NULL byte assumption.
> > > 
> > > Regarding NULL byte assumption. You mention it in the context of 
> > > supplying password
> > > from grub terminal via interactive console or via '-p' option. How user 
> > > is supposed
> > > to input NULL character? I couldn't find how to do it. If supplying NULL 
> > > byte is not
> > > possible, then supporting this feature is useless. In cryptsetup 
> > > 'password size' is
> > 
> > 
> > Yes, I don't think there is currently a way to add NULL bytes into the
> > password string. This could change in the future (eg. patch to allow
> > \xXX or \oOOO character escapes). Making this NULL byte agnostic will
> > future proof this code. Do you think it will take much effort to make
> > the change? If you are really against this, I can accept that, but in
> > that case there should be at a minimum a comment above the function
> > stating that it does not handle passwords containing NULL bytes.
> 
> I am not against this change, actually I already implemented it. I just 
> wanted to clatify
> this issue - I was not sure that currently there is no way to type NULL byte. 
> I will make
> the code NULL agnostic and write a big comment explaining current situation.

Thanks!

> > 
> > > not used to support NULL byte in password, it is used for different 
> > > purpose: the key
> > > size (-s in terms of plainmount syntax) parameter is optional, so 
> > > 'password size'
> > > parameter keeps password size. In plainmount key size is mandatory and 
> > > should match
> > > the actual password size, so 'password size' parameter is not needed.
> > 
> > 
> > No, the 'password size' is not used to determine the 'key size' when
> > key size is not given, nor is it to support NULL bytes in the password
> > string. It is to support passwords that are a different size from the
> > key size. What I was saying is that this allows the use of NULL bytes
> > in the password hashing code (I've not tried sending a password with
> > NULL bytes from the command line to see if there is a way for a user
> > to put NULL bytes in the password string).
> > 
> > Why should key size match password size? Why shouldn't I be able to
> > have a password not equal to key size? The hash function should hash
> > the password of any size to a string of bytes that is key size long.
> > And that is what the cryptsetup code does.
> > 
> > Glenn
> 
> I was incorrect when saying that key size must match password size. Current 
> plainmount
> implementaion truncates hashed password if key size < len(hash(password)) and 
> zero-padds
> hash if key size > len(hash(password)). This happens if key size != 
> len(hash()). This
> behavior matches cryptsetup. However, I believe most users choose key size 
> length that
> matches len(hash_algorithm), so those 'corner cases' do not arise in practice.

Yes, but this isn't really what I was speaking to. Even when the hash
size is equal to the key size, the password can be longer or shorter
than the key size. I think its probably pretty common that the user
chooses a password that does not equal key size. And in this case all
bits of the password should be fed to the hash to create the key (which
is just a hash of the password bits).

Glenn