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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5617 bytes --]

Hi,

this is the third version of my patch series which fixes decoding of
digests and salts in LUKS2 headers in case they happen to contain
escaped characters. While modern cryptsetup versions in fact don't
escape any characters part of the Base64 alphabet, old versions of
cryptsetup did this until v2.0.2.

Changes compared to v2:

    - I've split out the logic to unescape JSON-escaped strings into a
      new `grub_json_unescape ()` function. This function now provides
      full support for unescaping as specified in the JSON standard.

    - I've converted the errors returned by `luks2_base64_decode ()` to
      use `grub_error ()` to provide more context. Note though that I
      haven't yet started to use `grub_error_push ()` as Glenn proposes:
      as he said, that's a more general refactoring and I thus feel like
      it should be part of a different patch series.

    - I've done my research and finally found out why this was only
      happening on Ubuntu 18.04, which uses cryptsetup v2.0.2, and
      documented this in the commit message.

Patrick

Patrick Steinhardt (2):
  json: Add function to unescape JSON-encoded strings
  luks2: Fix decoding of digests and salts with escaped chars

 grub-core/disk/luks2.c    | 28 +++++++++--
 grub-core/lib/json/json.c | 98 +++++++++++++++++++++++++++++++++++++++
 grub-core/lib/json/json.h | 12 +++++
 3 files changed, 134 insertions(+), 4 deletions(-)

Range-diff against v2:
-:  --------- > 1:  3055f9f2f json: Add function to unescape JSON-encoded strings
1:  b5a9fcdb5 ! 2:  69424b2d1 luks2: Fix decoding of digests and salts with escaped chars
    @@ Commit message
     
         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.
    +    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.
     
    -    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.
    +    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 ##
    -@@ grub-core/disk/luks2.c: luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
    +@@ grub-core/disk/luks2.c: 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, size_t *decodedlen)
    ++luks2_base64_decode (const char *in, size_t inlen, grub_uint8_t *decoded, idx_t *decodedlen)
     +{
    ++  size_t unescaped_len;
     +  char *unescaped;
     +  bool successful;
    -+  size_t i, j;
     +
    -+  unescaped = grub_malloc (inlen);
    -+  if (!unescaped)
    -+    return GRUB_ERR_OUT_OF_MEMORY;
    ++  if (grub_json_unescape (&unescaped, &unescaped_len, in, inlen) != GRUB_ERR_NONE)
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not unescape Base64 string");
     +
    -+  grub_memmove (unescaped, in, inlen);
    -+
    -+  /*
    -+   * 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.
    -+   *
    -+   * 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];
    -+  }
    -+
    -+  successful = base64_decode (unescaped, j, (char *)decoded, decodedlen);
    ++  successful = base64_decode (unescaped, unescaped_len, (char *)decoded, decodedlen);
     +  grub_free (unescaped);
     +  if (!successful)
    -+    return GRUB_ERR_BAD_ARGUMENT;
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not decode Base64 string");
     +
     +  return GRUB_ERR_NONE;
     +}
-- 
2.36.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 1/2] json: Add function to unescape JSON-encoded strings

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4097 bytes --]

JSON strings require certain characters to be encoded, either by using a
single reverse solidus character "\" for a set of popular characters, or
by using a Unicode representation of "\uXXXXX". The jsmn library doesn't
handle unescaping for us, so we must implement this functionality for
ourselves.

Add a new function `grub_json_unescape ()` that takes a potentially
escaped JSON string as input and returns a new unescaped string.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/lib/json/json.c | 98 +++++++++++++++++++++++++++++++++++++++
 grub-core/lib/json/json.h | 12 +++++
 2 files changed, 110 insertions(+)

diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
index 1c20c75ea..3e5e25f79 100644
--- a/grub-core/lib/json/json.c
+++ b/grub-core/lib/json/json.c
@@ -262,3 +262,101 @@ grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char *ke
 
   return GRUB_ERR_NONE;
 }
+
+grub_err_t
+grub_json_unescape (char **out, size_t *outlen, const char *in, size_t inlen)
+{
+  grub_err_t ret = GRUB_ERR_NONE;
+  size_t inpos, resultpos;
+  char *result;
+
+  result = grub_calloc (1, inlen + 1);
+
+  for (inpos = resultpos = 0; inpos < inlen; inpos++)
+    {
+      if (in[inpos] == '\\')
+	{
+	  inpos++;
+	  if (inpos >= inlen)
+	    {
+	      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Expected escaped character");
+	      goto err;
+	    }
+
+	  switch (in[inpos])
+	    {
+	      case '"':
+		result[resultpos++] = '"'; break;
+	      case '/':
+		result[resultpos++] = '/'; break;
+	      case '\\':
+		result[resultpos++] = '\\'; break;
+	      case 'b':
+		result[resultpos++] = '\b'; break;
+	      case 'f':
+		result[resultpos++] = '\f'; break;
+	      case 'r':
+		result[resultpos++] = '\r'; break;
+	      case 'n':
+		result[resultpos++] = '\n'; break;
+	      case 't':
+		result[resultpos++] = '\t'; break;
+	      case 'u':
+		{
+		  unsigned char values[4] = {0};
+		  int i;
+
+		  inpos++;
+		  if (inpos + ARRAY_SIZE(values) > inlen)
+		    {
+		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unicode sequence too short");
+		      goto err;
+		    }
+
+		  for (i = 0; i < 4; i++)
+		    {
+		      char c = in[inpos++];
+
+		      if (c >= '0' && c <= '9')
+			values[i] = c - '0';
+		      else if (c >= 'A' && c <= 'F')
+			values[i] = c - 'A' + 10;
+		      else if (c >= 'a' && c <= 'f')
+			values[i] = c - 'a' + 10;
+		      else
+			{
+			  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
+					    "Unicode sequence with invalid character '%c'", c);
+			  goto err;
+			}
+		    }
+
+		  if (values[0] != 0 || values[1] != 0)
+		    result[resultpos++] = values[0] << 4 | values[1];
+		  result[resultpos++] = values[2] << 4 | values[3];
+
+		  /* Offset the increment that's coming in via the loop increment. */
+		  inpos--;
+
+		  break;
+		}
+	      default:
+		ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unrecognized escaped character '%c'", in[inpos]);
+		goto err;
+	    }
+	}
+      else
+	{
+	  result[resultpos++] = in[inpos];
+	}
+    }
+
+  *out = result;
+  *outlen = resultpos;
+
+err:
+  if (ret != GRUB_ERR_NONE)
+    grub_free (result);
+
+  return GRUB_ERR_NONE;
+}
diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
index 4ea2a22d8..37a5a5f57 100644
--- a/grub-core/lib/json/json.h
+++ b/grub-core/lib/json/json.h
@@ -125,4 +125,16 @@ extern grub_err_t EXPORT_FUNC(grub_json_getint64) (grub_int64_t *out,
 						   const grub_json_t *parent,
 						   const char *key);
 
+/*
+ * Unescape escaped characters and Unicode sequences in the
+ * given JSON-encoded string. Returns a newly allocated string
+ * passed back via the `out` parameter that has a length of
+ * `*outlen`.
+ *
+ * See https://datatracker.ietf.org/doc/html/rfc8259#section-7 for more
+ * information on escaping in JSON.
+ */
+extern grub_err_t EXPORT_FUNC(grub_json_unescape) (char **out, size_t *outlen,
+						   const char *in, size_t inlen);
+
 #endif
-- 
2.36.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

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)
+{
+  size_t unescaped_len;
+  char *unescaped;
+  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);
+  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;
-- 
2.36.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/2] json: Add function to unescape JSON-encoded strings

[Glenn Washburn]

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

> JSON strings require certain characters to be encoded, either by using a
> single reverse solidus character "\" for a set of popular characters, or
> by using a Unicode representation of "\uXXXXX". The jsmn library doesn't
> handle unescaping for us, so we must implement this functionality for
> ourselves.
> 
> Add a new function `grub_json_unescape ()` that takes a potentially
> escaped JSON string as input and returns a new unescaped string.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/lib/json/json.c | 98 +++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h | 12 +++++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 1c20c75ea..3e5e25f79 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -262,3 +262,101 @@ grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char *ke
>  
>    return GRUB_ERR_NONE;
>  }
> +
> +grub_err_t
> +grub_json_unescape (char **out, size_t *outlen, const char *in, size_t inlen)

Why not grub_size_t instead of size_t?

> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  size_t inpos, resultpos;

Same here.

> +  char *result;
> +
> +  result = grub_calloc (1, inlen + 1);

Should check for grub_calloc failure.

I like the idea of being able to do an inplace conversion if in ==
*out, which would save on memory and be safe as the out string length
should always be less than or equal to the length of the in string.
I'll admit its not really necessary for the current use-case.

> +
> +  for (inpos = resultpos = 0; inpos < inlen; inpos++)
> +    {
> +      if (in[inpos] == '\\')
> +	{
> +	  inpos++;
> +	  if (inpos >= inlen)
> +	    {
> +	      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Expected escaped character");
> +	      goto err;
> +	    }
> +
> +	  switch (in[inpos])
> +	    {
> +	      case '"':
> +		result[resultpos++] = '"'; break;
> +	      case '/':
> +		result[resultpos++] = '/'; break;
> +	      case '\\':
> +		result[resultpos++] = '\\'; break;
> +	      case 'b':
> +		result[resultpos++] = '\b'; break;
> +	      case 'f':
> +		result[resultpos++] = '\f'; break;
> +	      case 'r':
> +		result[resultpos++] = '\r'; break;
> +	      case 'n':
> +		result[resultpos++] = '\n'; break;
> +	      case 't':
> +		result[resultpos++] = '\t'; break;
> +	      case 'u':
> +		{
> +		  unsigned char values[4] = {0};
> +		  int i;
> +
> +		  inpos++;
> +		  if (inpos + ARRAY_SIZE(values) > inlen)
> +		    {
> +		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unicode sequence too short");
> +		      goto err;
> +		    }
> +
> +		  for (i = 0; i < 4; i++)
> +		    {
> +		      char c = in[inpos++];
> +
> +		      if (c >= '0' && c <= '9')
> +			values[i] = c - '0';
> +		      else if (c >= 'A' && c <= 'F')
> +			values[i] = c - 'A' + 10;
> +		      else if (c >= 'a' && c <= 'f')
> +			values[i] = c - 'a' + 10;
> +		      else
> +			{
> +			  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
> +					    "Unicode sequence with invalid character '%c'", c);
> +			  goto err;
> +			}
> +		    }
> +
> +		  if (values[0] != 0 || values[1] != 0)
> +		    result[resultpos++] = values[0] << 4 | values[1];
> +		  result[resultpos++] = values[2] << 4 | values[3];
> +
> +		  /* Offset the increment that's coming in via the loop increment. */
> +		  inpos--;
> +
> +		  break;
> +		}
> +	      default:
> +		ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unrecognized escaped character '%c'", in[inpos]);
> +		goto err;
> +	    }
> +	}
> +      else
> +	{
> +	  result[resultpos++] = in[inpos];
> +	}

Iirc, Daniel has questioned the need for brackets when the block is
one line.

> +    }
> +
> +  *out = result;
> +  *outlen = resultpos;

Might be good to check these, out and outlen, pointers for NULL before
using.

> +
> +err:
> +  if (ret != GRUB_ERR_NONE)
> +    grub_free (result);
> +
> +  return GRUB_ERR_NONE;

This should be "return ret;", otherwise this function always returns
success, even when it sets grub_errno.

> +}
> diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
> index 4ea2a22d8..37a5a5f57 100644
> --- a/grub-core/lib/json/json.h
> +++ b/grub-core/lib/json/json.h
> @@ -125,4 +125,16 @@ extern grub_err_t EXPORT_FUNC(grub_json_getint64) (grub_int64_t *out,
>  						   const grub_json_t *parent,
>  						   const char *key);
>  
> +/*
> + * Unescape escaped characters and Unicode sequences in the
> + * given JSON-encoded string. Returns a newly allocated string
> + * passed back via the `out` parameter that has a length of
> + * `*outlen`.
> + *
> + * See https://datatracker.ietf.org/doc/html/rfc8259#section-7 for more
> + * information on escaping in JSON.
> + */
> +extern grub_err_t EXPORT_FUNC(grub_json_unescape) (char **out, size_t *outlen,
> +						   const char *in, size_t inlen);
> +
>  #endif

Glenn

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

Re: [PATCH v3 1/2] json: Add function to unescape JSON-encoded strings

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]

On Sun, Jun 05, 2022 at 02:00:44PM -0500, Glenn Washburn wrote:
> On Mon, 30 May 2022 18:01:01 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > JSON strings require certain characters to be encoded, either by using a
> > single reverse solidus character "\" for a set of popular characters, or
> > by using a Unicode representation of "\uXXXXX". The jsmn library doesn't
> > handle unescaping for us, so we must implement this functionality for
> > ourselves.
> > 
> > Add a new function `grub_json_unescape ()` that takes a potentially
> > escaped JSON string as input and returns a new unescaped string.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  grub-core/lib/json/json.c | 98 +++++++++++++++++++++++++++++++++++++++
> >  grub-core/lib/json/json.h | 12 +++++
> >  2 files changed, 110 insertions(+)
> > 
> > diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> > index 1c20c75ea..3e5e25f79 100644
> > --- a/grub-core/lib/json/json.c
> > +++ b/grub-core/lib/json/json.c
> > @@ -262,3 +262,101 @@ grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char *ke
> >  
> >    return GRUB_ERR_NONE;
> >  }
> > +
> > +grub_err_t
> > +grub_json_unescape (char **out, size_t *outlen, const char *in, size_t inlen)
> 
> Why not grub_size_t instead of size_t?
> 
> > +{
> > +  grub_err_t ret = GRUB_ERR_NONE;
> > +  size_t inpos, resultpos;
> 
> Same here.
> 
> > +  char *result;
> > +
> > +  result = grub_calloc (1, inlen + 1);
> 
> Should check for grub_calloc failure.
> 
> I like the idea of being able to do an inplace conversion if in ==
> *out, which would save on memory and be safe as the out string length
> should always be less than or equal to the length of the in string.
> I'll admit its not really necessary for the current use-case.
[snip]

Yeah, I thought about this, and in fact my first iteration did exactly
that. But it complicates the interface somewhat given that you now have
to pass in the caller-allocated size, return the end result's size and
also have to somehow handle NUL-terminating the result. So altogether it
added complexity and felt a lot more fragile compared to just allocating
the result array.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/2] json: Add function to unescape JSON-encoded strings

[Glenn Washburn]

On Mon, 6 Jun 2022 07:18:28 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Sun, Jun 05, 2022 at 02:00:44PM -0500, Glenn Washburn wrote:
> > On Mon, 30 May 2022 18:01:01 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> > 
> > > JSON strings require certain characters to be encoded, either by using a
> > > single reverse solidus character "\" for a set of popular characters, or
> > > by using a Unicode representation of "\uXXXXX". The jsmn library doesn't
> > > handle unescaping for us, so we must implement this functionality for
> > > ourselves.
> > > 
> > > Add a new function `grub_json_unescape ()` that takes a potentially
> > > escaped JSON string as input and returns a new unescaped string.
> > > 
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > >  grub-core/lib/json/json.c | 98 +++++++++++++++++++++++++++++++++++++++
> > >  grub-core/lib/json/json.h | 12 +++++
> > >  2 files changed, 110 insertions(+)
> > > 
> > > diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> > > index 1c20c75ea..3e5e25f79 100644
> > > --- a/grub-core/lib/json/json.c
> > > +++ b/grub-core/lib/json/json.c
> > > @@ -262,3 +262,101 @@ grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char *ke
> > >  
> > >    return GRUB_ERR_NONE;
> > >  }
> > > +
> > > +grub_err_t
> > > +grub_json_unescape (char **out, size_t *outlen, const char *in, size_t inlen)
> > 
> > Why not grub_size_t instead of size_t?
> > 
> > > +{
> > > +  grub_err_t ret = GRUB_ERR_NONE;
> > > +  size_t inpos, resultpos;
> > 
> > Same here.
> > 
> > > +  char *result;
> > > +
> > > +  result = grub_calloc (1, inlen + 1);
> > 
> > Should check for grub_calloc failure.
> > 
> > I like the idea of being able to do an inplace conversion if in ==
> > *out, which would save on memory and be safe as the out string length
> > should always be less than or equal to the length of the in string.
> > I'll admit its not really necessary for the current use-case.
> [snip]
> 
> Yeah, I thought about this, and in fact my first iteration did exactly
> that. But it complicates the interface somewhat given that you now have
> to pass in the caller-allocated size, return the end result's size and
> also have to somehow handle NUL-terminating the result. So altogether it
> added complexity and felt a lot more fragile compared to just allocating
> the result array.

My thought was to have the caller pass in outlen pointing to the length
of the already allocated output array. No need for extra parameters.
Then outlen would be written to with the number of bytes written to the
output buffer. As for NULL terminating the result, no need to either,
just return an error indicating that the buffer was too small. I was
also thinking of keeping the current functionality, eg. output buffer
allocation, when *out == NULL. But yeah, this can be done some other
day when it would actually be useful.

Glenn