diff mbox series

verifiers: Don't return error for deferred image

Message ID 20221222111439.2653118-1-leo.yan@linaro.org
State New
Headers show
Series verifiers: Don't return error for deferred image | expand

Commit Message

Leo Yan Dec. 22, 2022, 11:14 a.m. UTC
When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
grub returns error:

 Booting a command list

 error: verification requested but nobody cares: (hd0,gpt1)/Image.

 Press any key to continue...

In this case, the image should be deferred for authentication, grub
should return the file handle and pass down to later firmware (e.g.
U-Boot, etc) for authentication.

For this purpose, rather than returning error, this patch prints log
and returns file handler.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 grub-core/kern/verifiers.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Zhang Boyang Dec. 22, 2022, 11:25 a.m. UTC | #1
Hi,

On 2022/12/22 19:14, Leo Yan wrote:
> When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
> grub returns error:
> 
>   Booting a command list
> 
>   error: verification requested but nobody cares: (hd0,gpt1)/Image.
> 
>   Press any key to continue...
> 
> In this case, the image should be deferred for authentication, grub
> should return the file handle and pass down to later firmware (e.g.
> U-Boot, etc) for authentication.

This is probably not what verification framework designed to be. It 
seems to be designed to verify files during GRUB is executing (e.g. 
check file signature if UEFI Secure Boot is enabled).

By the way, I didn't understand what does "return the file handle and 
pass down to later firmware" means. If you means you want GRUB call into 
firmware's function, you can write a verifier to do that and register 
your verifier with grub_verifier_register().

Best Regards,
Zhang Boyang

> 
> For this purpose, rather than returning error, this patch prints log
> and returns file handler.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   grub-core/kern/verifiers.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
> index 75d7994cf..ada753e69 100644
> --- a/grub-core/kern/verifiers.c
> +++ b/grub-core/kern/verifiers.c
> @@ -115,11 +115,7 @@ grub_verifiers_open (grub_file_t io, enum grub_file_type type)
>     if (!ver)
>       {
>         if (defer)
> -	{
> -	  grub_error (GRUB_ERR_ACCESS_DENIED,
> -		      N_("verification requested but nobody cares: %s"), io->name);
> -	  goto fail_noclose;
> -	}
> +	grub_printf("%s verification is deferred\n", io->name);
>   
>         /* No verifiers wanted to verify. Just return underlying file. */
>         return io;
Leo Yan Dec. 22, 2022, 12:22 p.m. UTC | #2
Hi Boyang,

On Thu, Dec 22, 2022 at 07:25:13PM +0800, Zhang Boyang wrote:
> Hi,
> 
> On 2022/12/22 19:14, Leo Yan wrote:
> > When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
> > grub returns error:
> > 
> >   Booting a command list
> > 
> >   error: verification requested but nobody cares: (hd0,gpt1)/Image.
> > 
> >   Press any key to continue...
> > 
> > In this case, the image should be deferred for authentication, grub
> > should return the file handle and pass down to later firmware (e.g.
> > U-Boot, etc) for authentication.
> 
> This is probably not what verification framework designed to be. It seems to
> be designed to verify files during GRUB is executing (e.g. check file
> signature if UEFI Secure Boot is enabled).

Good point.  We expect the solution is grub can defer authentication for
an image and invokes EFI LoadImage service, then EFI loader can load
and verify the image.

For more specific, now I am debugging U-boot EFI with grub, since U-boot
EFI provides functionality for loading and authentication image (see
efi_load_image() in [1]), this is my purpose to use U-boot EFI to
authenticate kernel image (and even for initrd image).

> By the way, I didn't understand what does "return the file handle and pass
> down to later firmware" means. If you means you want GRUB call into
> firmware's function, you can write a verifier to do that and register your
> verifier with grub_verifier_register().

To be clear, I am not experienced for EFI and grub, I try my best to
give info :)

As explained above, we don't want to introduce any new verifier in
grub, it's about we want to verify image in U-boot EFT rather than in
grub.  So this is why I wrote this patch to dimiss the failure in grub
and pass image info to U-boot EFI service.  (and sorry my commit log
introduced confusion).

Thanks,
Leo

[1] https://github.com/u-boot/u-boot/blob/master/lib/efi_loader/efi_boottime.c#L2021
Zhang Boyang Dec. 22, 2022, 2:37 p.m. UTC | #3
Hi,

On 2022/12/22 20:22, Leo Yan wrote:
> Hi Boyang,
> 
> On Thu, Dec 22, 2022 at 07:25:13PM +0800, Zhang Boyang wrote:
>> Hi,
>>
>> On 2022/12/22 19:14, Leo Yan wrote:
>>> When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
>>> grub returns error:
>>>
>>>    Booting a command list
>>>
>>>    error: verification requested but nobody cares: (hd0,gpt1)/Image.
>>>
>>>    Press any key to continue...
>>>
>>> In this case, the image should be deferred for authentication, grub
>>> should return the file handle and pass down to later firmware (e.g.
>>> U-Boot, etc) for authentication.
>>
>> This is probably not what verification framework designed to be. It seems to
>> be designed to verify files during GRUB is executing (e.g. check file
>> signature if UEFI Secure Boot is enabled).
> 
> Good point.  We expect the solution is grub can defer authentication for
> an image and invokes EFI LoadImage service, then EFI loader can load
> and verify the image.
> 

Since you mentioned "authentication" and "verify", I guess you are 
using/implementing some kind of secure boot mechanism. Is it an standard 
UEFI Secure Boot implementation?

The standard UEFI Secure Boot for Linux works like this (at least on x86):

1) Firmware verifies and loads shim (shimx64.efi), which is signed by 
Microsoft. (https://github.com/rhboot/shim )

2) shim registers an EFI protocol, to provide an API for verifying files.

3) shim verifies and loads GRUB (grubx64.efi), using certificate 
embedded in itself. The certificate is generated by vendor (e.g. Debian).

4) GRUB opens kernel image file and loads and executes that file. The 
key point is, during grub_file_open() of the kernel image, the verifier 
framework will call shim lock verifier, which calls the API provided by 
shim, to verify the signature of kernel file. (grub-core/kern/efi/sb.c)

In the above example, the kernel is loaded by GRUB, not shim, not EFI 
firmware. GRUB calls the API provided by shim to verify the kernel.

Another example is GRUB can also chainload Windows:

1) 2) 3) is same

4) GRUB invokes EFI firmware's LoadFile() and StartImage(). The 
verification is completely delegated to EFI firmware. shim is not envolved.

In this example, the verification is done by EFI firmware, and GRUB/shim 
has no control of it. You can't even chainload GRUB itself because it's 
not signed by Microsoft.


> For more specific, now I am debugging U-boot EFI with grub, since U-boot
> EFI provides functionality for loading and authentication image (see
> efi_load_image() in [1]), this is my purpose to use U-boot EFI to
> authenticate kernel image (and even for initrd image).
> 

It seems efi_load_image() is just a wrapper of EFI firmware's LoadFile() 
and there is no implementation of verification in U-boot?

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).

Let me guess: You are using standard Secure Boot, and want to boot linux 
kernel image directly signed by keys in EFI firmware, and your GRUB is 
installed with --disable-shim-lock. If this is what you are doing, you 
might run into the following situation:

1) During GRUB init, grub_efi_init() detects Secure Boot is enabled, so 
it calls grub_lockdown(). [grub-core/kern/efi/init.c]

2) grub_lockdown() registers lockdown verifier. [grub-core/kern/lockdown.c]

3) grub_efi_init() also calls grub_shim_lock_verifier_setup(). However, 
grub_shim_lock_verifier_setup() does nothing because of 
"--disable-shim-lock" [grub-core/kern/efi/sb.c]

