Bug#906124: Additional debug info

[Vladislav Yarmak]

On Thu, 18 Jul 2019 15:00:44 +0100
Colin Watson <cjwatson at debian.org> wrote:

> This format is difficult to review and apply, not least because later
> versions of the source package have moved on in different ways.  For
> the next version, please just send a patch against
> grub-core/loader/i386/linux.c (etc.) directly that makes your change
> relative to what's currently in the Debian source package.
> Please prepare your patch against the version in unstable (preferably
> https://salsa.debian.org/grub-team/grub), not the version in buster.
> Patches generally flow backwards from unstable rather than forwards
> from stable releases.

I thought it is more preferable to replace one patch with another in
order to keep patch stack size. But placing one more patch on top of
all rest sounds also OK for me.

> This specific approach isn't suitable, I'm afraid.  The copied code is
> too fragile (things could go badly wrong if the check_signatures
> parsing were changed upstream and we forgot to update this patch to
> match). More importantly, this approach encodes an implicit
> assumption that if that environment variable is set then some other
> bit of code is actually set up to consume it and verify the kernel in
> some other way.  This is not robust, because there's nothing to say
> that the pgp module is loaded if the linux module is loaded: it would
> be quite possible to have an image that contains the linux module but
> not the pgp module, and then you have a UEFI Secure Boot bypass just
> by setting check_signatures=1. (Note that the module layout is a bit
> different in 2.04 than in 2.02, which is why it's important to
> prepare this patch against the latest version.)

It is not true because grub_dl_get("verify") in "if" condition checks if
required module was loaded. And before I emailed my patch and said
"tested", I made efforts and *really* tested various cases this
condition should cover, one by one.

Point about code copy is also weak because "verify"/"pgp" module
internally contains this condition code at least in two locations and
sudden change of interpretation for such security variables is a
security risk regardless of anything else.

In fact, even if we export from "verify" module some function which
exposes state of "sec" variable, it still will not guarantee internal
state will be always represented by boolean values of that variable or
those values retain its sense. In some sense, my solution relies on a
public documented interface of that module, while function export
solution relies on internal mechanics of module, requiring additional
hacks to expose it.
 
> I'm not sure exactly how this should look, but I'm pretty sure that
> it's going to be too hard to get this right in
> grub-core/loader/i386/linux.c, and that it needs to live in
> grub-core/loader/i386/efi/linux.c which has more context about what's
> going on.  I think it would help to push the integration with PGP
> signature checking down to grub_linuxefi_secure_validate or somewhere
> near that.  That would allow also taking advantage of the kernel's
> UEFI quirks handling in the case where a kernel has been PGP-signed
> (since we'd be able to enter the kernel without calling
> ExitBootServices), and it would allow for less fragile code layout.

If we agree about condition which checks "verify"/"pgp" module state, I
can modify grub_linuxefi_secure_validate to skip validation when PGP is
active same way it does if secureboot is not enabled at all (there are
already some cases when linuxefi skips validation, so it will be
probably ok to keep in one place).

-- 
Best Regards,
Vladislav Yarmak