Bug#906124: Additional debug info

[Colin Watson]

[CCing Mathieu, since this will probably interact with Ubuntu's changes
to how SB works, and has some thoughts on future direction.]

On Mon, Jul 08, 2019 at 09:15:49PM +0300, Vladislav Yarmak wrote:
> On Mon, 8 Jul 2019 14:57:08 +0100 Colin Watson <cjwatson at debian.org>
> wrote:
> > I'm not aware of anyone working on it at the moment.  I won't directly
> > revert the patch that introduced this problem because doing so would
> > have too much other fallout, but I'd be happy to help you if you're
> > interested in working on a patch to make GRUB behave differently in
> > the presence of check_signatures while preserving the current default
> > workflow.
> 
> Thanks!
> 
> Alright, here new version of linuxefi_disable_sb_fallback.patch
> attached, which does what was discussed here.

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.

> May I hope it'll be included into Debian updates?

I'm not sure I'm comfortable adding this to Debian 10 updates, at least
not in the short term, but something like it could probably go into
unstable and we can see how it goes.  It'll depend on the details of the
discussion below.

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.

> @@ -720,7 +718,16 @@ grub_cmd_linux (grub_command_t cmd __att
>  		  return GRUB_ERR_NONE;
>  		}
>  	      grub_dprintf ("linux", "linuxefi failed (%d)\n", grub_errno);
> -	      grub_errno = GRUB_ERR_NONE;
> +	      /* Preserve default workflow if verify module is loaded and
> +	         signatures are being checked. Condition below is even with
> +	         code which parses "check_signatures" variable in verify.c */
> +	      const char *env_chk_sig = grub_env_get ("check_signatures");
> +	      if (env_chk_sig &&
> +	      (env_chk_sig[0] == '1' || env_chk_sig[0] == 'e') &&
> +	      grub_dl_get("verify"))
> +	        grub_errno = GRUB_ERR_NONE;
> +	      else
> +	        goto fail;
>  	    }
>  	}
>      }

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

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.

In fact, perhaps part of the solution could be to integrate the shim
checking with the verifiers framework, and then linuxefi would (if
anything) just need to check that some appropriate verifier has been
run, rather than the current code that predates verifiers and does the
check by hand.  This would make much more logical sense: rather than
scattering verification logic around all over the place, it would all be
neatly encapsulated in verifiers.  Doing this would probably be part of
a useful path to getting shim verification upstream, too.  And, even if
we do end up backporting this to 2.02 which doesn't have verifiers, it's
always easier to unroll a well-encapsulated approach into a series of
ad-hoc checks than the other way round.

So, while the approach above is fatally flawed (at least on 2.04), I
think there are some other ways this could be made to work.

Thanks,

-- 
Colin Watson                                       [cjwatson at debian.org]