Re: [PATCH v3 2/2] luks2: Fix decoding of digests and salts with escaped chars

[Glenn Washburn]

On Mon, 30 May 2022 18:01:05 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> It was reported in the #grub IRC channel on Libera that decryption of
> LUKS2 partitions fails with errors about invalid digests and/or salts.
> In all of these cases, what failed was decoding the Base64
> representation of these, where the encoded data contained invalid
> characters.
> 
> As it turns out, the root cause is that json-c, which is used by
> cryptsetup to read and write the JSON header, will escape some
> characters by prepending a backslash when writing JSON strings by
> default. Most importantly, json-c also escapes the forward slash, which
> is part of the Base64 alphabet. Because GRUB doesn't know to unescape
> such characters, decoding this string will rightfully fail.
> 
> Interestingly, this issue has until now only been reported by users of
> Ubuntu 18.04. And a bit of digging in fact reveals that cryptsetup has
> changed the logic in a054206d (Suppress useless slash escaping in json
> lib, 2018-04-20), which has been released with cryptsetup v2.0.3. Ubuntu
> 18.04 is still shipping with cryptsetup v2.0.2 though, which explains
> why this is not a more frequent issue.
> 
> Fix the issue by using our new `grub_json_unescape ()` helper function
> that handles unescaping for us.
> 
> Reported-by: Afdal
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/disk/luks2.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index bf741d70f..623607794 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -384,6 +384,24 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t 
> cargs)
>    return cryptodisk;
>  }
>  
> +static grub_err_t
> +luks2_base64_decode (const char *in, size_t inlen, grub_uint8_t *decoded, 
> idx_t *decodedlen)

We should use grub_size_t instead of size_t here and below.

> +{
> +  size_t unescaped_len;
> +  char *unescaped;

I like initializing this to NULL so that its explicit to
grub_json_unescape() that it doesn't point to a valid buffer. But not
as important if grub_json_unescape() never tries to read from it.

> +  bool successful;
> +
> +  if (grub_json_unescape (&unescaped, &unescaped_len, in, inlen) != 
> GRUB_ERR_NONE)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not unescape Base64 
> string");
> +
> +  successful = base64_decode (unescaped, unescaped_len, (char *)decoded, 
> decodedlen);

Here we call outside of GRUB land and so unescaped_len will get coerced
to size_t. Would this cause any problems? (I suspect not, but we should
do an explicit cast)

> +  grub_free (unescaped);
> +  if (!successful)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not decode Base64 
> string");
> +
> +  return GRUB_ERR_NONE;
> +}
> +
>  static grub_err_t
>  luks2_verify_key (grub_luks2_digest_t *d, grub_uint8_t *candidate_key,
>                 grub_size_t candidate_key_len)
> @@ -395,9 +413,11 @@ luks2_verify_key (grub_luks2_digest_t *d, grub_uint8_t 
> *candidate_key,
>    gcry_err_code_t gcry_ret;
>  
>    /* Decode both digest and salt */
> -  if (!base64_decode (d->digest, grub_strlen (d->digest), (char *)digest, 
> &digestlen))
> +  if (luks2_base64_decode (d->digest, grub_strlen (d->digest),

grub_strlen() returns a grub_size_t, which is getting coerced to size_t
here. I would prefer to have the luks2_base64_decode() signature
changed.

> +                        digest, &digestlen) != GRUB_ERR_NONE)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid digest");
> -  if (!base64_decode (d->salt, grub_strlen (d->salt), (char *)salt, 
> &saltlen))
> +  if (luks2_base64_decode (d->salt, grub_strlen (d->salt),
> +                        salt, &saltlen) != GRUB_ERR_NONE)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid digest salt");
>  
>    /* Configure the hash used for the digest. */
> @@ -435,8 +455,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>    gcry_err_code_t gcry_ret;
>    grub_err_t ret;
>  
> -  if (!base64_decode (k->kdf.salt, grub_strlen (k->kdf.salt),
> -                  (char *)salt, &saltlen))
> +  if (luks2_base64_decode (k->kdf.salt, grub_strlen (k->kdf.salt),
> +                        salt, &saltlen) != GRUB_ERR_NONE)
>      {
>        ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid keyslot salt");
>        goto err;

Glenn