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

[Patrick Steinhardt]

On Tue, Apr 14, 2020 at 08:12:22PM +0200, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 06:02:23PM +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.
> > ---
> >  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..58ac7bae1 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, "luks2", "Read error: %s\n", grub_errmsg);
> 
> I think that you should drop "luks2" here.

Oops, yes, obviously.

> >        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);
> 
> This messages will be printed only if debugging is enabled. Is it what
> you expect?

Well, I'm honestly not quite sure. The thing is that we potentially have
a number of different keyslots, not only one. So when we try keyslots in
turn, it's not unexpected for the first n-1 keyslots to not work if the
nth keyslot is the one we're trying to unlock.

The above is probably just an edge case as I expect most users to use a
single keyslot, only. And in that case it would probably we useful to
always print those messages, not only in case debugging is turned on.

What do you think?

Patrick

signature.asc
Description: PGP signature