tSU-RooT commented 5 months ago

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

[x] completed README.md file with the necessary information
[x] shim.efi to be signed
[x] public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
[x] binaries, for which hashes are added to vendor_db ( if you use vendor_db and have hashes allow-listed )
[x] any extra patches to shim via your own git tree or as files
[x] any extra patches to grub via your own git tree or as files
[x] build logs
[x] a Dockerfile to reproduce the build of the provided shim EFI binaries

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

https://github.com/miraclelinux/shim-review/tree/miraclelinux-x64-20240524

What is the SHA256 hash of your final SHIM binary?

60485ece0fa7d8a01975e65d6845b9c084018e4c15d40e2d2d1bbe0bbbdca5d9 shimx64.efi

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

https://github.com/rhboot/shim-review/issues/264 is previous shim-review request (accepted)

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

Security contacts are changed, Primary contact is updated to my colleague.
Second contact is me(verified).
https://github.com/rhboot/shim-review/issues/266#issuecomment-1238797230

steve-mcintyre commented 4 months ago

@tSU-RooT has had contact verification done before (hi!) Sending a message to Masayuki Moriyama now...

moriyama commented 4 months ago

I received the following words:

Chaldean darkening alienation defenses ea keybinding Skye cupid skillful co=
llectivism

aronowski commented 3 months ago

Seems OK to me. I'll submit the appropriate UKI/systemd-boot SBAT entries to this issue.

Just one positive review (might be from a non-accredited reviewer) remaining and the application can be accepted.

akodanev commented 3 months ago

Shim review of "miraclelinux-x64-20240524" (by a non-accredited reviewer):

Now includes one more certificate, valid until 2029.
There are no additional patches for the 15.8 release.
NX bit is not set.
The grub policy is set to `grub,3' (NTFS module not included)

.sbat section: why is shim.miracle now set to 2?

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.miracle,2,Cybertrust Japan,shim,15.8,ml-packager@miraclelinux.com

Reproducible build:

#25 [21/21] RUN sha256sum /usr/share/shim/15.8-2.el9.ML.1/x64/shimx64.efi /shimx64.efi
#25 0.283 60485ece0fa7d8a01975e65d6845b9c084018e4c15d40e2d2d1bbe0bbbdca5d9  /usr/share/shim/15.8-2.el9.ML.1/x64/shimx64.efi
#25 0.289 60485ece0fa7d8a01975e65d6845b9c084018e4c15d40e2d2d1bbe0bbbdca5d9  /shimx64.efi
#25 DONE 0.3s

GRUB, Linux kernel

Based on RHEL9.

So there is a question about the generation number bump of shim.miracle, otherwise the rest looks good to me.

Also minor comments about SBAT sections consistency:

The last part of the component name changes for UKI, for all other entries it is .miracle. Just out of curiosity, is there a reason for this?
Some vendor_url entries with ml-packager@miraclelinux.com address have mail:, mailto: prefixes or nothing.

steve-mcintyre commented 1 month ago

Review of shim 15.8 for MIRACLE LINUX 9

Good

General

Contact verification completed - OK
key management using an HSM: Yubikey - OK
Switched to a new embedded cert for a new root of trust - OK

Shim

Builds from 15.8 upstream (ish - see issues below), with no patches - OK
shim builds reproduce here for x64 and ia32 with the Dockerfile - OK

60485ece0fa7d8a01975e65d6845b9c084018e4c15d40e2d2d1bbe0bbbdca5d9  /usr/share/shim/15.8-2.el9.ML.1/x64/shimx64.efi
60485ece0fa7d8a01975e65d6845b9c084018e4c15d40e2d2d1bbe0bbbdca5d9  /shimx64.efi

NX bit not set on shim binaries - OK

Issues / queries

I don't see why you're grabbing Peter's shim-unsigned-x64-15.8-2.el9.src.rpm rather than just using the upstream shim tarball directly. This isn't a blocker for this review as it's possible to check the tarball checksum from the src.rpm. Please don't do this again, though.
You say "We don't use vendor_db functionality in this build." but you quite clearly do. From the build:
```
make TOPDIR=.. -f ../Makefile COMMIT_ID=5914984a1ffeab841f482c791426d7ca9935a5e6 EFIDIR=almalinux PKGNAME=shim RELEASE=2.el9.ML.1 ENABLE_SHIM_HASH=true SBAT_AUTOMATIC_DATE=2023012900 VENDOR_DB_FILE=/builddir/build/SOURCES/vendordb.esl 'DEFAULT_LOADER=\\\\grubx64.efi' all
```
I can see three .der files in your submission, and maybe those three keys in your vendordb.esl. As requested in the README.ms template: "If you use vendor_db functionality of providing multiple certificates and/or hashes please briefly describe your certificate setup."

I'm stopping here until this is resolved.

moriyama commented 1 month ago

I can see three .der files in your submission, and maybe those three keys in your vendordb.esl. As requested in the README.ms template: "If you use vendor_db functionality of providing multiple certificates and/or hashes please briefly describe your certificate setup."

Thanks for your review.

2 certificates enrolled in vendor_db. vendor_db contains ml9secureboot001.der and ml9secureboot002.der, which are included in this repository.

moriyama commented 3 weeks ago

I'll answer some of your questions since they were left unanswered.

akodanev commented on Jul 12:

5..sbat section: why is shim.miracle now set to 2?

Following the example of RHEL 9 [1] and AlmaLinux OS 8 [2], we increased the value of shim.miracle.

The last part of the component name changes for UKI, for all other entries it is .miracle. Just out of curiosity, is there a reason for this?

This is because the different people in charge of the packages each decided on their own the last part of the component name.

Some vendor_url entries with ml-packager@miraclelinux.com address have mail:, mailto: prefixes or nothing.

I will add mailto: in the next update.

[1] https://github.com/rhboot/shim-review/issues/373 [2] https://github.com/rhboot/shim-review/issues/407

akodanev commented 6 days ago

@moriyama Thanks for the update!

Following the example of RHEL 9 [1] and AlmaLinux OS 8 [2], we increased the value of shim.miracle.

OK, I just thought there was some Miracle Linux specific issue not mentioned here that needs product-specific bump in shim, good to know there really is nothing.

BTW, looks like there is still no fixed tag with vendor_db functionality: https://github.com/miraclelinux/shim-review/tree/miraclelinux-x64-20240524.

moriyama commented 4 days ago

@akodanev We updated README. New tag is: https://github.com/miraclelinux/shim-review/tree/miraclelinux-x64-20241017

rhboot / shim-review

shim 15.8 for MIRACLE LINUX 9 #421