-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shim 15.7 for Hedgehog ONIE and SONiC #334
Comments
I'm positively impressed with several things here. Like performing additional static analysis of the binaries with Still, I can see a few curiosities regarding your SBAT entries. With the line With GRUB2 I'm unsure. With |
@aronowski essentially, I kept the number to the same of the next upstream version which we are relying on (so 3 for shim, and 2 for fedora grub) - mainly because it made it easier for me to reason about it. Do you guys want me to start with |
SBAT was formalized in a way so the numbers do matter. It should be changed unless there was some specific reason for an exception. Maybe there is one that I just don't know about. I'll let the official committee decide. |
@aronowski no worries, thanks for the input. I'll change the numbers and update the tag 👍 |
@aronowski I updated the builds for the SBAT to start at 1, and created a new tag at: https://github.com/githedgehog/shim-review/tree/hedgehog-shim-15.7-amd64-arm64-20230513 I also updated the issue text above to correctly reflect the new tag, as well as the SHA256 hashes. Is there anything else I need to do right now? Or do I just need to wait for somebody to review this? |
as my builds did not reproduce any longer I had to update them. I guess the compiler or something in Ubuntu 22.04 changed which is why the resulting hashes did not match any longer. I updated the main comment with the new tag, but I'm also putting it here again: https://github.com/githedgehog/shim-review/tree/hedgehog-shim-15.7-amd64-arm64-20230925 @aronowski if you would be so kind and verify it again? 🙏 ... nothing really has changed from what you have already reviewed before. @THS-on thanks so much for pointing this out to me on slack! It's ready for review now if you would like to give it a go as well! 🙏 I apologize that this took a bit longer, I was a bit swamped lately. |
You know it! ;) I managed to reproduce the shim binaries with Docker from the official Fedora 38 repository and installing the docker-buildx plugin v0.11.2 manually. The builds succeed and the checksums match. The usage of additional patches to the kernel as well as its Secure Boot inclusions and exclusions have been explained and are publicly located in the patch folder in the repository The usage of a non-ephemeral key has been thoroughly explained. The aforementioned kernel modules signing enforcement is located in the kconfig-secure-boot-inclusions file in that directory. The kernels used in SONiC and ONIE have been described precisely in regard to the versions used. Furthermore, in regard to the Microsoft requirements, a strictly enforced NX implementation in the kernel might be soon out there. Therefore, the assets have been well prepared for that, I believe.
Maybe @vathpela can confirm the NX implementation in the 6.0 kernels here. |
Review for
|
@THS-on received your PGP encrypted email for the verification, I believe this is what you are looking for:
|
arithmetize subrange tourists custom carbons contrasts potentialities |
Phrases are correct, therefore the contact verification was successful. |
@THS-on please see answers to your questions below
I saw no reason not to use the forked shim-review repo for the build. To be honest, it would be great if the main repo could be enhanced with similar Dockerfile, Makefile, GitHub CI workflow files, etc. as a standardized template for folks to use for their shim builds. That would also make reviewing the builds easier.
Yes, you can find the same instructions in the
As far as I understand it, key usage and extended key usage are optional, right? If that is the case, then I would rather not include this.
The keys were generated inside of the HSM. However, there are no export restrictions to the key. This is to facilitate two things: (1) so that we are able to transfer the key to the backup HSM, (2) so that we can keep an encrypted backup of the key. There are attestation certificates for the keys that prove that the keys were generated within the HSM in our public certificates repo here: https://github.com/githedgehog/certificates/tree/main/attestation The keys that can decrypt the backup also reside on the HSMs. There is no way to unwrap them without the HSM, and also only for the purpose of importing them into the HSM. There is only one (offline) backup of the wrapping key which exists in printed form in our company's bank vault. The people that have access to the backup and are able to decrypt them are the same as our listed security contacts, which are myself and @thedvorkin .
I will send you a read-only invite to the repository shortly. It is our intention that this repository is public, and as it builds/contains GPL software it actually must be public. However, as it uses GitHub runners which are hosted in our lab in the datacenter, we did not make it public yet because of the security and access implications. That said, it looks like there are now ways to better expose and control that, so we will look into that again. This is not the only repository which is affected by this unfortunately.
Yes, this is definitely a feasible option for us. Even if we run out of space, we can always keep an encrypted backup of older keys. The only thing which we do not want to do is to throw away the key after a build/release for the already mentioned reasons.
Just to be clear, you are essentially saying that as long as we compile the SONiC kernel with |
Thank you for the explanation. According to the Microsoft signing requirements the operating environment must achieve a level of security at least equal to FIPS 140-2 Level 2 and the key must be protected by a hardware security module (https://techcommunity.microsoft.com/t5/hardware-dev-center/updated-uefi-signing-requirements/ba-p/1062916).
I've got the invite. If it takes longer to make it publicly available, can you upload an archive for example to the shim-review repository, so that other reviewers can have a look?
For the CA certificate from a technical perspective yes. The leaf certificates can definitely be more restricting.
If you increase the version number accordingly probably. I've described the potential solutions in #345 (comment), but there is currently no consensus on what solutions are accepted. |
Great 👍 Yes, and the shim-review should probably get updated that an HSM is required for those keys. We just did it this way because we thought it's the right thing to do.
I have just made the repository public, so it's now available to view for everybody here: https://github.com/githedgehog/grub-build
This is a patch which originally comes from Debian 2.04 grub: https://sources.debian.org/patches/grub2/2.04-12/no-devicetree-if-secure-boot.patch/ However, I adopted it from the ONIE grub patchset here: https://github.com/opencomputeproject/onie/blob/master/patches/grub/2.04/no-devicetree-if-secure-boot.patch That said, that patch might not be required anymore, as I can see (right now) that the later Debian grub versions are not including that patch any longer. @steve-mcintyre might know, as he signed off on that patch :) Also, this patch would be specific/required for the aarch64 grub build only, which we currently don't target beyond a virtual machine image with ArmVirt firmware as no ARM hardware platform has been selected yet (which will come sooner though than I want for sure).
Makes sense. I got burned in the past with certificates that had too many extensions, so by now I'm mindful using them when not needed - and particularly with hardware/firmware the probability that something can and will go wrong is just higher.
Yeah, I read that comment. Both solutions in there essentially match what we discussed here, and both would work for us. |
👍
Upstream there exists now the Lockdown framework since 2.06 and that implements similar functionality: https://git.savannah.gnu.org/cgit/grub.git/commit/?id=468a5699b249fe6816b4e7e86c5dc9d325c9b09e If I see that correctly, Fedora uses that feature when Secure Boot is enabled, so that patch is likely not needed anymore.
I think for now choose the option that best fits and we see after the meeting what options are accepted. |
@THS-on as discussed earlier re managing keys for kernel modules, we are going to choose the following path:
|
As discussed today the kernel module signing is not a blocker. If we can another (also non official) review, then is is ready to go. Maybe someone of the new (nearly) accepted shims can have a look (@akodanev, @Fabian-Gruenbichler, @keithdlopresto)? |
I’m not an authorized reviewer, I’m just trying to help and learn. I was only able to build the x86_64 version, so I'm reviewing only that. sha256:
Build is reproducible, sha256 is confirmed. Obj Alignment:shimx64.efi
Alignement is ok. DllCharacteristics:shimx64.efi
NX_COMPAT is enabled. Sections:shimx64.efi
Code section is not writable: OK SBAT:shimx64.efi
SBAT seems ok to me. Certificate:
It's a CA, good ============================================ |
@ClaudioGranatiero-10zig thanks for having also a look at it. Got three reviews now, marking as accepted. |
What is the status of this? Did you get a signed shim back or are you creating a new submission for 15.8? |
@THS-on after the news about the upcoming 15.8 release I did not bother anymore to submit the shim to Microsoft in December. As I'm no longer working for Hedgehog I cannot speak to the actual state of this though. |
@mheese ok I'll close this one then. |
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/githedgehog/shim-review/tree/hedgehog-shim-15.7-amd64-arm64-20230925
What is the SHA256 hash of your final SHIM binary?
What 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: