Re: [PATCH] verifiers: Don't return error for deferred image

[Leo Yan]

Hi Boyang,

On Fri, Dec 30, 2022 at 08:56:59PM +0800, Zhang Boyang wrote:

[...]

> > > I'd like to ask what kind of image you are trying to boot/execute? If it 
> > > is
> > > an EFI application, I think you can "chainloader" it and forget U-boot (if
> > > U-boot have no verification in it self).
> > 
> > We want to create a trust of chian:
> > 
> >    firmware -> U-boot -> GRUB -> Linux kernel image / initrd / grub.cfg
> > 
> > Comparing against you mentioned solution which uses shim to verify and
> > load GRUB, we use U-boot to verify and load GRUB.  Therefore, in our
> > implementation we don't use shim verifier for GRUB, and not for later
> > Linux kernel image / initrd / GRUB config files.
> > 
> 
> In my opinion, one problem that shim tries to solve is the so called
> "anti-tivoization" problem in GPLv3 which is used by GRUB2. A user can
> always configure his own shim to allow his own modified GRUB to be executed.
> If your firmware/U-boot doesn't allow user to execute user's modified GRUB,
> there might be a violation of GPLv3. IANAL but I think you might want to
> contact your legal department (if applicable).

Usually law (and license) is about definition and boundary.

Here I am not quite clear what's your meaning for "user", if here "user"
means the developers (or like OEM/OEM, etc), I think this would be fine.
Since we are working on open-sourced solution, which allows developers
to create their own GRUB, in this case, the booting sequence will verify
the signed GRUB, and developers still have chance to update GRUB with
complying with the security mechanism.

If here "user" you mean end users who buy a production, then we still
can allow this user to change GRUB and shim verifier, this would break
the trust of chain and I personally think this is not what we expect.

I am also not a lawyer :)  I will bring up this question internally to
check this aspect, thanks a lot for reminding!

[...]

> > > As a workaround, you can use shim: build shim for you platform and install
> > > GRUB with shim support.
> > > 
> > > You can also submit a fix to --disable-shim-lock (recommended). However it
> > > should be done very carefully. I'm afraid you can't remove the security
> > > measure (i.e. the "verification requested but nobody cares") directly. I
> > > think it is a good idea to add an EFI verifier (like shim verifier), which
> > > use LoadImage() to do verification, and enable it if (and only if)
> > > --disable-shim-lock is specified. You can talk to GRUB maintainers about
> > > your proposal before coding.
> > 
> > Thanks a lot for the suggestion.  As you proposed to add an EFI
> > verifier, this would be a candidate solution we should consider
> > seriously, seems to me, this is a neat way to allow us to hook the
> > verifier in U-boot and GRUB.
> > 
> > However, I found actually we have another choice (though it's not neat
> > but it does work after I did some experiments).  We can enable GRUB's
> > internal verifier 'gpg', thus we can sign grub.cfg / kernel image
> > / initrd with the gpg key and GRUB will use 'gpg' verifier to load
> > these images, please note, we sign these files with detached format so
> > every file has a corresponding signature file with suffix '.sig'.
> > 
> > The tricky thing is the kernel image, since GRUB invokes EFI LoadImage()
> > to load kernel image, and the kernel image has a self-contained
> > signature, so finally the kernel image also will be authenticated by
> > U-boot's EFI service.  This means the kernel image will be
> > authenticated for twice, one is by GRUB's verifier and another is by
> > U-boot's efi_load_image().
> > 
> > At least it's a pragmatic solution for me, I will check internally if we
> > should move forward for using a central place (like U-boot) for all files
> > authentication and it also can simplify the key management.
> > 
> > > The above is what I guess about what's happening. It might be wrong. 
> > > Please
> > > point out if there is something wrong.
> > 
> > I would like to step back a bit and ask a question: if GRUB doesn't
> > enable any crypto verifier and it runs into grub_verifiers_open(), in
> > current code grub_verifiers_open() prevents to load kernel image.  In
> > this case, here I am confused why we cannot proceed to load kernel
> > image?  Or I want to check with maintainers if this patch is still
> > valid :)
> > 
> 
> I think you can modify your patch to only enable that change if
> "--disable-shim-lock" is specified, thus the behavior of verifier framework
> is only changed in this specific situation.

Even if "--disable-shim-lock" is specified, we still can use other
verifier (pgp or tpm), so we need to consider for this case.

> I think another solution is to change lockdown verifier
> (grub-core/kern/lockdown.c), make it not set the
> GRUB_VERIFY_FLAGS_DEFER_AUTH flag if "--disable-shim-lock" is specified, so
> there is no need to change the verifier framework.

This would introduce coupling between lockdown verifier and
"--disable-shim-lock", essentially we want to skip verification if
GRUB has no any verifier rather than lockdown verifier.  How about
below change:

diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
index 75d7994cf..2430da482 100644
--- a/grub-core/kern/verifiers.c
+++ b/grub-core/kern/verifiers.c
@@ -84,6 +84,7 @@ grub_verifiers_open (grub_file_t io, enum grub_file_type type)
   grub_file_t ret = 0;
   grub_err_t err;
   int defer = 0;
+  int verifiers_not_lockdown = 0;
 
   grub_dprintf ("verify", "file: %s type: %d\n", io->name, type);
 
@@ -103,6 +104,11 @@ grub_verifiers_open (grub_file_t io, enum grub_file_type 
type)
       err = ver->init (io, type, &context, &flags);
       if (err)
        goto fail_noclose;
+
+      /* Count verifiers which is not "lockdown" */
+      if (grub_strcmp(ver->name, "lockdown_verifier"))
+        verifiers_not_lockdown++;
+
       if (flags & GRUB_VERIFY_FLAGS_DEFER_AUTH)
        {
          defer = 1;
@@ -114,7 +120,7 @@ grub_verifiers_open (grub_file_t io, enum grub_file_type 
type)
 
   if (!ver)
     {
-      if (defer)
+      if (verifiers_not_lockdown && defer)
        {
          grub_error (GRUB_ERR_ACCESS_DENIED,
                      N_("verification requested but nobody cares: %s"), 
io->name);

> Nevertheless, both solution introduce fundamental changes, so I think you
> might want to write to maintainers to ask their opinion. :)

Exactly!  It would be appreciate if any maintainers could review above
change and let me know if this is right way to move down.

Thank you for suggestions!

Leo