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

[Glenn Washburn]

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.

> 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.
 
> Note that keyfile method of providing key material supports any character, 
> including
> NULL byte, and its current implementation does not depend on strcpy() 
> function.

Yep, this is good.

Glenn