rhboot / shim-review

Reviews of shim
66 stars 131 forks source link

Ctrl IQ, Inc EL9 Shim 15.8 for x64 #420

Open jason-rodri opened 5 months ago

jason-rodri commented 5 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/ctrliq/ciq-shim-build/releases/tag/ciqliq-shim-EL9-x64-20240705


What is the SHA256 hash of your final SHIM binary?


SHA256 (shimx64.efi) = f67bf3bb333d1e8ecfbb372f93ad7056e12c43c4eedc335e235c66b7af9fa940


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


Ctrl IQ, Inc Shim 15.8 for x64 & ia32 #366


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)?


Jason Rodriguez Michael Young

SherifNagy commented 5 months ago

Can we get the UKI's SBAT, or you aren't providing UKI kernel?

jason-rodri commented 5 months ago

Can we get the UKI's SBAT, or you aren't providing UKI kernel?

We are not moving forward with signing UKI build at this time.

jason-rodri commented 5 months ago

@aronowski @SherifNagy Please let me know if I can provide additional information to help the review process.

SherifNagy commented 5 months ago

@jason-rodri nothing at the moment, we are working through the queue

aronowski commented 4 months ago

The application looks alright, apart from one minor nitpick:

*******************************************************************************
### Does your SHIM load any loaders that support loading unsigned kernels (e.g. GRUB2)?
*******************************************************************************
Grub2 will only load unsigned code if the secureboot feature is turned off  load unsigned kernels, but only with secureboot mode turned off on an end-user's system.

The duplicated explanation in the answer hasn't been fixed yet, just like in the EL8 application. ;-)

SherifNagy commented 4 months ago

Review of ciqliq-shim-EL9-x64-20240518

Shim

Shim Comparison, original binary vs. freshly built binaries:

SHA256 sums :: f67bf3bb333d1e8ecfbb372f93ad7056e12c43c4eedc335e235c66b7af9fa940 /shimx64.efi f67bf3bb333d1e8ecfbb372f93ad7056e12c43c4eedc335e235c66b7af9fa940 /shim_result/usr/share/shim/15.8-0.el9/x64/shimx64.efi


* NX flag is not set, because the chain is not yet ready
* Self signed 4096 bit cert and valid for almost 24 years

## GRUB2
* SBAT looks fine (keeps upstream RHEL/rocky grub2)
* Version currently does not include NTFS patches, but the signed versions also not include the NTFS module so sbat `grub,3`
* Module list sound fine

## Kernel
* Ephemeral keys are used for signing kernel modules
* Lockdown patches are included (keeps upstream RHEL kernel)

## Notes

Along side the note from @aronowski 
* UKI kernel isn't signed, so keep an eye on the meta issue #397 
* grub2's SBAT note section  "version" isn't 100% accurate with upstream
* fwupd notes section as well needs fixing, below is what  Rocky currently has

sbat,1,UEFI shim,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md fwupd-efi,1,Firmware update daemon,fwupd-efi,1.4,https://github.com/fwupd/fwupd-efi fwupd-efi.rhel,1,Red Hat Enterprise Linux,fwupd,1.9.13,mail:secalert@redhat.com fwupd-efi.rocky,1,Rocky Linux,fwupd,1.9.13,mail:security@rockylinux.org


* You aren't keeping any upstream fwupd entries for sbat, I assume this is acceptable Cc: @steve-mcintyre for input

Other than those few notes, LGTM, we will need one more reviewer
THS-on commented 3 months ago

Review of ciqliq-shim-EL9-x64-20240705

Shim

GRUB2 and fwupd

Kernel

LGTM! I'll raise the certmule/certwrapper question during today's call and will get back to you

THS-on commented 3 months ago

As discussed yesterday using certwrapper to import the Rocky Linux CA is likely fine. We just need confirmation that certwrapper is ready for production use and can be signed. @jsetje do you know the status of certwrapper?

jason-rodri commented 3 months ago

Signed binaries returned from MSFT.

I understand the that there is still a question around certwrapper. Do we still want to keep this open until @jsetje replies?

THS-on commented 3 months ago

I understand the that there is still a question around certwrapper. Do we still want to keep this open until @jsetje replies?

Yeah let's keep this open until there is a definite answer if you can sign certwrapper

THS-on commented 2 months ago

@jsetje is certwrapper now ready to sign for people or should they wait?

THS-on commented 2 weeks ago

As discussed with @steve-mcintyre, the certwrapper is ready to sign.