grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Patrick Steinhardt
Subject: Re: [PATCH v4 2/2] luks2: Fix decoding of digests and salts with escaped chars
Date: Mon, 11 Jul 2022 12:39:58 +0200

On Thu, Jun 30, 2022 at 06:05:12PM +0200, Daniel Kiper wrote:
> On Mon, Jun 06, 2022 at 07:29:00AM +0200, Patrick Steinhardt 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>
> 
> I think this patch should be merged with the first one. Of course it
> requires similar fixes mentioned in my earlier reply. Plus a few
> additional ones mentioned below...

I think it's easier to review this as two independent atomic changes.
The decoding logic is complex enough to warrant its own patch given it
already requires some focus to properly review, and the second patch has
historic context around cryptsetup that's not relevant to JSON itself.

I'll send v5 with separate patches, but I'll change it if you insist.

> >  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..728f93a8c 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, grub_size_t inlen, grub_uint8_t 
> > *decoded, idx_t *decodedlen)
> > +{
> > +  grub_size_t unescaped_len = 0;
> > +  char *unescaped = NULL;
> > +  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, (size_t)unescaped_len, (char 
> > *)decoded, decodedlen);
> 
> s/(char *)decoded/(char *) decoded/
> 
> You need an additional space here.
> 
> s/(size_t)unescaped_len/(size_t) unescaped_len/
> 
> s/size_t/grub_size_t/? Hmmm... It is idx_t in declaration...
> Do we need cast here at all?

Yeah, should be `grub_size_t`. But the cast is required, isn't it?
`idx_t` is a typedef of `ptrdiff_t`, which is signed, and `grub_size_t`
is unsigned.

> > +  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),
> > +                      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;
> 
> Daniel

Patrick

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]