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

[Glenn Washburn]

On Tue, 28 Sep 2021 17:13:10 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> CC-ing Glenn...

Thanks, I missed this

> On Wed, Aug 11, 2021 at 08:55:32PM +0200, Patrick Steinhardt wrote:
> > It was reported in the #grub IRC channel that decryption of LUKS2
> 
> Which IRC network?
> 
> > 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
> 
> Wait, is not it jsmn lib bug? If it is should not we take updated
> version of jsmn? Or if it is not fixed there propose jsmn upstream
> patch?

I had this same thought. It looks like its not a bug in upstream, but
expected behaviour and a known and intentional limitation[1]. Based on
a cursory look, it seems that a goal of jsmn is to do no memory copying
(I suppose to be aimed at memory constrained environments). So the
parsed json strings returned are basically pointers into the original
json string passed to the parser.

There has been some work to add unescaping[2], but nothing that has
been accepted as far as I can tell.

> > 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.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > Reported-by: Afdal
> 
> Reported-by should go before SOB. And it would be nice if you could
> get Afdal's email address...
> 
> > ---
> >  grub-core/disk/luks2.c | 34 ++++++++++++++++++++++++++++++----
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 371a53b83..7e596f90d 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -383,6 +383,32 @@ luks2_scan (grub_disk_t disk, const char
> > *check_uuid, int check_boot) return cryptodisk;
> >  }
> >
> > +static int luks2_base64_decode (const char *in, size_t inlen, char
> > *decoded, size_t *decodedlen)
> 
> static int
> luks2_base64_decode (...
> 
> > +{
> > +  char *unescaped = NULL;
> 
> I think you can drop this initialization.
> 
> > +  size_t i, j;
> > +  int err;
> > +
> > +  unescaped = grub_malloc (inlen);
> 
> The grub_malloc() may return NULL. Please do not ignore that...
> 
> > +  grub_memmove (unescaped, in, inlen);
> > +
> > +  /*
> > +   * JSON strings can escape some characters. Given that we expect
> > +   * Base64-encoded input here, the only character from the Base64
> > alphabet
> > +   * which may optionally be escaped is the forward slash. We thus
> > strip away
> > +   * the escape character if we see '\/' in the input.

Not technically true[3], "Any character may be escaped". This is only
true (I assume) when assuming that the current JSON lib that cryptsetup
uses is used to create the json header.

> > +   */
> > +  for (i = j = 0; i < inlen; i++) {
> > +    if (i + 1 < inlen && unescaped[i] == '\\' && unescaped[i + 1]
> > == '/')
> > +      continue;
> > +    unescaped[j++] = unescaped[i];
> > +  }

Another edge case would be to check for unicode escaped characters in
the base64 alphabet. This would allow us to parse all valid base64
encoded strings. Admittedly, it probably academic at this point because
there's probably no LUKS2 software encoding, for instance, "A" as
"\u0065". But I don't think it would be that hard to use a switch
checking for "\u" followed by 4 digits in the proper range and decoding
that.

> > +
> > +  err = base64_decode (unescaped, j, decoded, decodedlen);
> > +  grub_free (unescaped);
> > +  return err;
> > +}
> > +
> >  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 +420,9 @@ 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),
> > (char *)digest, &digestlen)) 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), (char
> > *)salt, &saltlen)) return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > "Invalid digest salt");
> >
> >    /* Configure the hash used for the digest. */
> > @@ -434,8 +460,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),
> > +                       (char *)salt, &saltlen))
> >      {
> >        ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid keyslot
> > salt"); goto err;
> 
> Daniel

I'm okay with the general approach of this patch after looking into
jsmn limitations.

Glenn

[1] https://github.com/zserge/jsmn/issues/189
[2] https://github.com/zserge/jsmn/issues/48
[3] https://datatracker.ietf.org/doc/html/rfc8259#section-7