4) During GRUB load linux kernel, the lockdown verifier sets a flag 
(GRUB_VERIFY_FLAGS_DEFER_AUTH) for kernel file, and expect shim lock 
verifier to do the verification.

5) However, there is no shim lock verifier. As a security measure, it 
reports the error "verification requested but nobody cares".

So the root cause seems like --disable-shim-lock is broken?

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.

The above is what I guess about what's happening. It might be wrong. 
Please point out if there is something wrong.

>> By the way, I didn't understand what does "return the file handle and pass
>> down to later firmware" means. If you means you want GRUB call into
>> firmware's function, you can write a verifier to do that and register your
>> verifier with grub_verifier_register().
> 
> To be clear, I am not experienced for EFI and grub, I try my best to
> give info :)

Me too :)

Best Regards,
Zhang Boyang

> 
> As explained above, we don't want to introduce any new verifier in
> grub, it's about we want to verify image in U-boot EFT rather than in
> grub.  So this is why I wrote this patch to dimiss the failure in grub
> and pass image info to U-boot EFI service.  (and sorry my commit log
> introduced confusion).
> 
> Thanks,
> Leo
> 
> [1] https://github.com/u-boot/u-boot/blob/master/lib/efi_loader/efi_boottime.c#L2021
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
Leo Yan Dec. 26, 2022, 8:13 a.m. UTC | #4
Hi Boyang,

On Thu, Dec 22, 2022 at 10:37:01PM +0800, Zhang Boyang wrote:

[...]

> > > On 2022/12/22 19:14, Leo Yan wrote:
> > > > When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
> > > > grub returns error:
> > > > 
> > > >    Booting a command list
> > > > 
> > > >    error: verification requested but nobody cares: (hd0,gpt1)/Image.
> > > > 
> > > >    Press any key to continue...
> > > > 
> > > > In this case, the image should be deferred for authentication, grub
> > > > should return the file handle and pass down to later firmware (e.g.
> > > > U-Boot, etc) for authentication.
> > > 
> > > This is probably not what verification framework designed to be. It seems to
> > > be designed to verify files during GRUB is executing (e.g. check file
> > > signature if UEFI Secure Boot is enabled).
> > 
> > Good point.  We expect the solution is grub can defer authentication for
> > an image and invokes EFI LoadImage service, then EFI loader can load
> > and verify the image.
> 
> Since you mentioned "authentication" and "verify", I guess you are
> using/implementing some kind of secure boot mechanism. Is it an standard
> UEFI Secure Boot implementation?

I don't read specification for standard UEFI secure boot, but from my
very limited understanding, I don't think now I am using a standard
UEFI secure boot sequence.

Anyway, I will explain more in below comments.

> The standard UEFI Secure Boot for Linux works like this (at least on x86):
> 
> 1) Firmware verifies and loads shim (shimx64.efi), which is signed by
> Microsoft. (https://github.com/rhboot/shim )
> 
> 2) shim registers an EFI protocol, to provide an API for verifying files.
> 
> 3) shim verifies and loads GRUB (grubx64.efi), using certificate embedded in
> itself. The certificate is generated by vendor (e.g. Debian).
> 
> 4) GRUB opens kernel image file and loads and executes that file. The key
> point is, during grub_file_open() of the kernel image, the verifier
> framework will call shim lock verifier, which calls the API provided by
> shim, to verify the signature of kernel file. (grub-core/kern/efi/sb.c)
> 
> In the above example, the kernel is loaded by GRUB, not shim, not EFI
> firmware. GRUB calls the API provided by shim to verify the kernel.
> 
> Another example is GRUB can also chainload Windows:
> 
> 1) 2) 3) is same
> 
> 4) GRUB invokes EFI firmware's LoadFile() and StartImage(). The verification
> is completely delegated to EFI firmware. shim is not envolved.
> 
> In this example, the verification is done by EFI firmware, and GRUB/shim has
> no control of it. You can't even chainload GRUB itself because it's not
> signed by Microsoft.

