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

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

[Patrick Steinhardt]

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

Hi,

this is the fourth 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 v3:

    - Fixed the confusion between `size_t` and `grub_size_t` so that we
      consistently use the latter.

    - Improved error handling in `grub_json_unescape ()`: we now verify
      that the out-parameters are set, check for memory allocation
      errors and now return any errors encountered.

    - `luks2_base64_decode ()` now initializes the out-parameters it
      passes to `grub_json_unescape ()`.

This should address all the feedback by Glenn, except for modifying
`grub_json_unescape ()` to allow for in-place unescaping. I found the
end result to be less readable and more fragile when requiring the
caller to pass in a buffer, and we cannot make use of it right now
anyway. Thanks for your feedback!

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 | 101 ++++++++++++++++++++++++++++++++++++++
 grub-core/lib/json/json.h |  12 +++++
 3 files changed, 137 insertions(+), 4 deletions(-)

Range-diff against v3:
1:  3055f9f2f ! 1:  c2233323a json: Add function to unescape JSON-encoded strings
    @@ grub-core/lib/json/json.c: grub_json_getint64 (grub_int64_t *out, const grub_jso
      }
     +
     +grub_err_t
    -+grub_json_unescape (char **out, size_t *outlen, const char *in, size_t inlen)
    ++grub_json_unescape (char **out, grub_size_t *outlen, const char *in, grub_size_t inlen)
     +{
     +  grub_err_t ret = GRUB_ERR_NONE;
    -+  size_t inpos, resultpos;
    ++  grub_size_t inpos, resultpos;
     +  char *result;
     +
    ++  if (!out || !outlen)
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Output parameters are not set");
    ++
     +  result = grub_calloc (1, inlen + 1);
    ++  if (!result)
    ++    return GRUB_ERR_OUT_OF_MEMORY;
     +
     +  for (inpos = resultpos = 0; inpos < inlen; inpos++)
     +    {
    @@ grub-core/lib/json/json.c: grub_json_getint64 (grub_int64_t *out, const grub_jso
     +	    }
     +	}
     +      else
    -+	{
     +	  result[resultpos++] = in[inpos];
    -+	}
     +    }
     +
     +  *out = result;
    @@ grub-core/lib/json/json.c: grub_json_getint64 (grub_int64_t *out, const grub_jso
     +  if (ret != GRUB_ERR_NONE)
     +    grub_free (result);
     +
    -+  return GRUB_ERR_NONE;
    ++  return ret;
     +}
     
      ## grub-core/lib/json/json.h ##
    @@ grub-core/lib/json/json.h: extern grub_err_t EXPORT_FUNC(grub_json_getint64) (gr
     + * 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);
    ++extern grub_err_t EXPORT_FUNC(grub_json_unescape) (char **out, grub_size_t *outlen,
    ++						   const char *in, grub_size_t inlen);
     +
      #endif
2:  69424b2d1 ! 2:  84370adba luks2: Fix decoding of digests and salts with escaped chars
    @@ grub-core/disk/luks2.c: luks2_scan (grub_disk_t disk, grub_cryptomount_args_t ca
      }
      
     +static grub_err_t
    -+luks2_base64_decode (const char *in, size_t inlen, grub_uint8_t *decoded, idx_t *decodedlen)
    ++luks2_base64_decode (const char *in, grub_size_t inlen, grub_uint8_t *decoded, idx_t *decodedlen)
     +{
    -+  size_t unescaped_len;
    -+  char *unescaped;
    ++  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, unescaped_len, (char *)decoded, decodedlen);
    ++  successful = base64_decode (unescaped, (size_t)unescaped_len, (char *)decoded, decodedlen);
     +  grub_free (unescaped);
     +  if (!successful)
     +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not decode Base64 string");
-- 
2.36.1


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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4267 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 | 101 ++++++++++++++++++++++++++++++++++++++
 grub-core/lib/json/json.h |  12 +++++
 2 files changed, 113 insertions(+)

diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
index 1c20c75ea..adb4747a4 100644
--- a/grub-core/lib/json/json.c
+++ b/grub-core/lib/json/json.c
@@ -262,3 +262,104 @@ 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, grub_size_t *outlen, const char *in, grub_size_t inlen)
+{
+  grub_err_t ret = GRUB_ERR_NONE;
+  grub_size_t inpos, resultpos;
+  char *result;
+
+  if (!out || !outlen)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Output parameters are not set");
+
+  result = grub_calloc (1, inlen + 1);
+  if (!result)
+    return GRUB_ERR_OUT_OF_MEMORY;
+
+  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 ret;
+}
diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
index 4ea2a22d8..626074c35 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, grub_size_t *outlen,
+						   const char *in, grub_size_t inlen);
+
 #endif
-- 
2.36.1


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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3610 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..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);
+  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 v4 1/2] json: Add function to unescape JSON-encoded strings

[Glenn Washburn]

On Mon, 6 Jun 2022 07:28:56 +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>

Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn

> ---
>  grub-core/lib/json/json.c | 101 ++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h |  12 +++++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 1c20c75ea..adb4747a4 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -262,3 +262,104 @@ 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, grub_size_t *outlen, const char *in, grub_size_t inlen)
> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_size_t inpos, resultpos;
> +  char *result;
> +
> +  if (!out || !outlen)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Output parameters are not set");
> +
> +  result = grub_calloc (1, inlen + 1);
> +  if (!result)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +
> +  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 ret;
> +}
> diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
> index 4ea2a22d8..626074c35 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, grub_size_t *outlen,
> +						   const char *in, grub_size_t inlen);
> +
>  #endif

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

[Glenn Washburn]

On Mon, 6 Jun 2022 07:29:00 +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>

Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn

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

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

[Daniel Kiper]

On Mon, Jun 06, 2022 at 07:28:56AM +0200, Patrick Steinhardt 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 | 101 ++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h |  12 +++++
>  2 files changed, 113 insertions(+)
>
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 1c20c75ea..adb4747a4 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -262,3 +262,104 @@ 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, grub_size_t *outlen, const char *in, grub_size_t inlen)
> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_size_t inpos, resultpos;
> +  char *result;
> +
> +  if (!out || !outlen)

I prefer "if (out == NULL || outlen == NULL)"

Please fix similar issues below.

> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Output parameters are not set");

grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output parameters are not set"))

Two things are important here. First, you should use N_() in
grub_error() calls. It enables i18n for the GRUB. Second, all
grub_error() messages should start with lowercase except e.g.
abbreviations.

Please fix similar things below.

> +
> +  result = grub_calloc (1, inlen + 1);
> +  if (!result)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +
> +  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;

Please split this line into two...

result[resultpos++] = '"';
break;

> +	      case '/':
> +		result[resultpos++] = '/'; break;

result[resultpos++] = '/';
break;

> +	      case '\\':
> +		result[resultpos++] = '\\'; break;

Ditto and below please...

> +	      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};

Why values is unsigned char if c is char. I think values should be char too.

> +		  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++)

"i < ARRAY_SIZE(values)" instead of "i < 4"?

> +		    {
> +		      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;
> +		}

Please add empty line here and above after every break. OK, the first
one above does not need an additional empty line...

> +	      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:

Please add a space before "err" label.

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

Daniel

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

[Daniel Kiper]

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...

>  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?

> +  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

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

[Patrick Steinhardt]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]