Re: [PATCH v2] luks2: Fix decoding of digests and salts with escaped chars

[Glenn Washburn]

On Mon, 4 Oct 2021 13:21:49 +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. Most
> importantly, this also includes the forward slash, which is part of the
> Base64 alphabet and which may optionally be escaped. Because GRUB
> doesn't know to unescape such characters, decoding this string will
> rightfully fail.
> 
> Fix the issue by stripping the escape character for forward slashes.
> There is no need to do so for any other escaped character given that
> valid Base64 does not contain anything else.

It certainly could though with an alternate implementation and still be
valid as defined by the specification. See below.

> 
> Reported-by: Afdal
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> 
> Changes compared to v1:
> 
>     - `luks2_base64_decode` now takes `decoded` as `grub_uint8_t *`
>       instead of as `char *`
> 
>     - Adapted the comment explaining why we only unescape forward
>       slashes, based on Glenn's feedback.
> 
>     - Fixed error handling for the new function. `base64_decode` returns
>       a bool, where `true` indicates success. In v1, error handling only
>       worked by chance due to accidental double negation. We now
>       properly handle errors returned by `base64_decode` and return
>       `grub_err_t` errors.
> 
> Patrick
> 
>  grub-core/disk/luks2.c | 48 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 371a53b83..2a70c66dc 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -383,6 +383,44 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, 
> int check_boot)
>    return cryptodisk;
>  }
>  
> +static grub_err_t
> +luks2_base64_decode (const char *in, size_t inlen, grub_uint8_t *decoded, 
> size_t *decodedlen)
> +{
> +  char *unescaped;
> +  bool successful;
> +  size_t i, j;
> +
> +  unescaped = grub_malloc (inlen);
> +  if (!unescaped)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +
> +  grub_memmove (unescaped, in, inlen);

This seems completely superfluous. Why is this grub_memmove needed when
we can use "in" below?

> +
> +  /*
> +   * Characters in JSON strings may be escaped, either via their 
> six-character
> +   * "\uXXXX" representation or (at least for a subset of characters) via a
> +   * single backslash. In context of the Base64-encoded string we expect 
> here,
> +   * the only character that gets escaped by cryptsetup is the forward slash.
> +   * So while incomplete, we only strip away the escape character if we see
> +   * '\/' in the input.

This still assumes the base64 encoded string is created by the json lib
that cryptsetup currently uses. What if it changes its implementation
or they start using a different library that has a different
implementation where '/' is escaped as '\u002f' instead of '\/'? If we
aren't going to do anything about the unicode escape case, I think at
least we should document a TODO to do something about it at a future
date. But I don't think it would be much more code to do the unicode
unescape.

> +   *
> +   * See https://datatracker.ietf.org/doc/html/rfc8259#section-7 for more
> +   * information on escaping in JSON.
> +   */
> +  for (i = j = 0; i < inlen; i++) {
> +    if (i + 1 < inlen && unescaped[i] == '\\' && unescaped[i + 1] == '/')
> +      continue;
> +    unescaped[j++] = unescaped[i];
> +  }

We're writing all of unescaped used by base64_decode, why not use "in"
as the source as opposed to unescaped.

> +
> +  successful = base64_decode (unescaped, j, (char *)decoded, decodedlen);
> +  grub_free (unescaped);
> +  if (!successful)
> +    return GRUB_ERR_BAD_ARGUMENT;

This should be a grub_error with a message indicating that there was a
base64 decoding error.

> +
> +  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)
> @@ -394,9 +432,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),
> +                        digest, &digestlen) != GRUB_ERR_NONE)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid digest");

It would be nice if this grub_error were preceeded by a
grub_error_push, so that the user can get context, as opposed to the
error message getting overwritten.  Here we know the issue was an
invalid digest, but we've thrown away the message indicating that the
digest was invalid because of the base64 decoding error.

This also leads me to grub_error_push should have the same function
signature as grub_error to amek it easier to use. But that's a
different patch series.

> -  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");

ditto

>  
>    /* Configure the hash used for the digest. */
> @@ -434,8 +474,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");

ditto

>        goto err;

Adding grub_error_push should be a different patch because there's a
lot more than just these three. But I've noticed this before and want
to put it on the radar.

Glenn