grub support [ready for review, not for merge] #96

GovanifY · 2023-02-03T18:37:20Z

Ready for review, see #96 (comment) for current concerns

~~This is more of an RFC than anything else to discuss of the design decisions of lanzaboote and know where to go from here.~~

The way this PR works is it implements a new CLI option to lanzatool to specify a EFI binary to install, instead of systemd-boot.
It is then specified by the variant option of the nixos module, which is then generated by a new package/derivation, grub-efi-binary, in order to have a static reproducible binary.

Now the two main issues: First, GRUB cannot generate menus at runtime by giving it a folder the way systemd-boot does.
AFAIK you need to generate a config file AOT which does include a list of the current generations. This leads to two issues:

We cannot load an external configuration file separate from the binary that we regenerate without adding a new signature mechanism integrated in GRUB, GPG based and all the like because of SB, and thus:

We need to either modify the UKI for each generation or add an external configuration that is signed or use UKI names that are reproducible, along with a default configuration.

I'm not sure whether that's relevant yet as we do not have a custom configuration possible for systemd-boot but it's definitely something to keep in mind..

Fixed by including the TPM module, this seems to be a bug of 2.06 requiring the TPM verifier in order to call the proper UEFI functions:

This leads to the current second main issue: shim. GRUB, in theory, can work on either user specified keys or shim. If you don't want shim support, e.g. to use your own keys, you can disable it at runtime using a CLI flag.
Disabling it leads to this fancy error message:

vm-test-run-unsigned-initrd-do-not-boot-under-secureboot> machine # error: verification requested but nobody cares: (hd0,gpt1)/efi/Linux/nixos-generation-1.efi. vm-test-run-unsigned-initrd-do-not-boot-under-secureboot> machine # error: you need to load the kernel first.

Which, well, urgh. In theory GRUB should use the EFI provided verification mechanism, but doesn't even try to call it, which seems like a bug to me. linux doesn't work either, but GRUB does seem to have a mechanism to use LoadImage etc to load the binary, as seen here:

https://github.com/rhboot/grub2/blob/4e9020a937a30873fa63ba34e16c1e6fb7e7b718/grub-core/loader/efi/linux.c#L161

which is a part of the linux grub CLI:

https://github.com/rhboot/grub2/blob/a048b2280ae6a2cf90fd5d2960823843e17e4d66/grub-core/loader/i386/efi/linux.c#L552

...and is simply not called :)

I probably need to debug the current GRUB version we have to figure out what goes wrong but I'm just too tired to actually try to do it right now, so it's probably a better idea to draft this PR and see what people thinks of this way and what seems terrible and what doesn't! Plus, someone other than me might know the answer.

Sorry for this wall of text, and thank for reviewing !

This closes #29

cc @RaitoBezarius

GovanifY · 2023-02-04T16:33:54Z

grub now boots! Although lanzaboote panics

vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine: QEMU running (pid 8)
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine: must succeed: bootctl status
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine: waiting for the VM to finish booting
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine # BdsDxe: loading Boot0001 "UEFI Misc Device" from PciRoot(0x0)/Pci(0x8,0x0)
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine # BdsDxe: starting Boot0001 "UEFI Misc Device" from PciRoot(0x0)/Pci(0x8,0x0)
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine # Welcome to GRUB!
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine # /EndEntire
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine # file path: /ACPI(a0341d0,0)/PCI(0,8)/HD(2,7a000,279800,3b5122f2d1defa49,2,2)/File(\efi\Linux)
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine # /File(nixos-generation-1.efi)/EndEntire
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #   _                      _                 _
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #  | |                    | |               | |
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #  | | __ _ _ __  ______ _| |__   ___   ___ | |_ ___
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #  | |/ _` | '_ \|_  / _` | '_ \ / _ \ / _ \| __/ _ \
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #  | | (_| | | | |/ / (_| | |_) | (_) | (_) | ||  __/
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #  |_|\__,_|_| |_/___\__,_|_.__/ \___/ \___/ \__\___|
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine #
vm-test-run-signed-files-boot-with-secrets-under-secureboot> machine # [PANIC]: panicked at 'called `Result::unwrap()` on an `Err` value: Error { status: NOT_FOUND, data: () }', src/main.rs:107:89

GovanifY · 2023-02-07T11:07:32Z

I disabled the systemd version check during the rebasing as we need some way to infer whether a binary is systemd and that it did not work at all on my system, even with systemd:

Error: Failed to read systemd-boot version from "/nix/store/9rjdvhq7hnzwwhib8na2gmllsrh671xg-systemd-252.1/lib/systemd/boot/efi/linuxx64.efi.stub".

Caused by:
    PE section '.osrel' is empty: "/nix/store/9rjdvhq7hnzwwhib8na2gmllsrh671xg-systemd-252.1/lib/systemd/boot/efi/linuxx64.efi.stub"

