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

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

[Patrick Steinhardt]

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

Hi,

this is the fifth 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 v4 include mostly style-related fixes pointed out by
Daniel. Please refer to the range-diff below.

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

Range-diff against v4:
1:  c2233323a ! 1:  ebab6b092 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_size_t inpos, resultpos;
     +  char *result;
     +
    -+  if (!out || !outlen)
    -+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Output parameters are not set");
    ++  if (out == NULL || outlen == NULL)
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output parameters are not set"));
     +
     +  result = grub_calloc (1, inlen + 1);
    -+  if (!result)
    ++  if (result == NULL)
     +    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
     +	  inpos++;
     +	  if (inpos >= inlen)
     +	    {
    -+	      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Expected escaped character");
    ++	      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("expected escaped character"));
     +	      goto err;
     +	    }
     +
     +	  switch (in[inpos])
     +	    {
     +	      case '"':
    -+		result[resultpos++] = '"'; break;
    ++		result[resultpos++] = '"';
    ++		break;
    ++
     +	      case '/':
    -+		result[resultpos++] = '/'; break;
    ++		result[resultpos++] = '/';
    ++		break;
    ++
     +	      case '\\':
    -+		result[resultpos++] = '\\'; break;
    ++		result[resultpos++] = '\\';
    ++		break;
    ++
     +	      case 'b':
    -+		result[resultpos++] = '\b'; break;
    ++		result[resultpos++] = '\b';
    ++		break;
    ++
     +	      case 'f':
    -+		result[resultpos++] = '\f'; break;
    ++		result[resultpos++] = '\f';
    ++		break;
    ++
     +	      case 'r':
    -+		result[resultpos++] = '\r'; break;
    ++		result[resultpos++] = '\r';
    ++		break;
    ++
     +	      case 'n':
    -+		result[resultpos++] = '\n'; break;
    ++		result[resultpos++] = '\n';
    ++		break;
    ++
     +	      case 't':
    -+		result[resultpos++] = '\t'; break;
    ++		result[resultpos++] = '\t';
    ++		break;
    ++
     +	      case 'u':
     +		{
    -+		  unsigned char values[4] = {0};
    -+		  int i;
    ++		  char values[4] = {0};
    ++		  unsigned i;
     +
     +		  inpos++;
     +		  if (inpos + ARRAY_SIZE(values) > inlen)
     +		    {
    -+		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unicode sequence too short");
    ++		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unicode sequence too short"));
     +		      goto err;
     +		    }
     +
    -+		  for (i = 0; i < 4; i++)
    ++		  for (i = 0; i < ARRAY_SIZE(values); i++)
     +		    {
     +		      char c = in[inpos++];
     +
    @@ grub-core/lib/json/json.c: grub_json_getint64 (grub_int64_t *out, const grub_jso
     +		      else
     +			{
     +			  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
    -+					    "Unicode sequence with invalid character '%c'", c);
    ++					    N_("unicode sequence with invalid character '%c'"), c);
     +			  goto err;
     +			}
     +		    }
    @@ grub-core/lib/json/json.c: grub_json_getint64 (grub_int64_t *out, const grub_jso
     +
     +		  break;
     +		}
    ++
     +	      default:
    -+		ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unrecognized escaped character '%c'", in[inpos]);
    ++		ret = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized escaped character '%c'"), in[inpos]);
     +		goto err;
     +	    }
     +	}
    @@ grub-core/lib/json/json.c: grub_json_getint64 (grub_int64_t *out, const grub_jso
     +  *out = result;
     +  *outlen = resultpos;
     +
    -+err:
    ++ err:
     +  if (ret != GRUB_ERR_NONE)
     +    grub_free (result);
     +
2:  84370adba ! 2:  60ccd669d 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
     +  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");
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("could not unescape Base64 string"));
     +
    -+  successful = base64_decode (unescaped, (size_t)unescaped_len, (char *)decoded, decodedlen);
    ++  successful = base64_decode (unescaped, (grub_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_error (GRUB_ERR_BAD_ARGUMENT, N_("could not decode Base64 string"));
     +
     +  return GRUB_ERR_NONE;
     +}
-- 
2.37.0


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

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

[Patrick Steinhardt]

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

diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
index 1c20c75ea..1eadd1ce9 100644
--- a/grub-core/lib/json/json.c
+++ b/grub-core/lib/json/json.c
@@ -262,3 +262,121 @@ 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 == NULL || outlen == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output parameters are not set"));
+
+  result = grub_calloc (1, inlen + 1);
+  if (result == NULL)
+    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, N_("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':
+		{
+		  char values[4] = {0};
+		  unsigned i;
+
+		  inpos++;
+		  if (inpos + ARRAY_SIZE(values) > inlen)
+		    {
+		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unicode sequence too short"));
+		      goto err;
+		    }
+
+		  for (i = 0; i < ARRAY_SIZE(values); 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,
+					    N_("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, N_("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.37.0


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

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

[Nicholas Vinson]

On 7/11/22 06:44, 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 | 118 ++++++++++++++++++++++++++++++++++++++
>   grub-core/lib/json/json.h |  12 ++++
>   2 files changed, 130 insertions(+)
> 
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 1c20c75ea..1eadd1ce9 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -262,3 +262,121 @@ 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 == NULL || outlen == NULL)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output parameters are not set"));
> +
> +  result = grub_calloc (1, inlen + 1);
> +  if (result == NULL)
> +    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, N_("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':
> +		{
> +		  char values[4] = {0};
> +		  unsigned i;
> +
> +		  inpos++;
> +		  if (inpos + ARRAY_SIZE(values) > inlen)
> +		    {
> +		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unicode sequence too short"));
> +		      goto err;
> +		    }

I recommend relaxing these errors a little bit. While technically
correct, I'm thinking it might serve GRUB better to be a bit more
forgiving.

Perhaps something like:

if the input is '\u????' where ? is not a hex digit, set the value to
'u', and process ???? separately.

if ???? is less than ARRAY_SIZE chars, left-pad ???? with 0s* until it's
the correct length then convert the value.

(* I don't mean to literally pad. I mean to treat the short number as if
it was left-padded with 0s)

> +
> +		  for (i = 0; i < ARRAY_SIZE(values); 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
> +			{

With a similar argument here. Treat the short ???? as suggested above
and resume normal processing at the non-hex digit position.

> +			  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
> +					    N_("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, N_("unrecognized escaped character '%c'"), in[inpos]);
> +		goto err;

And the final prong of the suggestion would be here. Treat the improper
escape as if it had not been escaped at all or as '\\?' where '?' is the
escaped character.

Thanks,
Nicholas Vinson

> +	    }
> +	}
> +      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
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Daniel Kiper]

On Mon, Jul 11, 2022 at 09:08:09AM -0400, Nicholas Vinson wrote:
> On 7/11/22 06:44, 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 | 118 ++++++++++++++++++++++++++++++++++++++
> >   grub-core/lib/json/json.h |  12 ++++
> >   2 files changed, 130 insertions(+)
> >
> > diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> > index 1c20c75ea..1eadd1ce9 100644
> > --- a/grub-core/lib/json/json.c
> > +++ b/grub-core/lib/json/json.c
> > @@ -262,3 +262,121 @@ 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 == NULL || outlen == NULL)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output parameters are not set"));
> > +
> > +  result = grub_calloc (1, inlen + 1);
> > +  if (result == NULL)
> > +    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, N_("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':
> > +		{
> > +		  char values[4] = {0};
> > +		  unsigned i;
> > +
> > +		  inpos++;
> > +		  if (inpos + ARRAY_SIZE(values) > inlen)
> > +		    {
> > +		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unicode sequence too short"));
> > +		      goto err;
> > +		    }
>
> I recommend relaxing these errors a little bit. While technically
> correct, I'm thinking it might serve GRUB better to be a bit more
> forgiving.
>
> Perhaps something like:
>
> if the input is '\u????' where ? is not a hex digit, set the value to
> 'u', and process ???? separately.
>
> if ???? is less than ARRAY_SIZE chars, left-pad ???? with 0s* until it's
> the correct length then convert the value.
>
> (* I don't mean to literally pad. I mean to treat the short number as if
> it was left-padded with 0s)
>
> > +
> > +		  for (i = 0; i < ARRAY_SIZE(values); 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
> > +			{
>
> With a similar argument here. Treat the short ???? as suggested above
> and resume normal processing at the non-hex digit position.
>
> > +			  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +					    N_("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, N_("unrecognized escaped character '%c'"), in[inpos]);
> > +		goto err;
>
> And the final prong of the suggestion would be here. Treat the improper
> escape as if it had not been escaped at all or as '\\?' where '?' is the
> escaped character.

Nicholas comments make sense for me.

Patrick?

Daniel

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

[Patrick Steinhardt]

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

On Tue, Jul 12, 2022 at 03:39:13PM +0200, Daniel Kiper wrote:
> On Mon, Jul 11, 2022 at 09:08:09AM -0400, Nicholas Vinson wrote:
> > On 7/11/22 06:44, 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 | 118 ++++++++++++++++++++++++++++++++++++++
> > >   grub-core/lib/json/json.h |  12 ++++
> > >   2 files changed, 130 insertions(+)
> > >
> > > diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> > > index 1c20c75ea..1eadd1ce9 100644
> > > --- a/grub-core/lib/json/json.c
> > > +++ b/grub-core/lib/json/json.c
> > > @@ -262,3 +262,121 @@ 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 == NULL || outlen == NULL)
> > > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output parameters are not set"));
> > > +
> > > +  result = grub_calloc (1, inlen + 1);
> > > +  if (result == NULL)
> > > +    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, N_("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':
> > > +		{
> > > +		  char values[4] = {0};
> > > +		  unsigned i;
> > > +
> > > +		  inpos++;
> > > +		  if (inpos + ARRAY_SIZE(values) > inlen)
> > > +		    {
> > > +		      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unicode sequence too short"));
> > > +		      goto err;
> > > +		    }
> >
> > I recommend relaxing these errors a little bit. While technically
> > correct, I'm thinking it might serve GRUB better to be a bit more
> > forgiving.
> >
> > Perhaps something like:
> >
> > if the input is '\u????' where ? is not a hex digit, set the value to
> > 'u', and process ???? separately.
> >
> > if ???? is less than ARRAY_SIZE chars, left-pad ???? with 0s* until it's
> > the correct length then convert the value.
> >
> > (* I don't mean to literally pad. I mean to treat the short number as if
> > it was left-padded with 0s)
> >
> > > +
> > > +		  for (i = 0; i < ARRAY_SIZE(values); 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
> > > +			{
> >
> > With a similar argument here. Treat the short ???? as suggested above
> > and resume normal processing at the non-hex digit position.
> >
> > > +			  ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > +					    N_("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, N_("unrecognized escaped character '%c'"), in[inpos]);
> > > +		goto err;
> >
> > And the final prong of the suggestion would be here. Treat the improper
> > escape as if it had not been escaped at all or as '\\?' where '?' is the
> > escaped character.
> 
> Nicholas comments make sense for me.
> 
> Patrick?

They do make sense indeed. But it feels like we're now iterating more on
the way we decode JSON-strings, which is exactly the kind of reason why
I initially wanted to punt on that and just decode only those characters
that cryptsetup v2.0.0 would've written with an escape character back
then. And this really only is the backslash character.

I don't think it's sensible to make this decoding more complicated by
handling all these different cases when we know that there only was a
single version of cryptsetup that would only have written a single
character with optional escaping, and that this case was changed at a
later point in time to not do that anymore. So ultimately this all feels
rather theoretical at this point, so I lean towards being pragmatic and
making adjustments at a later point when we ever see that this is indeed
a real-world problem.

Patrick

> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3625 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..c24c6e98d 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, N_("could not unescape Base64 string"));
+
+  successful = base64_decode (unescaped, (grub_size_t) unescaped_len, (char *) decoded, decodedlen);
+  grub_free (unescaped);
+  if (!successful)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("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.37.0


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

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

[Daniel Kiper]

On Mon, Jul 11, 2022 at 12:44:59PM +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

Any chance to add this guy email here?

> 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..c24c6e98d 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, N_("could not unescape Base64 string"));
> +
> +  successful = base64_decode (unescaped, (grub_size_t) unescaped_len, (char *) decoded, decodedlen);

Now (grub_size_t) cast seems redundant because unescaped_len is defined
as grub_size_t.

Otherwise patch LGTM. So, if you fix this you can add my RB.

> +  grub_free (unescaped);
> +  if (!successful)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("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 v5 2/2] luks2: Fix decoding of digests and salts with escaped chars

[Patrick Steinhardt]

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

On Tue, Jul 12, 2022 at 03:30:22PM +0200, Daniel Kiper wrote:
> On Mon, Jul 11, 2022 at 12:44:59PM +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
> 
> Any chance to add this guy email here?

He didn't want his mail address to be associated with his name here.

> > 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..c24c6e98d 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, N_("could not unescape Base64 string"));
> > +
> > +  successful = base64_decode (unescaped, (grub_size_t) unescaped_len, (char *) decoded, decodedlen);
> 
> Now (grub_size_t) cast seems redundant because unescaped_len is defined
> as grub_size_t.
> 
> Otherwise patch LGTM. So, if you fix this you can add my RB.

Oops, right. Will fix.

Patrick

> > +  grub_free (unescaped);
> > +  if (!successful)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("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

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