search

rhboot / shim-review

Reviews of shim
66 stars 131 forks source link

Shim 15.8 for Endless OS #439

Open dbnicholson opened 3 months ago

dbnicholson commented 3 months ago

Confirm the following are included in your repo, checking each box:

What is the link to your tag in a repo cloned from rhboot/shim-review?

https://github.com/endlessm/shim-review/releases/tag/endless-shim-x64-20240912

What is the SHA256 hash of your final SHIM binary?

7859e02e1fc6dff8e2b221dfcbfaffcb6d1e95e2acb65403e5db7c849f9221cd

What is the link to your previous shim review request (if any, otherwise N/A)?

https://github.com/rhboot/shim-review/issues/176

If no security contacts have changed since verification, what is the link to your request, where they've been verified (if any, otherwise N/A)?

The security contacts have not changed, however I can't find any indication of verification in any previous reviews. Those are:

https://github.com/rhboot/shim-review/issues/176 https://github.com/rhboot/shim-review/issues/105 https://github.com/rhboot/shim-review/issues/61

es-fabricemarie commented 2 months ago

Issue/question:

dbnicholson commented 2 months ago

@es-fabricemarie thanks for the review!

It actually does use the official tarball, but it restores it using pristine-tar from this data. I can update the Dockerfile so it shows the actual tarball used during the build. https://deb.endlessos.org/debian/pool/core/s/shim/shim_15.8-1~deb12u1endless1.dsc shows the data from the generated source package including the checksum of the tarball:

Checksums-Sha256:
 a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9 2315201 shim_15.8.orig.tar.bz2
 fcbd64a83973fdcec6a0f98aef07ba5e288ff2d07f1050bc6f9c3bdd875caed3 228 shim_15.8.orig.tar.bz2.asc
 baf1852fba47e2bd401d81169b6899b0b18b8fa93af6b7d1680b54dfe4c967fd 65700 shim_15.8-1~deb12u1endless1.debian.tar.xz
steve-mcintyre commented 2 months ago

The security contacts have not changed, however I can't find any indication of verification in any previous reviews. Those are:

176 #105 #61

Let's do this from scratch then...

Mails on the way.

ramcq commented 2 months ago

I got: distention knuckling unexceptionable hydras tourneys comeliness fraternize Villa cosmetologists domains

wjt commented 2 months ago

craws showbiz switch Dramamine unannounced internists babysitters tared quantum jouncing

The verb “to jounce” is new to me.

Thanks!

dbnicholson commented 2 months ago
  • additional shim patches present will need more eyeballs/reviews.

In case this is holding up the review, I'd like to discuss these. There are 4 patches:

The first 2 are backports from upstream post-15.8 and inherited from Debian. They simply make the newer SBAT policies available for use. The second 2 are our real downstream patches. They're both against fallback and are trying to address the same issue. 0003-Revert-fallback-work-around-the-issue-of-boot-option.patch is really an optimization once 0004-fallback-Clean-up-duplicate-boot-entries.patch is applied. So, really 0004-fallback-Clean-up-duplicate-boot-entries.patch is the downstream change we make.

Endless OS is always installed by flashing a raw disk image containing filesystems. By definition, the filesystems in the disk image contain UUIDs. During first boot the UUIDs are randomized since they'd no longer be unique if every Endless OS installation had the same partition UUIDs. However, changing the ESP UUID means that existing EFI load options referencing that partition are now invalid. What currently happens is this:

  1. User boots Endless OS for the first time.
  2. Shim finds no valid load options and loads fallback.
  3. Fallback finds endless/BOOT<arch>.csv, creates a new load option referencing the current ESP UUID and loads it.
  4. In the first boot initramfs, the disk partitions are updated. The UUIDs are replaced and the main partition is expanded to fill the disk. At this point, the load option just created by fallback has become invalid since it's referencing the old ESP UUID.
  5. On the second boot, shim again finds no valid load options and loads fallback.
  6. Fallback creates a new load option as in step 3, but now it's referencing the new ESP UUID.
  7. In the second boot initramfs, the disk partitions are left as is since they've already been changed.
  8. On the third and subsequent boot, shim finds a valid load option and loads it.

The problem 0004-fallback-Clean-up-duplicate-boot-entries.patch is trying to address is after step 4. If the invalid load option isn't removed, then there will be 2 entries for Endless OS, one of which is invalid. That's a poor user experience, so what the patch does is add a routine to delete any existing options with the desired title before adding the new load option. That means you can only have one load option with a given title created by fallback. If your system had another ESP or there was another vendor directory and either provided a load option with the same title, they would be deleted.

This works OK for Endless OS since that type of advanced functionality isn't supported, but it means the patch isn't really upstreamable. I've been working on a tool that updates the partition UUID in an existing load option, which we'd run at step 4 when the UUID is changed. I'm actually surprised that kind of tool doesn't exist (the closest I've seen is to manually do it with efibootmgr), but maybe people changing their ESP UUID is rare enough that the problem is ignored. Anyways, if it would speed up review to drop that patch, I can try to finish up my tool and get it integrated in Endless OS.

dbnicholson commented 2 months ago

I was able to finish up the work that handles updating the UUID in the load option after the ESP partition UUID is changed. The program is here if anyone cares. With that I was able to update our shim package to drop our 2 downstream fallback patches (0003-Revert-fallback-work-around-the-issue-of-boot-option.patch and 0004-fallback-Clean-up-duplicate-boot-entries.patch).

The shim binary is exactly the same as before since the patches were against fallback rather than shim. Still, I've updated our shim-review repo with the current build log. The new tag is https://github.com/endlessm/shim-review/releases/tag/endless-shim-x64-20240912. @es-fabricemarie would you mind reviewing again? Everything should be the same as before except for the 2 dropped patches.

es-fabricemarie commented 2 months ago

Sorry for the delay @dbnicholson

This shim looks good to go to me. You'll need an official approver to move this along, maybe @steve-mcintyre if you have time? (This is a Debian based shim)

evilteq commented 2 months ago

I really went mad looking up and down for the 0003 and 0004 patches :P

Built using gpb's black magic against a github repo fork, and the generated orig tarball matches the mainstream one. I was able to reproduce it it 7859e02e1fc6dff8e2b221dfcbfaffcb6d1e95e2acb65403e5db7c849f9221cd.

Nothing out of the ordinary, looks good to me!

THS-on commented 1 month ago

Review of endless-shim-x64-20240912

Shim

GRUB2

Linux

Questions

dbnicholson commented 1 month ago

Questions

  • Is your main CA key also in an HSM or differently?

The CA key is kept on an offline encrypted disk that only a couple people have access to and can decrypt.

  • Can you provide a easy way to compare your GRUB2 patches against the Debian package or upstream GRUB2? Mostly interested in that all the CVE patches are applied and what custom modifications are made to GRUB2 that are not packaging/config related

The way we generate the grub source package is kinda unusual. We take all of Debian's patches and apply them directly as commits. Then we add our own changes on top so that we can treat it as a normal git repo instead of using patch files. There are better ways to do that nowadays, but nobody has taken the time to rework our build system.

Anyway, the full set of commits on top of grub 2.06 as of today is here. All of Debian's patches are prefixed with [DEB] and then ours start after that. This is based on Debian's 2.06-13+deb12u1 version. You can see it in their format here with their patches in particular here.

We haven't added any additional security patches on top of what Debian has done. The main thing we add is the blscfg module from RedHat. That and a couple other command tweaks allows us to use a fixed grub.cfg with upgrades managed by ostree.

  • Note because you apply the Debian patches, you are going to automatically revoke your old Shim when booting a new one. (should be the same revocation entries as via Windows upates)

Yeah, that should be fine. We don't plan to support rolling back to an earlier shim. The people that need to do something like that can also just change the SBAT policy. Not coincidentally, Windows Update erroneously setting the SBAT policy in our dual boot cases is what spurred getting shim updated.

dbnicholson commented 1 month ago

@THS-on anything else I can provide to answer your questions?

THS-on commented 1 month ago

@dbnicholson had a look at the GRUB2 patches and they match the ones that are from Debian and required for SBAT level 4.

Regarding the custom ones:

The rest LGTM

dbnicholson commented 1 month ago

I didn't develop those patches but I agree it would be good to upstream them. I can start an effort to do that. We're usually good about upstreaming our patches for no other reason than it eased our work when upgrading. I don't know why we haven't made an effort with grub. I would guess that at the time most of those were developed, grub upstream was in a different state.

steve-mcintyre commented 1 month ago

Looks good here with enough positive reviews, marking accepted

dbnicholson commented 1 month ago

Thank you! @THS-on thanks for raising the flag on grub. For next time I hope that patch queue can be much smaller.