[PATCH 0/4] Various LUKS2 improvements

[PATCH 0/4] Various LUKS2 improvements

[Glenn Washburn]

Patch #1: Allows GRUB to be more resilient in the fact or unexpected json
  data, thus allowing access to LUKS2 volumes in cases where currently it
  would be inaccessible
Patch #2-3: Add some text to go along with the error in case it gets
  bubbled up to the user
Patch #4: Simplifies some error handling and makes the code a little easier
  to read

Glenn

Glenn Washburn (4):
  luks2: Continue trying all keyslots even if there are some failures
  luks2: Add error message strings to crypto errors
  luks2: Add error message strings to errors in luks2_read_header
  luks2: Fix potential grub_free with NULL pointer

 grub-core/disk/luks2.c | 58 ++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 30 deletions(-)

-- 
2.27.0

[PATCH 1/4] luks2: Continue trying all keyslots even if there are some failures

[Glenn Washburn]

luks2_get_keyslot can fail for a variety of reasons that do not neccesarily
mean the next keyslot should not be tried (eg. a new kdf type). So always
try the next slot. This will make GRUB more resilient to non-spec json data
that 3rd party systems may add. We do not care if some of the keyslots are
unusable, only if there is at least one that is.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 125e8609a..199e11473 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -617,7 +617,15 @@ luks2_recover_key (grub_disk_t source,
       grub_errno = GRUB_ERR_NONE;
       ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, json_idx);
       if (ret)
-	goto err;
+	{
+	  /*
+	   * luks2_get_keyslot can fail for a variety of reasons that do not
+	   * neccesarily mean the next keyslot should not be tried (eg. a new
+	   * kdf type). So always try the next slot.
+	   */
+	  grub_dprintf ("luks2", "Failed to get keyslot %" PRIuGRUB_UINT64_T "\n", keyslot.idx);
+	  continue;
+	}
       if (grub_errno != GRUB_ERR_NONE)
 	  grub_dprintf ("luks2", "Ignoring unhandled error %d from luks2_get_keyslot\n", grub_errno);
 
-- 
2.27.0

[PATCH 2/4] luks2: Add error message strings to crypto errors

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 199e11473..4b259cbdb 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -411,7 +411,8 @@ luks2_verify_key (grub_luks2_digest_t *d, grub_uint8_t *candidate_key,
 				 d->iterations,
 				 candidate_digest, digestlen);
   if (gcry_ret)
-    return grub_crypto_gcry_error (gcry_ret);
+    return grub_error (grub_crypto_gcry_error (gcry_ret),
+		       "grub_crypto_pbkdf2 failed with code %d", gcry_ret);
 
   if (grub_memcmp (candidate_digest, digest, digestlen) != 0)
     return grub_error (GRUB_ERR_ACCESS_DENIED, "Mismatching digests");
@@ -463,7 +464,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 				       area_key, k->area.key_size);
 	if (gcry_ret)
 	  {
-	    ret = grub_crypto_gcry_error (gcry_ret);
+	    ret = grub_error (grub_crypto_gcry_error (gcry_ret),
+			      "grub_crypto_pbkdf2 failed with code %d", gcry_ret);
 	    goto err;
 	  }
 
@@ -484,7 +486,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   gcry_ret = grub_cryptodisk_setkey (crypt, area_key, k->area.key_size);
   if (gcry_ret)
     {
-      ret = grub_crypto_gcry_error (gcry_ret);
+      ret = grub_error (grub_crypto_gcry_error (gcry_ret),
+		        "grub_cryptodisk_setkey failed with code %d", gcry_ret);
       goto err;
     }
 
@@ -512,7 +515,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 				      GRUB_LUKS1_LOG_SECTOR_SIZE);
   if (gcry_ret)
     {
-      ret = grub_crypto_gcry_error (gcry_ret);
+      ret = grub_error (grub_crypto_gcry_error (gcry_ret),
+			"grub_cryptodisk_decrypt failed with code %d", gcry_ret);
       goto err;
     }
 
@@ -529,7 +533,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   gcry_ret = AF_merge (hash, split_key, out_key, k->key_size, k->af.stripes);
   if (gcry_ret)
     {
-      ret = grub_crypto_gcry_error (gcry_ret);
+      ret = grub_error (grub_crypto_gcry_error (gcry_ret),
+			"AF_merge failed with code %d", gcry_ret);
       goto err;
     }
 