make the tests run with a grub efi binary

Remove the systemd arg as it is redundant with efi_boot. The version check is currently commented out as it cannot work in all situations, we need to figure out something for this!

GovanifY · 2023-02-07T11:41:28Z

Not ready to be merged, but marking it as ready for review so that people can look at this PR and discuss it.

The main sticking points so far are:

The systemd specific bits (e.g. systemd version, config, etc) need to be redesigned and/or removed for other bootloaders. What to do here? Semi-related but systemd's version checker can return an empty .osrel on an unmodified systemd binary.
The issue is grub support [ready for review, not for merge] #96 (comment) GRUB currently panics at lanzaboote because Boots Services do not return expected handles . What to do here? Patch (in a pretty dirty fashion) GRUB, change the boot logic? Not sure what the best course of action here is...
Currently a pre-generated config is built in the GRUB EFI binary. Do we want to be able to load an external signed config, using GRUB's own signature scheme so we don't have to update the bootloader each time there is a config change? This is probably a longer term goal and I think it would make sense to first have config built-in and then add this on later as a feature.

GovanifY · 2023-02-07T14:01:15Z

I added a patchset implementing LoaderInfo and LoaderDevicePartUUID part of https://systemd.io/BOOT_LOADER_INTERFACE/ so that we can rely on them.

I can confirm GRUB currently chainloads and correctly verifies the binary sent to it through the standard LoadImage and StartImage functions of the spec, as seen below:

  status = efi_call_6 (b->load_image, 0, grub_efi_image_handle, file_path,
		       boot_image, size,
		       &image_handle);
[...]
static grub_err_t
grub_chainloader_boot (void *context)
{
  [...]
  b = grub_efi_system_table->boot_services;
  status = efi_call_3 (b->start_image, image_handle, &exit_data_size, &exit_data);

The whole chain seems to be secure on my end, just need to investigate the boot services now.

GovanifY · 2023-02-07T20:30:20Z

2. GRUB currently panics at lanzaboote because Boots Services do not return expected handles . What to do here? Patch (in a pretty dirty fashion) GRUB, change the boot logic? Not sure what the best course of action here is...

GRUB does not implement Simple Filesystem it seems like: https://savannah.gnu.org/bugs/?51570
So, I guess our only way is to rewrite a good bunch of lanzaboote to read in memory those resources...
Actually, isn't the current implementation a big TOCTOU by using SimpleFS to re-read the binary after StartImage verified it?

blitz · 2023-02-08T09:16:00Z

I've converted it to a draft to prevent merging it for now.

blitz · 2023-02-08T09:17:02Z

@GovanifY I'll look into this in the coming days!

nix/packages/grub-efi-image.nix

s/prePatch/postPatch and add grub_uki to nativeBuildInputs

blitz · 2023-02-09T21:48:09Z

nix/modules/lanzaboote.nix

+    variant = mkOption {
+      type = types.str;
+      default = "${pkgs.systemd}/lib/systemd/boot/efi/systemd-bootx64.efi";
+      description = "Bootloader variant to use with lanzaboot";


Couldn't resist:

Suggested change

description = "Bootloader variant to use with lanzaboot";

description = "Bootloader variant to use with lanzaboote";

oops, indeed!

blitz · 2023-02-09T21:55:38Z

nix/modules/lanzaboote.nix

+          ${optionalString cfg.enrollKeys ''
+            mkdir -p /tmp/pki
+            cp -r ${cfg.pkiBundle}/* /tmp/pki
+            ${sbctlWithPki}/bin/sbctl enroll-keys --yes-this-might-brick-my-machine
+          ''}


@RaitoBezarius We should really get rid of this functionality. It's an easy way to get to the keys on a multiuser system.

blitz · 2023-02-09T21:57:39Z

nix/packages/grub-efi-image.nix

+    patches = [ ];
+    src = fetchFromGitHub {
+      owner = "GovanifY";
+      repo = "grub";
+      rev = "master";
+      sha256 = "sha256-2XhZdRCK73QJ5QIeTdqXVFinzXq36pJ/6EwnluY5rnA=";
+    };


What changes does your Grub fork have? Do you have an idea about how these could be upstreamed?

The fork is currently not necessary per se, this was more of an experiment. The patch adds some of systemd's boot loader interface, but are really unneeded, I'll probably remove it. The more pressing concern is that GRUB does not implement SimpleFS which is used in the stub and thus causes it to panic.

Correct me if I'm wrong too but it seems like EmbeddedConfig reads back the binary from the filesystem after it was verified by SB with StartImage? If so this is a TOCTOU in the making as anything that could MITM the filesystem could first return a correctly signed stub which then loads back its config from an edited version after that. If I'm right we need to redesign the stub's way of loading the config, regardless of GRUB, which would also make it boot as a side effect.

blitz · 2023-02-09T21:59:01Z

nix/packages/grub-efi-image.nix

+    # this is very probably a terrible idea but grub doesn't allow to generate
+    # at runtime menus like systemd-boot does, so we will need to wait for
+    # lanzaboot to be ready to generate config for both sd-boot and grub.


I guess this is one of the bigger items left to be done?

Well we won't have any runtime menus, no. What can be done though is auto-generation of config a la systemd, but nothing can be done about runtime menus parsing I'm afraid. (We can have menus, but they will need to be generated build-time is what I meant)

blitz · 2023-02-09T22:02:13Z

rust/tool/src/cli.rs

@@ -39,6 +35,10 @@ struct InstallCommand {
    #[arg(long, default_value_t = 1)]
    configuration_limit: usize,

+    /// EFI Bootloader Path (e.g. systemd/lib/systemd/boot/efi/systemd-bootx64.efi)
+    #[arg(long)]
+    efi_boot_path: PathBuf,


lzbt would need two different modes, one for systemd-boot and one for grub to generate the right configs.

Do we want two modes specific for those two boot loaders? If we add support to other things like ext/syslinux later on, wouldn't it make more sense to have a standard interface and then specifics for each bootloaders? E.g. a config generator that could be extended for systemd and grub, an EFI path that would work for any given EFI binary, etc.

blitz · 2023-02-09T22:02:59Z

rust/tool/src/install.rs

+            // TODO(GovanifY): The tests currently fail because of an empty .osrel in my machine
+            // and can fail if using something else than systemd as an EFI binary, what to do with
+            // this?


/cc @nikstur

relevant error:

Error: Failed to read systemd-boot version from "/nix/store/9rjdvhq7hnzwwhib8na2gmllsrh671xg-systemd-252.1/lib/systemd/boot/efi/linuxx64.efi.stub". Caused by: PE section '.osrel' is empty: "/nix/store/9rjdvhq7hnzwwhib8na2gmllsrh671xg-systemd-252.1/lib/systemd/boot/efi/linuxx64.efi.stub"

blitz · 2023-02-09T22:05:50Z

It seems to me the next steps here are:

to sketch out the lzbt changes required to generate the grub config
to figure out how to upstream the grub changes (or at least have some nice patches that we can apply in the meantime to the existing derivation)

@nikstur @RaitoBezarius What do you say?

GovanifY · 2023-02-09T22:38:43Z

It seems to me the next steps here are:

to figure out how to upstream the grub changes (or at least have some nice patches that we can apply in the meantime to the existing derivation)

Those changes probably won't be required as commented above, sorry about the confusion.

See also #96 (comment) for the current main concern

GovanifY · 2023-02-13T21:44:36Z

So, to summarize everything: I found a TOCTOU in the config loading routine.

Here we load the configuration from the binary using uefi_helpers. Those helpers make use of the SimpleFilesystem EFI API. This configuration contains the hashes of the kernel and initrd. This is a cause for concern as anyone who can MITM the filesystem can then return a different binary that was not verified, as the verification of Secure Boot happens during the Load/StartImage stages, much earlier in the boot stage. We need to switch to an in-memory way of reading the hashes.

Now, the second issue is that we use SimpleFs at all, e.g. to load the kernel after the config. Some UEFIs do not have it implemented and currently cause my GRUB tests to fail, but could very well make system-boot fail as well. Fortunately for us, uefi-rs has a function that automatically toggles between SimpleFs, LoadFile2 and Loadfile which we definitely want to use compared to the current method.

This is essentially the main concern of this PR. Any thoughts regarding this/possible implementation?

@nikstur @blitz @RaitoBezarius

RaitoBezarius · 2023-02-13T22:08:02Z

It seems to me the next steps here are:

to sketch out the lzbt changes required to generate the grub config

to figure out how to upstream the grub changes (or at least have some nice patches that we can apply in the meantime to the existing derivation)

@nikstur @RaitoBezarius What do you say?

Per last comment, I think we only need to do the first and address the TOCTOU thing, enable the fallback to LoadFile(2) and we have a prototype support here.

RaitoBezarius · 2023-10-30T12:34:24Z

Please reopen when you rebased it on master (we have some CI issues that requires closing any PRs that are not rebased), do not let this prevent you from rebasing + reopening so we can finally merge it.

GovanifY force-pushed the grub branch from 5fad2d1 to 14f0936 Compare February 7, 2023 11:08

GovanifY added 7 commits February 7, 2023 12:08

WIP: add efi_boot_path cli

5a55cc7

nix: add grub toggle in module

155af45

packages: add grub-efi-image

a6e391e

grub: add efi-binary

7b27da6

make the tests run with a grub efi binary

grub-efi-image: switch to standalone and add part modules

d36d5c9

grub-efi-image: generating default config

7af1c7e

grub-efi-image: add tpm module and setup root device

9f033ef

GovanifY force-pushed the grub branch from 14f0936 to 1d1c2b0 Compare February 7, 2023 11:09

GovanifY added 4 commits February 7, 2023 12:23

grub: few syntactic fixes

95f497b

lanzatool: update the version check

37bc703

Remove the systemd arg as it is redundant with efi_boot. The version check is currently commented out as it cannot work in all situations, we need to figure out something for this!

lanzaboote: update lint/tests for grub

626bc88

tests: add basic grub test

3b26210

GovanifY force-pushed the grub branch from d989150 to 3b26210 Compare February 7, 2023 11:23

GovanifY marked this pull request as ready for review February 7, 2023 11:41

GovanifY changed the title ~~grub support: preliminary PR~~ grub support [ready for review, not for merge] Feb 7, 2023

grub: add unstable version with UKI patchset

e68d4b9

stub/uefi_helpers: add expect error

3e9ed92

GovanifY mentioned this pull request Feb 7, 2023

image_file_system returns NOT_FOUND on own handle under GRUB rust-osdev/uefi-rs#654

Closed

blitz marked this pull request as draft February 8, 2023 09:15

SuperSandro2000 reviewed Feb 8, 2023

View reviewed changes

nix/packages/grub-efi-image.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Feb 8, 2023

View reviewed changes

nix/packages/grub-efi-image.nix Show resolved Hide resolved

SuperSandro2000 reviewed Feb 8, 2023

View reviewed changes

nix/packages/grub-efi-image.nix Show resolved Hide resolved

grub-efi: take care of review comments

ae2e5db

s/prePatch/postPatch and add grub_uki to nativeBuildInputs

GovanifY force-pushed the grub branch from 14da920 to ae2e5db Compare February 8, 2023 15:47

blitz reviewed Feb 9, 2023

View reviewed changes

RaitoBezarius mentioned this pull request Feb 28, 2023

Potential TOCTOU while reading stub data from filesystem rather than memory #123

Closed

RaitoBezarius mentioned this pull request Jun 16, 2023

[RFC 0153] Non-legacy boot NixOS NixOS/rfcs#154

Draft

RaitoBezarius closed this Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grub support [ready for review, not for merge] #96

grub support [ready for review, not for merge] #96

GovanifY commented Feb 3, 2023 •

edited

GovanifY commented Feb 4, 2023 •

edited

GovanifY commented Feb 7, 2023

GovanifY commented Feb 7, 2023 •

edited

GovanifY commented Feb 7, 2023 •

edited

GovanifY commented Feb 7, 2023 •

edited

blitz commented Feb 8, 2023

blitz commented Feb 8, 2023

blitz Feb 9, 2023

GovanifY Feb 9, 2023

blitz Feb 9, 2023

RaitoBezarius Feb 11, 2023

blitz Feb 9, 2023

GovanifY Feb 9, 2023 •

edited

blitz Feb 9, 2023

GovanifY Feb 9, 2023 •

edited

blitz Feb 9, 2023

GovanifY Feb 9, 2023

blitz Feb 9, 2023

GovanifY Feb 9, 2023 •

edited

blitz commented Feb 9, 2023

GovanifY commented Feb 9, 2023

GovanifY commented Feb 13, 2023 •

edited

RaitoBezarius commented Feb 13, 2023

RaitoBezarius commented Oct 30, 2023

	description = "Bootloader variant to use with lanzaboot";
	description = "Bootloader variant to use with lanzaboote";

grub support [ready for review, not for merge] #96

grub support [ready for review, not for merge] #96

Conversation

GovanifY commented Feb 3, 2023 • edited

GovanifY commented Feb 4, 2023 • edited

GovanifY commented Feb 7, 2023

GovanifY commented Feb 7, 2023 • edited

GovanifY commented Feb 7, 2023 • edited

GovanifY commented Feb 7, 2023 • edited

blitz commented Feb 8, 2023

blitz commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GovanifY Feb 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GovanifY Feb 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GovanifY Feb 9, 2023 • edited

Choose a reason for hiding this comment

blitz commented Feb 9, 2023

GovanifY commented Feb 9, 2023

GovanifY commented Feb 13, 2023 • edited

RaitoBezarius commented Feb 13, 2023

RaitoBezarius commented Oct 30, 2023

GovanifY commented Feb 3, 2023 •

edited

GovanifY commented Feb 4, 2023 •

edited

GovanifY commented Feb 7, 2023 •

edited

GovanifY commented Feb 7, 2023 •

edited

GovanifY commented Feb 7, 2023 •

edited

GovanifY Feb 9, 2023 •

edited

GovanifY Feb 9, 2023 •

edited

GovanifY Feb 9, 2023 •

edited

GovanifY commented Feb 13, 2023 •

edited