[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 <cjwat...@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 [cjwat...@debian.org]