Very good summary, thanks a lot!

> > For more specific, now I am debugging U-boot EFI with grub, since U-boot
> > EFI provides functionality for loading and authentication image (see
> > efi_load_image() in [1]), this is my purpose to use U-boot EFI to
> > authenticate kernel image (and even for initrd image).
> > 
> 
> It seems efi_load_image() is just a wrapper of EFI firmware's LoadFile() and
> there is no implementation of verification in U-boot?

This isn't right.  U-boot EFI service verifies image with below flow:

  efi_load_image()
    `> efi_load_pe()
         `> efi_image_authenticate()

> 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.

> Let me guess: You are using standard Secure Boot, and want to boot linux
> kernel image directly signed by keys in EFI firmware, and your GRUB is
> installed with --disable-shim-lock. If this is what you are doing, you might
> run into the following situation:
> 
> 1) During GRUB init, grub_efi_init() detects Secure Boot is enabled, so it
> calls grub_lockdown(). [grub-core/kern/efi/init.c]
> 
> 2) grub_lockdown() registers lockdown verifier. [grub-core/kern/lockdown.c]
> 
> 3) grub_efi_init() also calls grub_shim_lock_verifier_setup(). However,
> grub_shim_lock_verifier_setup() does nothing because of
> "--disable-shim-lock" [grub-core/kern/efi/sb.c]
> 
> 4) During GRUB load linux kernel, the lockdown verifier sets a flag
> (GRUB_VERIFY_FLAGS_DEFER_AUTH) for kernel file, and expect shim lock
> verifier to do the verification.
> 
> 5) However, there is no shim lock verifier. As a security measure, it
> reports the error "verification requested but nobody cares".
> 
> So the root cause seems like --disable-shim-lock is broken?

Excellent analysis, and it's exactly an issue I am facing.

> 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 :)

Very appreciate your insight comments and suggestions!

Leo
Zhang Boyang Dec. 30, 2022, 12:56 p.m. UTC | #5
Hi,

On 2022/12/26 16:13, Leo Yan wrote:
> Hi Boyang,
> 
> On Thu, Dec 22, 2022 at 10:37:01PM +0800, Zhang Boyang wrote:
> 
> [...]
> 
>>>> On 2022/12/22 19:14, Leo Yan wrote:
>>>>> When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
>>>>> grub returns error:
>>>>>
>>>>>     Booting a command list
>>>>>
>>>>>     error: verification requested but nobody cares: (hd0,gpt1)/Image.
>>>>>
>>>>>     Press any key to continue...
>>>>>
>>>>> In this case, the image should be deferred for authentication, grub
>>>>> should return the file handle and pass down to later firmware (e.g.
>>>>> U-Boot, etc) for authentication.
>>>>
>>>> This is probably not what verification framework designed to be. It seems to
>>>> be designed to verify files during GRUB is executing (e.g. check file
>>>> signature if UEFI Secure Boot is enabled).
>>>
>>> Good point.  We expect the solution is grub can defer authentication for
>>> an image and invokes EFI LoadImage service, then EFI loader can load
>>> and verify the image.
>>
>> Since you mentioned "authentication" and "verify", I guess you are
>> using/implementing some kind of secure boot mechanism. Is it an standard
>> UEFI Secure Boot implementation?
> 
> I don't read specification for standard UEFI secure boot, but from my
> very limited understanding, I don't think now I am using a standard
> UEFI secure boot sequence.
> 
> Anyway, I will explain more in below comments.
> 
>> The standard UEFI Secure Boot for Linux works like this (at least on x86):
>>
>> 1) Firmware verifies and loads shim (shimx64.efi), which is signed by
>> Microsoft. (https://github.com/rhboot/shim )
>>
>> 2) shim registers an EFI protocol, to provide an API for verifying files.
>>
>> 3) shim verifies and loads GRUB (grubx64.efi), using certificate embedded in
>> itself. The certificate is generated by vendor (e.g. Debian).
>>
>> 4) GRUB opens kernel image file and loads and executes that file. The key
>> point is, during grub_file_open() of the kernel image, the verifier
>> framework will call shim lock verifier, which calls the API provided by
>> shim, to verify the signature of kernel file. (grub-core/kern/efi/sb.c)
>>
>> In the above example, the kernel is loaded by GRUB, not shim, not EFI
>> firmware. GRUB calls the API provided by shim to verify the kernel.
>>
>> Another example is GRUB can also chainload Windows:
>>
>> 1) 2) 3) is same
>>
>> 4) GRUB invokes EFI firmware's LoadFile() and StartImage(). The verification
>> is completely delegated to EFI firmware. shim is not envolved.
>>
>> In this example, the verification is done by EFI firmware, and GRUB/shim has
>> no control of it. You can't even chainload GRUB itself because it's not
>> signed by Microsoft.
> 
> Very good summary, thanks a lot!
> 
>>> For more specific, now I am debugging U-boot EFI with grub, since U-boot
>>> EFI provides functionality for loading and authentication image (see
>>> efi_load_image() in [1]), this is my purpose to use U-boot EFI to
>>> authenticate kernel image (and even for initrd image).
>>>
>>
>> It seems efi_load_image() is just a wrapper of EFI firmware's LoadFile() and
>> there is no implementation of verification in U-boot?
> 
> This isn't right.  U-boot EFI service verifies image with below flow:
> 
>    efi_load_image()
>      `> efi_load_pe()
>           `> efi_image_authenticate()
> 
>> 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).

>> Let me guess: You are using standard Secure Boot, and want to boot linux
>> kernel image directly signed by keys in EFI firmware, and your GRUB is
>> installed with --disable-shim-lock. If this is what you are doing, you might
>> run into the following situation:
>>
>> 1) During GRUB init, grub_efi_init() detects Secure Boot is enabled, so it
>> calls grub_lockdown(). [grub-core/kern/efi/init.c]
>>
>> 2) grub_lockdown() registers lockdown verifier. [grub-core/kern/lockdown.c]
>>
>> 3) grub_efi_init() also calls grub_shim_lock_verifier_setup(). However,
>> grub_shim_lock_verifier_setup() does nothing because of
>> "--disable-shim-lock" [grub-core/kern/efi/sb.c]
>>
>> 4) During GRUB load linux kernel, the lockdown verifier sets a flag
>> (GRUB_VERIFY_FLAGS_DEFER_AUTH) for kernel file, and expect shim lock
>> verifier to do the verification.
>>
>> 5) However, there is no shim lock verifier. As a security measure, it
>> reports the error "verification requested but nobody cares".
>>
>> So the root cause seems like --disable-shim-lock is broken?
> 
> Excellent analysis, and it's exactly an issue I am facing.
> 

Then I think the root cause is clear now. The problem now comes to 
solution. :)

>> 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.

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.

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

Best Regards,
Zhang Boyang

> Very appreciate your insight comments and suggestions!
> 
> Leo
Leo Yan Jan. 6, 2023, 5:11 a.m. UTC | #6
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
diff mbox series

Patch

diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
index 75d7994cf..ada753e69 100644
--- a/grub-core/kern/verifiers.c
+++ b/grub-core/kern/verifiers.c
@@ -115,11 +115,7 @@  grub_verifiers_open (grub_file_t io, enum grub_file_type type)
   if (!ver)
     {
       if (defer)
-	{
-	  grub_error (GRUB_ERR_ACCESS_DENIED,
-		      N_("verification requested but nobody cares: %s"), io->name);
-	  goto fail_noclose;
-	}
+	grub_printf("%s verification is deferred\n", io->name);
 
       /* No verifiers wanted to verify. Just return underlying file. */
       return io;