@@ -770,7 +775,8 @@ luks2_recover_key (grub_disk_t source,
   gcry_ret = grub_cryptodisk_setkey (crypt, candidate_key, candidate_key_len);
   if (gcry_ret)
     {
-      ret = grub_crypto_gcry_error (gcry_ret);
+      ret = grub_error (grub_crypto_gcry_error (gcry_ret),
+			"grub_cryptodisk_setkey failed with code %d", gcry_ret);
       goto err;
     }
 
-- 
2.27.0

[PATCH 3/4] luks2: Add error message strings to errors in luks2_read_header

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 4b259cbdb..2ed9e5a80 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -328,7 +328,7 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
   /* Look for LUKS magic sequence.  */
   if (grub_memcmp (primary.magic, LUKS_MAGIC_1ST, sizeof (primary.magic)) ||
       grub_be_to_cpu16 (primary.version) != 2)
-    return GRUB_ERR_BAD_SIGNATURE;
+    return grub_error (GRUB_ERR_BAD_SIGNATURE, "Bad primary signature");
 
   /* Read the secondary header. */
   ret = grub_disk_read (disk, 0, grub_be_to_cpu64 (primary.hdr_size), sizeof (secondary), &secondary);
@@ -338,7 +338,7 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
   /* Look for LUKS magic sequence.  */
   if (grub_memcmp (secondary.magic, LUKS_MAGIC_2ND, sizeof (secondary.magic)) ||
       grub_be_to_cpu16 (secondary.version) != 2)
-    return GRUB_ERR_BAD_SIGNATURE;
+    return grub_error (GRUB_ERR_BAD_SIGNATURE, "Bad secondary signature");
 
   if (grub_be_to_cpu64 (primary.seqid) < grub_be_to_cpu64 (secondary.seqid))
       header = &secondary;
-- 
2.27.0

[PATCH 4/4] luks2: Fix potential grub_free with NULL pointer

[Glenn Washburn]

This is not strictly necessary because grub_free does nothing if it is
passed a NULL pointer. However, it simplifies the code and makes it a tad
bit easier to read.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 2ed9e5a80..0e86325c7 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -437,25 +437,18 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 
   if (!base64_decode (k->kdf.salt, grub_strlen (k->kdf.salt),
 		     (char *)salt, &saltlen))
-    {
-      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid keyslot salt");
-      goto err;
-    }
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid keyslot salt");
 
   /* Calculate the binary area key of the user supplied passphrase. */
   switch (k->kdf.type)
     {
       case LUKS2_KDF_TYPE_ARGON2I:
-	ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Argon2 not supported");
-	goto err;
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Argon2 not supported");
       case LUKS2_KDF_TYPE_PBKDF2:
 	hash = grub_crypto_lookup_md_by_name (k->kdf.u.pbkdf2.hash);
 	if (!hash)
-	  {
-	    ret = grub_error (GRUB_ERR_FILE_NOT_FOUND, "Couldn't load %s hash",
-			      k->kdf.u.pbkdf2.hash);
-	    goto err;
-	  }
+	  return grub_error (GRUB_ERR_FILE_NOT_FOUND, "Couldn't load %s hash",
+			    k->kdf.u.pbkdf2.hash);
 
 	gcry_ret = grub_crypto_pbkdf2 (hash, (grub_uint8_t *) passphrase,
 				       passphraselen,
@@ -463,11 +456,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 				       k->kdf.u.pbkdf2.iterations,
 				       area_key, k->area.key_size);
 	if (gcry_ret)
-	  {
-	    ret = grub_error (grub_crypto_gcry_error (gcry_ret),
-			      "grub_crypto_pbkdf2 failed with code %d", gcry_ret);
-	    goto err;
-	  }
+	  return grub_error (grub_crypto_gcry_error (gcry_ret),
+			     "grub_crypto_pbkdf2 failed with code %d", gcry_ret);
 
 	break;
     }
@@ -485,19 +475,13 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 
   gcry_ret = grub_cryptodisk_setkey (crypt, area_key, k->area.key_size);
   if (gcry_ret)
-    {
-      ret = grub_error (grub_crypto_gcry_error (gcry_ret),
-		        "grub_cryptodisk_setkey failed with code %d", gcry_ret);
-      goto err;
-    }
+    return grub_error (grub_crypto_gcry_error (gcry_ret),
+		       "grub_cryptodisk_setkey failed with code %d", gcry_ret);
 
  /* Read and decrypt the binary key area with the area key. */
   split_key = grub_malloc (k->area.size);
   if (!split_key)
-    {
-      ret = grub_errno;
-      goto err;
-    }
+    return grub_errno;
 
   grub_errno = GRUB_ERR_NONE;
   ret = grub_disk_read (source, 0, k->area.offset, k->area.size, split_key);
-- 
2.27.0

Re: [PATCH 0/4] Various LUKS2 improvements

[Daniel Kiper]

On Fri, Mar 19, 2021 at 07:14:47PM -0500, Glenn Washburn wrote:
> Patch #1: Allows GRUB to be more resilient in the fact or unexpected json
>   data, thus allowing access to LUKS2 volumes in cases where currently it
>   would be inaccessible
> Patch #2-3: Add some text to go along with the error in case it gets
>   bubbled up to the user
> Patch #4: Simplifies some error handling and makes the code a little easier
>   to read
>
> Glenn
>
> Glenn Washburn (4):
>   luks2: Continue trying all keyslots even if there are some failures
>   luks2: Add error message strings to crypto errors
>   luks2: Add error message strings to errors in luks2_read_header
>   luks2: Fix potential grub_free with NULL pointer

Are there any of these patches critical and should be applied before release?

Daniel

Re: [PATCH 0/4] Various LUKS2 improvements

[Daniel Kiper]

On Tue, Mar 23, 2021 at 06:43:59PM +0100, Daniel Kiper wrote:
> On Fri, Mar 19, 2021 at 07:14:47PM -0500, Glenn Washburn wrote:
> > Patch #1: Allows GRUB to be more resilient in the fact or unexpected json
> >   data, thus allowing access to LUKS2 volumes in cases where currently it
> >   would be inaccessible
> > Patch #2-3: Add some text to go along with the error in case it gets
> >   bubbled up to the user
> > Patch #4: Simplifies some error handling and makes the code a little easier
> >   to read
> >
> > Glenn
> >
> > Glenn Washburn (4):
> >   luks2: Continue trying all keyslots even if there are some failures
> >   luks2: Add error message strings to crypto errors
> >   luks2: Add error message strings to errors in luks2_read_header
> >   luks2: Fix potential grub_free with NULL pointer
>
> Are there any of these patches critical and should be applied before release?

...and may I ask you to send me links to the patches which you send to
the grub-devel recently which should be taken into 2.06 release?

Daniel

Re: [PATCH 0/4] Various LUKS2 improvements

[Glenn Washburn]

On Tue, 23 Mar 2021 18:43:59 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Mar 19, 2021 at 07:14:47PM -0500, Glenn Washburn wrote:
> > Patch #1: Allows GRUB to be more resilient in the fact or
> > unexpected json data, thus allowing access to LUKS2 volumes in
> > cases where currently it would be inaccessible
> > Patch #2-3: Add some text to go along with the error in case it gets
> >   bubbled up to the user
> > Patch #4: Simplifies some error handling and makes the code a
> > little easier to read
> >
> > Glenn
> >
> > Glenn Washburn (4):
> >   luks2: Continue trying all keyslots even if there are some
> > failures luks2: Add error message strings to crypto errors
> >   luks2: Add error message strings to errors in luks2_read_header
> >   luks2: Fix potential grub_free with NULL pointer
> 
> Are there any of these patches critical and should be applied before
> release?

None of these are critical. Considering the length of time between grub
releases, the first one might be worth considering. If there is a
non-standard key slot (eg. Someone adds their own KDF) that is before a
standard key slot, we will currently bail and not even check further
keyslots. This can be mitigated by the user making sure that
non-standard keyslots are after standard ones (but that may not
necessarily mean that the key index is larger depending if the json
writer is not sorting keys).

I'm fine with it not being included too. I would guess this use-case is
very improbable.

Glenn