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

[Maxim Fomin]

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

Note that keyfile method of providing key material supports any character, 
including
NULL byte, and its current implementation does not depend on strcpy() function.