[PATCH] luks2: Improve error reporting when decrypting/verifying key

[PATCH] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

While we already set up error messages in both `luks2_verify_key()` and
`luks2_decrypt_key()`, we do not ever print them. This makes it really
hard to discover why a given key actually failed to decrypt a disk.

Improve this by including the error message in the user-visible output.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 65c4f0aac..a48bddf5d 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
   if (ret)
     {
-      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
+      grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
       goto err;
     }
 
@@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
+	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
       ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
+	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
-- 
2.26.1


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

Re: [PATCH] luks2: Improve error reporting when decrypting/verifying key

[Daniel Kiper]

On Thu, Apr 16, 2020 at 12:19:55PM +0200, Patrick Steinhardt wrote:
> While we already set up error messages in both `luks2_verify_key()` and
> `luks2_decrypt_key()`, we do not ever print them. This makes it really
> hard to discover why a given key actually failed to decrypt a disk.
>
> Improve this by including the error message in the user-visible output.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/disk/luks2.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 65c4f0aac..a48bddf5d 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
>    if (ret)
>      {
> -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> +      grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
>        goto err;
>      }

AIUI the commit message says about this change but...

> @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
>  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
>        if (ret)
>  	{
> -	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
> +	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
> +			i, grub_errmsg);
>  	  continue;
>  	}
>
>        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
>        if (ret)
>  	{
> -	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
> +	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
> +			i, grub_errmsg);
>  	  continue;

...it does not say anything about these changes. If you update commit
message you can add Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

On Thu, Apr 16, 2020 at 02:27:02PM +0200, Daniel Kiper wrote:
> On Thu, Apr 16, 2020 at 12:19:55PM +0200, Patrick Steinhardt wrote:
> > While we already set up error messages in both `luks2_verify_key()` and
> > `luks2_decrypt_key()`, we do not ever print them. This makes it really
> > hard to discover why a given key actually failed to decrypt a disk.
> >
> > Improve this by including the error message in the user-visible output.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  grub-core/disk/luks2.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 65c4f0aac..a48bddf5d 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
> >    if (ret)
> >      {
> > -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> > +      grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
> >        goto err;
> >      }
> 
> AIUI the commit message says about this change but...
> 
> > @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
> >  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
> >        if (ret)
> >  	{
> > -	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
> > +	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
> > +			i, grub_errmsg);
> >  	  continue;
> >  	}
> >
> >        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
> >        if (ret)
> >  	{
> > -	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
> > +	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
> > +			i, grub_errmsg);
> >  	  continue;
> 
> ...it does not say anything about these changes. If you update commit
> message you can add Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> Daniel

Does the following commit message clear things up?

    luks2: Improve error reporting when recovering keys

    While we already set up error messages in both `luks2_verify_key()` and
    `luks2_decrypt_key()`, we do not ever print them in the calling function
    `luks2_recover_key()`. This makes it really hard to discover why a given
    key actually failed to decrypt a disk.

    Improve this by including the error message in the user-visible output.
    While at it, fix one error path in `luks2_decrypt_key()` that printed
    the error directly instead of returning it.

    Signed-off-by: Patrick Steinhardt <ps@pks.im>

Patrick

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

Re: [PATCH] luks2: Improve error reporting when decrypting/verifying key

[Daniel Kiper]

On Thu, Apr 16, 2020 at 02:36:10PM +0200, Patrick Steinhardt wrote:
> On Thu, Apr 16, 2020 at 02:27:02PM +0200, Daniel Kiper wrote:
> > On Thu, Apr 16, 2020 at 12:19:55PM +0200, Patrick Steinhardt wrote:
> > > While we already set up error messages in both `luks2_verify_key()` and
> > > `luks2_decrypt_key()`, we do not ever print them. This makes it really
> > > hard to discover why a given key actually failed to decrypt a disk.
> > >
> > > Improve this by including the error message in the user-visible output.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > >  grub-core/disk/luks2.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > index 65c4f0aac..a48bddf5d 100644
> > > --- a/grub-core/disk/luks2.c
> > > +++ b/grub-core/disk/luks2.c
> > > @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > >    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
> > >    if (ret)
> > >      {
> > > -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> > > +      grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
> > >        goto err;
> > >      }
> >
> > AIUI the commit message says about this change but...
> >
> > > @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
> > >  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
> > >        if (ret)
> > >  	{
> > > -	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
> > > +	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
> > > +			i, grub_errmsg);
> > >  	  continue;
> > >  	}
> > >
> > >        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
> > >        if (ret)
> > >  	{
> > > -	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
> > > +	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
> > > +			i, grub_errmsg);
> > >  	  continue;
> >
> > ...it does not say anything about these changes. If you update commit
> > message you can add Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >
> > Daniel
>
> Does the following commit message clear things up?
>
>     luks2: Improve error reporting when recovering keys
>
>     While we already set up error messages in both `luks2_verify_key()` and
>     `luks2_decrypt_key()`, we do not ever print them in the calling function
>     `luks2_recover_key()`. This makes it really hard to discover why a given
>     key actually failed to decrypt a disk.
>
>     Improve this by including the error message in the user-visible output.
>     While at it, fix one error path in `luks2_decrypt_key()` that printed
>     the error directly instead of returning it.
>
>     Signed-off-by: Patrick Steinhardt <ps@pks.im>

