Shim 15.7 for Proxmox Bookworm-based releases #330
Comments
|
While I'm not an official reviewer, I can see a few curiosities: Why is your GRUB2's product-specific generation number set to 3? The Does your kernel have the latest NX requirements mentioned here enabled:
In other words: do you have a patch that toggles the NX compatibility flag for the Since I can't see any inside the There's also an error when trying to browse the tree through your git web interface: |
|
thanks for the fast response! we are still in the process of preparing our bookworm based release, so some things are still in flux.
after re-reading https://github.com/rhboot/shim/blob/main/SBAT.md , it is clear to me it should be 1 - thanks for catching this! what's the procedure here (other than updating it in our Grub 2 packaging - which should be unproblematic given that nothing has been signed so far using the production keys) - point this issue at an updated tree in our fork of
I'll see about getting it published today.
as per #307 I was under the impression that submitting an NX-enabled shim while working on the rest of the stack is okay (it's linked at the top of the Issues queue ;)). if that's no longer the case it might be good to update it. if it's a requirement for getting a (new) shim signed, I'll prioritize pulling in and testing x86_64: Improvements at compressed kernel stage our current kernels for Bullseye are not signed, I'll see about publishing our Bookworm branch (which has the signing bits enabled and packaging updated to support post-build signing of the kernel image itself) later today as well and post instructions, since the git submodules part can be a bit annoying.
this is just a mirror of the Ubuntu kernel tags we use (as base) in our build, it usually only gets tags pushed to it and gitweb doesn't handle that very nicely. the actual repository is the pve-kernel one at https://git.proxmox.com/?p=pve-kernel.git;a=summary , which references the mirror (and other things) as git submodule. but please note that the public version of that git repo only contains our Bullseye state, not the one for the upcoming Bookworm based release, as indicated above I'll work on fixing that. |
|
The procedure I personally follow is to update my review e.g. in the You can use this for inspiration, but change things according to your situation - I attach assets directly in the review rather than pointing to a remote repository. I'm waiting for the GRUB2 repository related updates. Please, ping me once they are here. |
|
fwupd-efi packaging tree for our upcoming bookworm releases, based on Debian bookworm packaging: grub2 packaging tree, same (vendor SBAT already corrected to 1): kernel packaging tree, packaging is custom: the kernel packages consist of packaging files (custom, directly in the repository), kernel sources (based on Ubuntu Lunar's, which are in turn based on upstream 6.2.x, included via git submodule in to get all of the kernel build files a recursive clone can be used |
|
@aronowski here you go! updated review request to point at updated shim-review tag (all changes as discussed here in the comments are also noted in the additional information part of the review readme) |
|
Thank you for the update! The GRUB2-related things I'd like to discuss: I can now see that these GRUB2 sources come directly from Debian and you don't add anything custom on your own: However, it might be a good idea to leave Debian's SBAT entry as-is and append Proxmox's to the end. It's going to simplify management too as you'll always be having This would also comply with the Red Hat Bootloader Team's guidelines:
Recently there's also been this incident in Debian. Just saying. I checked the sources for GRUB2's NX support and I cannot see its presence. It appears to me that Debian doesn't have it implemented. Not that it's required right now as we're having the conversation, but it will be sooner or later for the whole chain to be compliant with Microsoft's requirements. There are already patches out there for Fedora/RHEL that implement it: look here for the files that contain When it comes to the kernel, I can see there's this mainline implementation: And I can see this line in your So judging by this hint, I assume the NX support is already here. But having no experience with Debian's configuration, I can't tell for sure. What I would do, however, would be to inspect the In other reviews I recommend tools for static analysis of PE binaries such as NTCore's CFF Explorer or zed-0xff's pedump to act as judges. You can use them yourself or link the artifact for me to inspect on my own (it could well be a Off-topic: What's going on with the git server? I can browse files through the web interface, but can't clone the GRUB2 and fwupd repositories. I keep on getting the message: Cloning these repositories would come in handy - it would be faster to browse them and I could have the whole history with me offline - useful when reviewing during a travel with no stable internet connection. |
that seems sensible - at least for the time being, since the amount of delta might change at some point, we are not as conservative as Debian w.r.t. backporting features or fixes, although for Grub it will definitely fall more into the latter category (e.g., we likely would have backported https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=987008 if we were already shipping our own, custom Grub packages). the same rationale would apply to fwupd-efi as well (or even more so, since the need for deviation is even smaller for that compared to Grub).
yes, we are aware of that (this issue was actually what triggered our mishap of
we will definitely stick to Debian's lead here, and are closely following developments there (I had some fruitful exchanges with the Debian EFI team regarding their implementation, and there is considerable overlap between that team and Debian Grub maintainership as well, and I am maintaining a few unrelated packages in Debian itself as well).
the default HEAD is likely pointing to a non-existent
you can clone them, just append |
|
So there will indeed be some changes rather than using the stock Debian releases. That's OK. As for the SBAT entry, I'll let the official committee decide on this specific scenario. Doesn't bother me personally but I may be missing something. Regarding GRUB2 NX support - while I'm not a developer, I'll see what can be done to speed up the process. Regarding this one off-topic thing I mentioned and others that may be to come, I don't want to cause unnecessary noise in the review and may instead send an email soon. To sum it up, the review looks good to me now. Good job! |
|
FWIW, the HEADs of our grub2, fwupd-efi, efi-boot-shim repos should now be properly pointing to their respective branches :) |
THS-on
added
new vendor
Review of
|
|
here's the verification part for my mail address: |
that seems sensible! is it okay to just include that in our first signed Grub package version, since it would not affect the shim hash/acceptance directly?
and also here: https://pve.proxmox.com/wiki/Security_Reporting :) if desired, we can also upload it to a few keyservers of course. |
Because the SBAT entries are part of the review, can you update it to include the new entry? Then just create a new tag where this is the only difference. After this it looks good to go from my side. |
|
done! we will closely follow Debian's development for all involved packages - at the moment the last CVE fixes for Grub (CVE-2023-4693 and CVE-2023-4692) are still not available for Bookworm, but we will ensure they are folded in before releasing our first signed packages, which will likely entail another SBAT bump for the upstream component. |
|
Tag I would like for at least one other reviewer to take a look at it before marking it as accepted (and the completing the contact verification). @aronowski you already had a look at it, can you take another closer look? |
|
As requested, for the Proxmox Security Team I got the following verification words in the mail: |
|
I'm not an authorized reviewer, I'm just trying to help and learn.
|
THS-on
removed
the
contact verification needed
|
Phrases are correct, therefore the contact verification is complete. |
You know it! A thorough review will be made the next Tuesday at the earliest. I'm kind of all tied up at the moment and commenting on this for transparency reasons. |
Basically what we've been discussing back then in April - seems A-OK to me! No "thorough review" as I said back on Saturday, though - I had too much on my mind and didn't check, that it was only the SBAT-related thing that changed.
Done! |
|
Marked as accepted. Thanks you @aronowski and @ClaudioGranatiero-10zig for helping with the review. |
|
thanks for the reviews (and acceptance)! :) |
|
got a signed shim from MS and using it in production already! closing accordingly. |
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/Fabian-Gruenbichler/shim-review/tree/proxmox-shim-15.7-amd64-20231006(updated based on first feedback by @aronowski)
(2023-10-06: updated again to include Debian Grub SBAT entry)
What is the SHA256 hash of your final SHIM binary?
d93f0245909a4655ceb8961778f382897b2cc50b1d1e996d1ac450cf7fbcfeb7What is the link to your previous shim review request (if any, otherwise N/A)?
N/A
The text was updated successfully, but these errors were encountered: