Re: [PATCH v3 4/8] verifiers: Add possibility to defer verification to other verifiers

[Daniel Kiper]

On Fri, Oct 05, 2018 at 12:09:43PM -0400, Ross Philipson wrote:
> On 10/03/2018 05:36 AM, Daniel Kiper wrote:
> > This way if a verifier requires verification of a given file it can
> > defer task to other verifier if it is not able to do it itself. E.g.
> > shim_lock verifier, posted as a subsequent patch, is able to verify
> > only PE files. This means that it is not able to verify any of GRUB2
> > modules which have to be trusted on UEFI systems with secure boot
> > enabled. So, it can defer verification to other verifier, e.g. PGP one.
> >
> > I silently assume that other verifiers are trusted and will do good
> > job for us. Or at least they will not do any harm.
> >
> > Signed-off-by: Daniel Kiper <address@hidden>
> > ---
> >  grub-core/commands/verify_helper.c |   23 ++++++++++++++++++++---
> >  include/grub/verify.h              |    3 ++-
> >  2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/grub-core/commands/verify_helper.c 
> > b/grub-core/commands/verify_helper.c
> > index 7effc5f..ba8b03d 100644
> > --- a/grub-core/commands/verify_helper.c
> > +++ b/grub-core/commands/verify_helper.c
> > @@ -83,6 +83,7 @@ grub_verify_helper_open (grub_file_t io, enum 
> > grub_file_type type)
> >    void *context;
> >    grub_file_t ret = 0;
> >    grub_err_t err;
> > +  int defer = 0;
> >
> >    grub_dprintf ("verify", "file: %s type: %d\n", io->name, type);
> >
> > @@ -102,13 +103,27 @@ grub_verify_helper_open (grub_file_t io, enum 
> > grub_file_type type)
> >        err = ver->init (io, type, &context, &flags);
> >        if (err)
> >     goto fail_noclose;
> > +      if (flags & GRUB_VERIFY_FLAGS_DEFER)
> > +   {
> > +     defer = 1;
> > +     continue;
> > +   }
> >        if (!(flags & GRUB_VERIFY_FLAGS_SKIP_VERIFICATION))
> >     break;
> >      }
> >
> >    if (!ver)
> > -    /* No verifiers wanted to verify. Just return underlying file.  */
> > -    return io;
> > +    {
> > +      if (defer)
> > +   {
> > +     grub_error (GRUB_ERR_ACCESS_DENIED,
> > +                 N_("verification requested but nobody cares: %s"), 
> > io->name);
> > +     goto fail_noclose;
> > +   }
>
> I don't really understand the logic here. Is the idea that if it was
> deferred and no one in the chain verified it, it ends up being an error?

Yep.

> Why is that?

Some verifiers may not be able to verify a given file themselves. So, they
may require that it have to be done by somebody else. shim_lock verifier
is good example. The commit message explains it.

> Also defer has different meanings. I think in this context it means
> defer verification to another authority as opposed to defer until later.

Yes, exactly. I will improve the commit message.

> > +      /* No verifiers wanted to verify. Just return underlying file. */
> > +      return io;
> > +    }
> >
> >    ret = grub_malloc (sizeof (*ret));
> >    if (!ret)
> > @@ -160,7 +175,9 @@ grub_verify_helper_open (grub_file_t io, enum 
> > grub_file_type type)
> >        err = ver->init (io, type, &context, &flags);
> >        if (err)
> >     goto fail_noclose;
> > -      if (flags & GRUB_VERIFY_FLAGS_SKIP_VERIFICATION)
> > +      if (flags & GRUB_VERIFY_FLAGS_SKIP_VERIFICATION ||
> > +     /* Verification done earlier. So, we are happy here. */
> > +     flags & GRUB_VERIFY_FLAGS_DEFER)
> >     continue;
> >        err = ver->write (context, verified->buf, ret->size);
> >        if (err)
> > diff --git a/include/grub/verify.h b/include/grub/verify.h
> > index 9f892d8..c385e3d 100644
> > --- a/include/grub/verify.h
> > +++ b/include/grub/verify.h
> > @@ -22,7 +22,8 @@
> >  enum grub_verify_flags
> >    {
> >      GRUB_VERIFY_FLAGS_SKIP_VERIFICATION    = 1,
> > -    GRUB_VERIFY_FLAGS_SINGLE_CHUNK = 2
> > +    GRUB_VERIFY_FLAGS_SINGLE_CHUNK = 2,
> > +    GRUB_VERIFY_FLAGS_DEFER                = 4
>
> What is the difference between skip verification and defer?

Skip == I do not care here. Defer == "defer verification to another authority".

Daniel