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

[Maxim Fomin]

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

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

Best regards,
Maxim