grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Patrick Steinhardt
Subject: Re: [PATCH 1/2] luks2: Improve error reporting when decrypting/verifying key
Date: Thu, 16 Apr 2020 12:03:16 +0200

On Thu, Apr 16, 2020 at 11:52:17AM +0200, Daniel Kiper wrote:
> On Wed, Apr 15, 2020 at 10:52:53PM +0200, Patrick Steinhardt wrote:
> > 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?
> 
> Should not we print an error unconditionally if all n slots failed?
> Then user will know that decryption is not possible but will not be
> bothered by subsequent keyslot failures. However, I would still print
> error for each keyslot if debugging is enabled. Then if you have some
> problems you can enable debugging and see which keyslots fail. Does it
> make sense?
> 
> Daniel

Okay, so that's exactly how it works right now: in case no slot
succeeds, we print "Invalid passphrase" to let the user know about it.
If he turns on debugging, he'll see why each of the individual keyslots
has failed, and this patch only increases the verbosity to actually
include useful information.

I'll resend this patch separate from the jsmn updates with the SOB and
the above fix for `grub_error()`.

Patrick

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]