Much better. However, after seeing this I think that this patch should
be split into two separate ones. If you do that and split the commit
message accordingly feel free to add Reviewed-by: Daniel Kiper
<daniel.kiper@oracle.com>.

Daniel

[PATCH v2 0/2] Improve LUKS2 error reporting

[Patrick Steinhardt]

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

Hi,

this is the second version of my patch that has the aim of improving
error reporting for LUKS2 in case decryption of the disk fails. There's
no functional changes, but I've split up the single patch into two as
proposed by Daniel.

@Daniel: Note that I've added your Reviewed-by to the second patch, but
not to the first one as it's message is completely new.

Patrick

Patrick Steinhardt (2):
  luks2: Propagate error when reading area key fails
  luks2: Improve error reporting when recovering keys

 grub-core/disk/luks2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.26.1


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

[PATCH v2 1/2] luks2: Propagate error when reading area key fails

[Patrick Steinhardt]

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

When decrypting a given keyslot, all error cases except for one set up
an error and return the error code. The only exception is when we try to
read the area key: instead of setting up an error message, we directly
print it via `grub_dprintf()`.

Convert the outlier to use `grub_error()` to allow more uniform handling
of errors.

Signed-off-by: Patrick Steinhardt <ps@kps.im>
---
 grub-core/disk/luks2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 65c4f0aac..e3ff7c83d 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
   if (ret)
     {
-      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
+      grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
       goto err;
     }
 
-- 
2.26.1


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

[PATCH v2 2/2] luks2: Improve error reporting when recovering keys

[Patrick Steinhardt]

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

While we already set up error messages in both `luks2_verify_key()` and
`luks2_decrypt_key()`, we do not ever print them in the calling function
`luks2_recover_key()`. This makes it really hard to discover why a given
key actually failed to decrypt a disk.

Improve this by including the error message in the user-visible output.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/disk/luks2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e3ff7c83d..a48bddf5d 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
+	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
       ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
+	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
-- 
2.26.1


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

Re: [PATCH v2 0/2] Improve LUKS2 error reporting

[Daniel Kiper]

On Thu, Apr 16, 2020 at 07:15:06PM +0200, Patrick Steinhardt wrote:
> Hi,
>
> this is the second version of my patch that has the aim of improving
> error reporting for LUKS2 in case decryption of the disk fails. There's
> no functional changes, but I've split up the single patch into two as
> proposed by Daniel.
>
> @Daniel: Note that I've added your Reviewed-by to the second patch, but
> not to the first one as it's message is completely new.

LGTM, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> for both...

Thanks,

Daniel

Re: [PATCH v2 0/2] Improve LUKS2 error reporting

[Patrick Steinhardt]

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

On Fri, Apr 17, 2020 at 03:54:04PM +0200, Daniel Kiper wrote:
> On Thu, Apr 16, 2020 at 07:15:06PM +0200, Patrick Steinhardt wrote:
> > Hi,
> >
> > this is the second version of my patch that has the aim of improving
> > error reporting for LUKS2 in case decryption of the disk fails. There's
> > no functional changes, but I've split up the single patch into two as
> > proposed by Daniel.
> >
> > @Daniel: Note that I've added your Reviewed-by to the second patch, but
> > not to the first one as it's message is completely new.
> 
> LGTM, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> for both...
> 
> Thanks,
> 
> Daniel

I see you pulled in commit c543d6781 (luks2: Propagate error when
reading area key fails, 2020-04-16), but didn't pull in the second patch
to improve error reporting when recovering keys fails. Was this by
accident or intentionally?

Patrick

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