search

rhboot / shim-review

Reviews of shim
67 stars 130 forks source link

Ctrl IQ, Inc Shim 15.7 for x64 & ia32 #339

Closed jason-rodri closed 8 months ago

jason-rodri commented 1 year 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://bitbucket.org/ciqinc/ciq-shim-build/src/ctrliq-shim-x64-ia32-20231222/ Repo migrated to https://github.com/ctrliq/ciq-shim-build/tree/ctrliq-shim-x64-ia32-20231222

What is the SHA256 hash of your final SHIM binary?

25aaf786950ab589de771097a87995a114d11df5aa195a2049ab88894028b28c /shimx64.efi 09841f7081a79084d993deeb3716f44537e5eb42297a0b62f8e43c56768e44ed /shimia32.efi

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

N/A. This is our first submission.

aronowski commented 1 year ago

As far as I can see, your shim binaries are affected by this bug.

Hint: since you're rebuilding RHEL, you can just use my port of the patch that fixes this, called buggy-binutils.patch I created here.

jason-rodri commented 1 year ago

I know there are a lot of things going on in the committee and we appreciate all of your hard work. Can we get an update on when our shim would start the process? When can we expect verification challenge and such?

Thank you in advance!

aronowski commented 1 year ago

Jason, as far as I'm aware, it would be optimal to start the process once the aforementioned binutils-related bug has been fixed.

Back then I did not add any label since I didn't have access to such an action, but as the current status is, the application simply wouldn't get accepted.
There's also a formatting error in this issue's introductory post - all boxes must be checked. I know that this might be counterintuitive as it is today, therefore I wrote a proposal, which would clarify this:

checking all the boxes in the sense "I've dealt with this, so I'm checking the checkbox." rather than "This is not applicable for me, so I'm leaving the checkbox unchecked."

If there's something that requires help, clarification, porting assistance, etc. from my side, then please, let me know - I'll be happy to make things go faster here for you and your team.

jason-rodri commented 1 year ago

Kamil,

Thank you for the guidance! I was able to update the our shim src repo and the reproducible build repo with the buggy binutil patch. I also corrected the check marks as advised.

The output to objdump output as well:

objdump -s -j .sbatlevel shimx64.efi 

shimx64.efi:     file format pei-x86-64

Contents of section .sbatlevel:
 85000 00000000 08000000 22000000 73626174  ........"...sbat
 85010 2c312c32 30323230 35323430 300a6772  ,1,2022052400.gr
 85020 75622c32 0a007362 61742c31 2c323032  ub,2..sbat,1,202
 85030 32313131 3530300a 7368696d 2c320a67  2111500.shim,2.g
 85040 7275622c 330a00                      rub,3..
aronowski commented 1 year ago

Thanks for the update! Moving on with the review.

Jason, your public key seems to be valid for less than 2 years. Will that be alright? I mean that once it expires, a new contact verification will be needed.

As far as I can see, Michael's public key has expired on September 27, 2023. Therefore, the contact verification can't proceed until this is resolved.

What patches are being applied and why

I believe this section should also mention the buggy binutils patch too.

I know it was already mentioned earlier, but still.

*******************************************************************************
### If these fixes have been applied, have you set the global SBAT generation on your GRUB binary to 3?
*******************************************************************************
We have not, as we are a new vendor.  Our release should exactly match the Rocky and RHEL equivalents.

Oh, this is the one section that was historically being misunderstood and for a good reason - it's a bit unclear and confusing. I filed a proposal here in the Improving the document for a review application paragraph to make it more straightforward in the future.

In most cases the answer should be: yes, it has been set to 3, because that's what there is in Rocky, am I right?

Try this command and see what happens in regard to the entry that mentions Free Software Foundation - is that line the same as in the listing below:

# objcopy --only-section .sbat -O binary /boot/efi/EFI/**/grubx64.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,3,Free Software Foundation,grub,2.02,https//www.gnu.org/software/grub/
grub.rh,2,Red Hat,grub2,2.02-148.el8_8.1,mailto:secalert@redhat.com
[...]
*******************************************************************************
### If your boot chain of trust includes a Linux kernel:
### Is upstream commit [1957a85b0032a81e6482ca4aab883643b8dae06e "efi: Restrict efivar_ssdt_load when the kernel is locked down"](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1957a85b0032a81e6482ca4aab883643b8dae06e) applied?
### Is upstream commit [75b0cea7bf307f362057cc778efe89af4c615354 "ACPI: configfs: Disallow loading ACPI tables when locked down"](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75b0cea7bf307f362057cc778efe89af4c615354) applied?
### Is upstream commit [eadb2f47a3ced5c64b23b90fd2a3463f63726066 "lockdown: also lock down previous kgdb use"](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eadb2f47a3ced5c64b23b90fd2a3463f63726066) applied?
*******************************************************************************
Yes, all of these patches are already in the Rocky/RHEL 8 + 9 kernels we plan to base on.

Historically the eadb2f47a3ced5c64b23b90fd2a3463f63726066 commit was not being shipped with the kernel that RHEL and its replicas provide. This is due to the fact that these kernels are built with CONFIG_KDB_DEFAULT_ENABLE=0x0, so the security issue, which this commit solves, is not there at all.

If something changed and I'm not aware of that, then please, point me to the exact location, where I can find this commit being introduced in CIQ's kernel - we're all here to learn, myself included.

*******************************************************************************
### What is the SHA256 hash of your final SHIM binary?
*******************************************************************************
SHA256 (shimx64.efi) = 7f739b33cd666685cdba46a9b16265d12b73e73c41838a76be4d01f342b5831b

As far as I can see this is the old checksum that referred to the shim binary with the buggy .sbatlevel section.

It needs to be updated to the current one(s). I've confirmed that the reproducibility is right there, and that's a great thing! The Insert shim-compare.sh script and run you have in your Dockerfile prints the correct, current checksums:

[...]
Step 17/17 : RUN chmod 0755 /root/shim-compare.sh;  /root/shim-compare.sh
 ---> Running in d6c652179153

Shim Comparison, original binary vs. freshly built binaries:

SHA256 sums ::
8f0d4cdae78a9c404ea70ff9a36189067dfa646aa3368d472e7782003d30a969  /shimx64.efi
8f0d4cdae78a9c404ea70ff9a36189067dfa646aa3368d472e7782003d30a969  /shim_result/usr/share/shim/15.7-3.el8/x64/shimx64.efi
c267adea2ad49ac3b2d595d3e7cf597a5caab8206415df748a5f7770be8c1a3f  /shimia32.efi
c267adea2ad49ac3b2d595d3e7cf597a5caab8206415df748a5f7770be8c1a3f  /shim_result/usr/share/shim/15.7-3.el8/ia32/shimia32.efi
[...]
*******************************************************************************
### How do you manage and protect the keys used in your SHIM?
*******************************************************************************
- Private key for CA is kept in offline vault by default
- Private keys for child certificates stored on HSM in a FIPS-140-2 environment for signing
- Private keys are not exportable from HSM by design
- Key backups are also kept in secure offline vault

Does Private key for CA mean the private part for the CA certificate, which can be found at http://crt.enterprise.sectigo.com/CtrlIQIncRootCA.crt, the ciq_sb_ca.der file in your shim SRPM (or is this the child certificate) or something else?

How is the vault being secured and how the security measures prevent exfiltration through covert channels, or by malware that can bridge air gaps, among others?

Are backups stored in a way that prevents destruction of everything in one fell swoop by a fire accident, a rogue employee, etc. - I mean if they are stored in, at the very least, a physically distinct place?

*******************************************************************************
### Do you add a vendor-specific SBAT entry to the SBAT section in each binary that supports SBAT metadata ( grub2, fwupd, fwupdate, shim + all child shim binaries )?
### Please provide exact SBAT entries for all SBAT binaries you are booting or planning to boot directly through shim.
### Where your code is only slightly modified from an upstream vendor's, please also preserve their SBAT entries to simplify revocation.
*******************************************************************************
Besides being signed with our keys, We intend to leave our grub2 and fwupd source code completely unchanged from the upstream Rocky (RHEL) versions, as we have no need to customize it beyond that.

Please, paste the exact entries, which your binaries include, right here in this section. They are required for the application to be reviewed.

Also, a hint: watch out to provide the entries straight from the binaries, rather than e.g. GRUB2 containing the @@VERSION@@ variable, which is interpolated during the RPM building process.

You can get them by running objcopy similarly like in a listing I wrote above in this comment.

Which modules are built into your signed grub image?

I'll check this out thoroughly in the future - please, ping me on this once new changes are introduced to the current application.

*******************************************************************************
### What kernel are you using? Which patches does it includes to enforce Secure Boot?
*******************************************************************************
We are using our RHEL upstream variants: 4.18 and 5.14 with minor patches (on top of the many patches from Red Hat and others).

We are also building and packaging supported upstream kernels designed for use on Rocky and enterprise-Linux variants.  These include supported LT versions (5.4, 5.10, 5.15, 6.1), as well as the rollling latest-stable version.

I understand that these all enforce secure boot "out of the box".

There were some ongoing discussions in this regard, e.g. if it's possible to kexec into an unsigned kernel, even though the whole bootchain appears to be prepared for UEFI Secure Boot.

I kindly request @mheese to help me out with this question.

mheese commented 1 year ago

What kernel are you using? Which patches does it includes to enforce Secure Boot?

We are using our RHEL upstream variants: 4.18 and 5.14 with minor patches (on top of the many patches from Red Hat and others).

We are also building and packaging supported upstream kernels designed for use on Rocky and enterprise-Linux variants. These include supported LT versions (5.4, 5.10, 5.15, 6.1), as well as the rollling latest-stable version.

I understand that these all enforce secure boot "out of the box".



There were some ongoing discussions in this regard, e.g. if it's possible to `kexec` into an unsigned kernel, even though the whole bootchain appears to be prepared for UEFI Secure Boot.

I kindly request @mheese to help me out with this question.

I highly doubt that these problems exist with the RHEL upstream Linux kernels. Essentially, there are two things one needs to confirm with the kernels:

jason-rodri commented 1 year ago

Concerning our public keys: Michael extended his expiration date. I was able to verify that the new expiration date is Sun Sep 28 16:22:24 EDT 2025. Is there an issue with resetting key expiration dates?

jason-rodri commented 1 year ago

Question about commit eadb2f47a3ced5c64b23b90fd2a3463f63726066

After some searching I was able to find CVE-2022-21499. The CVE "kernel: possible to use the debugger to write zero into a location of choice" was applied to RHEL kernels.

jason-rodri commented 1 year ago

@mheese is their a list of secure boot patches? Are the 3 patches in the questionnaire the patches you are referring to? Or are there other patches? Asking so we can verify if we have all the secure boot patches applied to our kernels.

Is their a patch for the "kexec" issue? Just trying to see how we can ensure we remedy the issue.

mheese commented 1 year ago

@mheese is their a list of secure boot patches?

Unfortunately, not that I am aware of. I really wish there would be. The questionnaire is pretty much just referring to previous vulnerabilities and that they are fixed. I'm going to raise that point with the committee.

Are the 3 patches in the questionnaire the patches you are referring to? Or are there other patches?

No, I just found out this week that the Debian 12 (bookworm) kernel which is based on 6.1.x still has some additional secure boot related patches which are not in upstream yet. I will include them now myself in my own builds - because my kernel is not based on a RHEL or Debian upstream version. In your case, you are most likely okay as you are using a RHEL upstream version.

fyi, these patches is what I'm talking about (from http://deb.debian.org/debian/pool/main/l/linux/linux_6.1.55-1.debian.tar.xz):

# Lockdown missing pieces
features/all/lockdown/efi-add-an-efi_secure_boot-flag-to-indicate-secure-b.patch
features/all/lockdown/efi-lock-down-the-kernel-if-booted-in-secure-boot-mo.patch
features/all/lockdown/mtd-disable-slram-and-phram-when-locked-down.patch
features/all/lockdown/arm64-add-kernel-config-option-to-lock-down-when.patch

Asking so we can verify if we have all the secure boot patches applied to our kernels.

I know, we really need to come up with a checklist of some sort.

Is their a patch for the "kexec" issue? Just trying to see how we can ensure we remedy the issue.

That is pretty much just a kernel configuration setting. You should enable lockdown (one of the patches above actually will enforce it anyways if secure boot is enabled), and require signatures for linux images with kexec (CONFIG_KEXEC_SIG_FORCE if I recall correctly).

jason-rodri commented 12 months ago

We have updated the README.md questionnaire with the requested information.

aronowski commented 12 months ago

Thanks for the updates!

Michael extended his expiration date. I was able to verify that the new expiration date is Sun Sep 28 16:22:24 EDT 2025. Is there an issue with resetting key expiration dates?

As far as I can see, only a public subkey's date got extended. The public key, to which that subkeys is bound to, is still expired.

We are also including the Buggy binutils patch as well.
Buggy binutils patch: https://bitbucket.org/ciqinc/shim-unsigned-x64/src/ciq8/SOURCES/buggy-binutils.patch
The patch remedies a compatibility issue with binutils versions prior to 2.36. (https://github.com/rhboot/shim/issues/533)

That's good, thanks!

Yes, the global SBAT generation on our GRUB binary has been set to 3

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,3,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.rhel8,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:secalert@redhat.com
grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:security@rockylinux.org

The update announcement is OK, but the SBAT entry looks suspicious - more on this in a moment.

After some searching I was able to find CVE-2022-21499. The CVE "kernel: possible to use the debugger to write zero into a location of choice" was applied to RHEL kernels.

Thanks, investigating it.

* SHA256 (shimx64.efi) = 8f0d4cdae78a9c404ea70ff9a36189067dfa646aa3368d472e7782003d30a969
* SHA256 (shimia32.efi) = c267adea2ad49ac3b2d595d3e7cf597a5caab8206415df748a5f7770be8c1a3f

Checksums fixed, OK!

We use a managed PKI solution that meets all industry standards and requirements for issuing, protecting, backing up a
d securing code signing certs.

There is a Private Root CA and a Private Issuing CA.  The Private Issuing CA was used for issuing of the private code
igning certs that are found in the SHIM.

Those issued certs are then stored on a physical HSM.  That HSM is installed within a FIPS environment.  All access to
that environment is strictly controlled with physical and logical controls in place, with no outside access permitted.
 The servers are in a locked environment and within a secure data center with proper physical access controls in place
at that location for security purposes.

Explained in detail, thanks!

objcopy --only-section .sbat -O binary grubx64.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,3,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.rhel8,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:secalert@redhat.com
grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:security@rockylinux.org

objcopy --only-section .sbat -O binary grubia32.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,3,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.rhel8,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:secalert@redhat.com
grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:security@rockylinux.org

objcopy --only-section .sbat -O binary fwupdx64.efi /dev/stdout
sbat,1,UEFI shim,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
fwupd-efi,1,Firmware update daemon,fwupd-efi,1.3,https://github.com/fwupd/fwupd-efi
fwupd-efi.rhel,1,Red Hat Enterprise Linux,fwupd,1.7.8,mail:secalert@redhat.com
fwupd-efi.rocky,1,Rocky Linux,fwupd,1.7.8,mail:security@rockylinux.org

objcopy --only-section .sbat -O binary  shimx64.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.7,mail:it_security@ciq.com

objcopy --only-section .sbat -O binary shimia32.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.7,mail:it_security@ciq.com

At least the GRUB2-related SBAT entries seem wrong.

Not saying that they are, but I need to dive deeper into this: the grub.rhel8 component name is actually correct as it did exist some time ago, even if nowadays the binary shipped by Red Hat, Inc. has the grub.rh component name.

However, to give a detailed answer on this, I need to study the history of the changes; what was happening back then and why certain values are used today. This applies at least to the component names and generation numbers.

While the grub2,2.02-148.el8_ciq.1.rocky.0.3 entry used as vendor package name in the grub.rocky8 line seems OK, why is it used in a line above that one too? Does Red Hat, Inc. ship packages tailored to RESF/CIQ or something?

I'll leave the bug label until the GPG-related thing gets resolved.

In the meantime, please, provide as much information as possible to make my investigation easier. Especially when it comes to the grub.rhel8 vs grub.rh thingy.

jason-rodri commented 12 months ago

Thank you for your time and guidance! We will definiately look into the pub key issue.

I was looking into the sbat issue and you you can see that the sbat from rocky grub2 repo lists grub.rhel8

I hope that will clear things up

jason-rodri commented 12 months ago

After looking at the sbat more closely I see what you are saying about the grub.rhel8 entry. I will look into remedy the issue.

jason-rodri commented 12 months ago

I am uploading an image of the objcopy output from rocky grub on my test system. You can see that the rhel line lists rocky as well.

image

elguero commented 12 months ago

Michael extended his expiration date. I was able to verify that the new expiration date is Sun Sep 28 16:22:24 EDT 2025. Is there an issue with resetting key expiration dates?

As far as I can see, only a public subkey's date got extended. The public key, to which that subkeys is bound to, is still expired.

I wonder what I am missing. I just ran the following inside of a container:

[root@USITL8XDQSQ3 /]# gpg --keyserver keyserver.ubuntu.com --search-keys myoung@ciq.com
gpg: directory '/root/.gnupg' created
gpg: keybox '/root/.gnupg/pubring.kbx' created
gpg: data source: http://185.125.188.27:11371
(1) Michael L. Young <myoung@ciq.co>
    Michael L. Young <myoung@ciq.com>
      3072 bit RSA key D84A6A5913926D2B, created: 2021-09-27
Keys 1-1 of 1 for "myoung@ciq.com".  Enter number(s), N)ext, or Q)uit > 1
gpg: /root/.gnupg/trustdb.gpg: trustdb created
gpg: key D84A6A5913926D2B: public key "Michael L. Young <myoung@ciq.co>" imported
gpg: Total number processed: 1
gpg:               imported: 1

[root@USITL8XDQSQ3 /]# gpg --list-keys myoung@ciq.com
pub   rsa3072 2021-09-27 [SC] [expires: 2025-09-28]
      CD8298087BCAC022B5EC84FAD84A6A5913926D2B
uid           [ unknown] Michael L. Young <myoung@ciq.co>
uid           [ unknown] Michael L. Young <myoung@ciq.com>
sub   rsa3072 2021-09-27 [E] [expires: 2025-09-28]

Thank you for your help.

SherifNagy commented 12 months ago 
objcopy --only-section .sbat -O binary grubx64.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,3,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.rhel8,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:secalert@redhat.com
grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:security@rockylinux.org

objcopy --only-section .sbat -O binary grubia32.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,3,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.rhel8,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:secalert@redhat.com
grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8_ciq.1.rocky.0.3,mail:security@rockylinux.org

objcopy --only-section .sbat -O binary fwupdx64.efi /dev/stdout
sbat,1,UEFI shim,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
fwupd-efi,1,Firmware update daemon,fwupd-efi,1.3,https://github.com/fwupd/fwupd-efi
fwupd-efi.rhel,1,Red Hat Enterprise Linux,fwupd,1.7.8,mail:secalert@redhat.com
fwupd-efi.rocky,1,Rocky Linux,fwupd,1.7.8,mail:security@rockylinux.org

objcopy --only-section .sbat -O binary  shimx64.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.7,mail:it_security@ciq.com

objcopy --only-section .sbat -O binary shimia32.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.7,mail:it_security@ciq.com

At least the GRUB2-related SBAT entries seem wrong.

Not saying that they are, but I need to dive deeper into this: the grub.rhel8 component name is actually correct as it did exist some time ago, even if nowadays the binary shipped by Red Hat, Inc. has the grub.rh component name.

However, to give a detailed answer on this, I need to study the history of the changes; what was happening back then and why certain values are used today. This applies at least to the component names and generation numbers.

While the grub2,2.02-148.el8_ciq.1.rocky.0.3 entry used as vendor package name in the grub.rocky8 line seems OK, why is it used in a line above that one too? Does Red Hat, Inc. ship packages tailored to RESF/CIQ or something?

I'll leave the bug label until the GPG-related thing gets resolved.

In the meantime, please, provide as much information as possible to make my investigation easier. Especially when it comes to the grub.rhel8 vs grub.rh thingy.

Actually that's a good point and I think it's a good area to improve the documentations abit, as far as I recall, when Rocky did apply for shim the name of RHEL binary was grub.rhel8, we were asked to include it in our SBAT and then it changed later "We didn't update our SBAT entry for grub2 since then but we are aware of this for our next grub release", however if any other vendors are rebuilding packages from Rocky as upstream for some reason, I assume they should have the 3 entries here for grub2 and fwupd:

I think an entry example should be something like this:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md grub,3,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/ grub.rh,Red Hat Enterprise Linux 8,grub2,2.02-148.el8 SHOULD BE RHEL's DIST TAG / PKG NAME,mail:secalert@redhat.com grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8 SHOULD BE ROCKY's DIST TAG / PKG NAME,mail:security@rockylinux.org grub.CIQ,1,CIQ's build of Rocky Linux 8, grub2,2.02-148.el8 AND WHATEVER CIQ DIST TAG,mail:SECURITY@CIQ.COM

PS caps letter can and should be changed to match the correct values

Even if it just a rebuild, one of those vendors might start carrying different patches in grub or fwupd or whatever EFI binary that might be allowed to load in the future. I can't think of any other reasons to include upstream vendor's binary's other than this and maybe prevents loading vulnerable version of upstream EFI binaries that doesn't match the correct SBAT level for that upstream. Not sure if I can make my point across though, it's too early in the morning here , at least for me :smile:

thoughts?

aronowski commented 11 months ago

@elguero, the process now works fine for me - confirmed with your listing. Thank you.

I'll send verification emails soon.

@SherifNagy, thanks for the clarification and the example.

I'd opt for keeping up with upstream entires, but I suspect some even more clarifications shall be made in the future. PR #348 is a good starting point.

The example has one error - there's no generation number after grub.rh,. But apart from that, it does its job just fine.

In regard to the build process of GRUB2, I'd like to clarify what happens. The @@VERSION_RELEASE@@ variable located in the sbat.csv.in file gets interpolated during the build process and the resulting output is, for instance, 2.02-148.el8.rocky.0.3 for Rocky Linux.

Therefore, let's keep in mind that the build process will replace upstream versions with downstream versions, which will be incorrect, whenever a downstream distro appends their own disttag. Most Enterprise Linux distros do so to distinguish between packages with downstream branding or other changes.

As a hint, I'd suggest basing the implementation on the one by AlmaLinux. See for yourself, how the great folks behind this project did it:

$ git clone https://git.almalinux.org/rpms/grub2.git alma8-grub2
$ cd alma8-grub2/
$ git diff af7d0b5667836089ac4c3ed831979ad4a43ce7ab 835ebbe2c2bc0eadd4e30b233ecb50cbdfbdf349 -- SOURCES/sbat.csv.in SPECS/grub2.spec
aronowski commented 11 months ago

Verification emails sent.

jason-rodri commented 11 months ago

Verification emails sent.

equiomnipotent cyphering rumpling talbotype sidles aeolus goose-stepping Erenburg turdine antimonarchists

jason-rodri commented 11 months ago

We have been investigating the grub sbat issues @aronowski brought to our attention. The resolution was to update the sbat.csv.in and SPEC file to generate the appropriate versions for the upstream lines. We also added our own ciq line to the sbat. Below you can see our latest output. If the output is correct we will update the questionnaire.

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md grub,2,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/ grub.rhel8,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8.1,mail:secalert@redhat.com grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8.1.rocky.0.3,mail:security@rockylinux.org grub.ciq_rocky8,2,Rocky Linux 8 (CIQ modified),grub2,2.02-148.el8_CtrlIQ.1.ciq.0.3,mail:secureboot@ciq.com `

elguero commented 11 months ago

Verification emails sent.

cotidal flectionless gurglet whauve trinitrate diapir Kaimo bronchocele cabinlike christine

aronowski commented 11 months ago

The words match the ones I sent - contacts have been verified!

@jason-rodri, Almost there! There are some curiosities that I'd like to address, though.

For context, let's focus on Rocky's 15.6 application, in particular on the SBAT section.

For the Red Hat, Inc. entry I'd preserve the one they currently ship, that is

grub.rh,2,Red Hat,grub2,2.02-148.el8_8.1,mailto:secalert@redhat.com

so the whole hierarchy stays faithfully represented. Though for that I'd need to contact folks from RESF, I think. @SherifNagy, please, help me out with this.

Now, when it comes to Rocky's and Ctrl IQ's entries:

grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8.1.rocky.0.3,mail:security@rockylinux.org
grub.ciq_rocky8,2,Rocky Linux 8 (CIQ modified),grub2,2.02-148.el8_CtrlIQ.1.ciq.0.3,mail:secureboot@ciq.com

While right here Rocky's entry is fine, at least the Ctrl IQ's generation number is wrong - it should be 1 rather than 2 under the assumption that this is the first time Ctrl IQ's GRUB2 is derived from Rocky in the context of the SBAT ecosystem.

Unless I'm wrong and it's justified, e.g. with number 1 being there earlier and Ctrl IQ having addressed a security issue that required that number to be bumped, but it was because of a downstream bug - one that affects Ctrl IQ's custom builds and doesn't affect Rocky's builds.

Furthermore, is there a need to use grub.ciq_rocky8 rather than just grub.ciq? Not that it can't stay in that form, just asking.

jason-rodri commented 11 months ago

Lets try this again:

Here is the updated sbat from the grub binary

bat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,2,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.rhel8,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8_8.1,mail:secalert@redhat.com
grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8_8.1.rocky.0.3,mail:security@rockylinux.org
grub.ciq_rocky8,1,Rocky Linux 8 (CIQ modified),grub2,2.02-148.el8.1.ciq.0.3,mail:secureboot@ciq.com

We replicated the rhel line to preserve the original grub sbat. As well as the rocky line. We would like to keep the ciq_rocky8. We did correct the generation number. Thank you for pointing that out.

aronowski commented 11 months ago

Thank you, Jason. The entry looks better now!

Although I'm still unsure if leaving the grub.rhel8 name instead of grub.rh would be the right thing to do - I'm afraid of the issues this might cause in the future, when grub.rh,3 (generation number 3) will be required for a successful boot, that is if the grub.rh,2 one lands into a always-deny list someday.

I'll try to bring this topic up during the next meeting.

In the meantime I'm trying to take a deeper dive into the curiosities I was mentioning earlier, that I'll be investigating. There are some hints, but no specifics, so I don't have an easy time in here.

jason-rodri commented 11 months ago

@aronowski Thank you again for pointing out an issue I will update the rhel grub entry to reflect the suggestion of grub.rh.

I would like to recap the issues that need more investigation.

aronowski commented 11 months ago

@aronowski Thank you again for pointing out an issue I will update the rhel grub entry to reflect the suggestion of grub.rh.

Thanks! Waiting for the change to be introduced.

I would like to recap the issues that need more investigation.

Same. There will be certain puzzles here for which I'll need help and those, which I won't be able to solve on my own. More on this in a minute.

* Which modules are built into your signed grub image?

  * Please let us know if we can help in any way. We are building the grub2-efi-x64-modules rpm and can list the modules being built. The modules should not be any different than the ones rocky builds since we are basically just recomping rocky source with the sbat updates you have pointed out and our cert.

I believe that this question may be one of the many instances of being ambiguous. Something I brought up here with the Improving the document for a review application section.

If so, I think it should be rewritten for clarity and brought up during the next meeting. Here are my ideas, why that might be. Keep in mind that these are written by someone, who uses GRUB2 as a tool, rather than being a developer that knows it inside-out.

We might base the answer on the grub.macros file, which defines grub_modules, efi_modules and platform_modules. Then the answer seems OK. And we could statically analyze the final binary for mentions of the modules, for instance with the strings program.
Though I'd kindly request some help, for instance regarding the btrfs module - there are references to its implementation, but I couldn't find, how the final binary uses it as a builtin module.

However, on the other hand, we could also mention the .mod files that reside in a filesystem, provided by packages like, for Rocky 8, grub2-efi-x64-modules-2.02-148.el8_8.1.rocky.0.3.noarch.rpm. While they are not, as the question says,

built into your signed grub image

, I believe this might be the case where some people may still get confused the same way as misinterpreting the question about global SBAT generation number in my comment I linked to above. We're humans, we make mistakes and may get overwhelmed by such a huge amount of work, so it comes as of no surprise to me.

* Which patches are needed by the kernel?

  * I know the shim development team is also looking into this issue.

Am i missing any outstanding issues?

In regard to the lockdown patches required for certain kernels, then yes, discussions are present, but I believe that in this case we can refer to an authority like Red Hat, Inc., since they're a prominent software vendor and do know, how this matter shall be handled in their products. And since Rocky Linux is a solid RHEL replica, I'm not worried here.

The only thing that may matter is using a different configuration to the one from the kernel RHEL provides. We've been having discussions about out-of-tree modules and have a relevant ticket here.

Now, in regard to the CVE-2022-21499 fix, the relevant commit that fixes this issue is eadb2f47a3ced5c64b23b90fd2a3463f63726066 (I'm linking to a unified diff on GitHub for a pleasant visual presentation).

As far as I'm aware, this code is not present in the kernels RHEL provides. That's because there's appropriate config, that disables the functionality, which would be introducing a security issue, which this commit fixes. Therefore it's simply not required in RHEL-provided kernels.

However, I understand that there might be an ambiguity, considering the information on Red Hat's website, along with the kernel's changelog in RHEL-provided specfile, which lists:

* Mon Aug 15 2022 Jarod Wilson <jarod@redhat.com> [4.18.0-418.el8]
[...]
- lockdown: also lock down previous kgdb use (Lenny Szubowicz) [2104748] {CVE-2022-21499}
[...]

I thought about checking out the appropriate entry at bugzilla.redhat.com, but the server doesn't authorize my Fedora Account to see, what was happening there. There's a message:

You are not authorized to access bug #2104748.

Most likely the bug has been restricted for internal development processes and we cannot grant access. [...]

Therefore, at this point I suppose this is something akin to a trade secret or non-disclosure agreement and even if I could preview that entry, I still wouldn't be allowed to talk about it publicly.

If that understanding is correct, there's nothing we can do about it, apart from getting the required authority, e.g. by applying for a job at Red Hat, Inc.

If it's not correct, and the people, who have been working on this fix, are allowed to talk about it publicly or privately (outside of the scope of their job), then you could try pinging the people here or, if feeling adventurous, emailing them - the latter idea might not be the most optimal one as I've tried this once and most likely there's a filter, which treats such emails as spam.

There are some hints that we could base our reasoning on, though. Not that they would provide us a definite answer, but may shed some light on the matter and tell us, if there really is anything to worry about.

So unless a Red Hat employee can enlighten me, I don't think I can provide more help here.

jason-rodri commented 11 months ago

Grub2 modules:

The modules that actually go into the Grub EFI binary are listed in our sources:

The RPM build we're using actually builds the full range of grub2 modules as a separate package, in case downstream users want to customize it themselves in some way. But our signed EFI binary includes only those listed modules from the macro file.

Updated SBAT:

That's good advice around the SBAT entries, I think we've finally settled on a cleaner one that will work (with the proper upstream RH entries):

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,2,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.rh,2,Red Hat Enterprise Linux 8,grub2,2.02-148.el8_8.1,mail:secalert@redhat.com
grub.rocky8,2,Rocky Linux 8,grub2,2.02-148.el8_8.1.rocky.0.3,mail:security@rockylinux.org
grub.ciq_rocky8,1,Rocky Linux 8 (CIQ modified),grub2,2.02-148.el8.1.ciq.0.3,mail:secureboot@ciq.com

Our templated sbat file produces this for the build:

SecureBoot Kernel Patches:

Still not sure about this one, as our RHEL/Rocky 8 base seems to have all the necessary secure-boot related patches included. Looking for further guidance here, or anything we can do to help clarify/confirm with you!

aronowski commented 11 months ago

Everything has been thoroughly explained and the updated SBAT entries look good!

In regard to the kernel patches, I would, as I wrote in an earlier comment, refer to an authority like Red Hat, Inc. - therefore, treating the existing kernel configuration as sufficient, unless there's a breakthrough and additional requirements get introduced.

I took a quick look at the current changes with git diff ctrliq-shim-x64-ia32-20231016 ctrliq-shim-x64-ia32-20231027 and the current revision seems alright to me.

Great job! Let's wait for another official review and let me wish you all the best!

SherifNagy commented 11 months ago

While I am not an official reviewer, here are my comments "after all the back and forth with @aronowski " :) :

I just have couple of questions for clarification, I see in your answers, you mention RHEL / Rocky 8 + 9 kernel version and grub version couple of times, are you planning to release this SHIM for both releases?

second question might be more for @aronowski "since I am still learning myself" , since CIQ's is going full secure boot chain away from Rocky, I think the fwupd-efi SBAT entry will require a CIQ entry if I am not mistaken, is that correct?

objcopy --only-section .sbat -O binary fwupdx64.efi /dev/stdout 
sbat,1,UEFI shim,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
fwupd-efi,1,Firmware update daemon,fwupd-efi,1.3,https://github.com/fwupd/fwupd-efi
fwupd-efi.rhel,1,Red Hat Enterprise Linux,fwupd,1.7.8,mail:secalert@redhat.com
fwupd-efi.rocky,1,Rocky Linux,fwupd,1.7.8,mail:security@rockylinux.org

Other thank this, everything looks good to me

SherifNagy commented 11 months ago

For preserving upstream's SBAT entries in fwupd, I got two answers:

So I guess we can leave this up to the distros? still need confirmation about this.

Now, MSFT just released a exception for NX support https://techcommunity.microsoft.com/t5/hardware-dev-center/nx-exception-for-shim-community/ba-p/3976522#M147 which means, I don't think they will sign a SHIM that has NX flag enabled while the rest of the stack isn't "that's just my guess" also need someone else to confirm this

One question remaining, is about the intention of using this shim review for modified Rocky 8 and 9 or just 8

jason-rodri commented 11 months ago

Thank you for reviewing our submission!

We are very focused on a modified Rocky 8. We'd like to be approved for 9 as well (as a Rocky derivative, similar to 8 ). But if that takes another submission, that's fine - we at CIQ are good either way.

We will leave the NX support in for now and see what guidance comes down from Committee or development group.

jason-rodri commented 11 months ago

If we have to generate a shim without NX support should that be done in this review or will need to generate an additional review?

jason-rodri commented 10 months ago

I have update our submission with a shim that does not have the NX patch.

I have also updated/removed the references to the NX patch in the README.md

Please let me know if this is not the correct protocol.

Thank you in advance

elguero commented 10 months ago

Hi @aronowski ,

Thank you for all the assistance that has been provided on the initial review.

Is there anything that we need to provide or do on our side to help get to the next stage of review?

Thanks!

aronowski commented 10 months ago

@elguero, I believe the best bet is to ping other reviewers from the committee.

It's worth to try and I wish people respond sooner than later, but considering how much is going on at the moment, both in professional (the NX support thread) and personal (obligations outside of jobs and the volunteer work) settings, I can't promise this. I tried to analyze things thoroughly and did my best, but can't duplicate myself to start either from scratch or from what already has been posted.

For instance, the fact that I'm replying here 5 days after being pinged is not out of malice, but instead because last week I've been getting little to no sleep and got overwhelmed with traveling across the country to clean up certain legal things, therefore volunteer work in these conditions is not optimal, to say the least.

Furthermore, considering the latest news, there may soon be the need to remove the NX compatibility patch. For that, replying to @jason-rodri, I think there's no need to open up another application, but instead, simply modify this one. The shim binary checksum will change, but the rest of the review should be fine. Maybe just mention at the very bottom of the application, what's the deal with no NX compatibility - so when people from Microsoft take a look at it, they will immediately know, what's going on.

jason-rodri commented 10 months ago

@elguero, I believe the best bet is to ping other reviewers from the committee.

It's worth to try and I wish people respond sooner than later, but considering how much is going on at the moment, both in professional (the NX support thread) and personal (obligations outside of jobs and the volunteer work) settings, I can't promise this. I tried to analyze things thoroughly and did my best, but can't duplicate myself to start either from scratch or from what already has been posted.

For instance, the fact that I'm replying here 5 days after being pinged is not out of malice, but instead because last week I've been getting little to no sleep and got overwhelmed with traveling across the country to clean up certain legal things, therefore volunteer work in these conditions is not optimal, to say the least.

Furthermore, considering the latest news, there may soon be the need to remove the NX compatibility patch. For that, replying to @jason-rodri, I think there's no need to open up another application, but instead, simply modify this one. The shim binary checksum will change, but the rest of the review should be fine. Maybe just mention at the very bottom of the application, what's the deal with no NX compatibility - so when people from Microsoft take a look at it, they will immediately know, what's going on.

@aronowski Thank you for your response, it seems like we have made you our shim review lifeline. You are just one of few, thank you again for all your hard work and dedication. I completely empathize with the issues you listed and wish you the best of luck. Thank you for the suggestion of tagging other reviewers.

jason-rodri commented 10 months ago

@THS-on @steve-mcintyre

We have updated our submission by removing the nx patch from the shim build. The removal of the NX patch was prompted by the lack of NX support from our grub and kernel offerings. The nx patch removal was also triggered by the nx exception issued by Microsoft and shim development group recommendation.

@aronowski has reviewed and approved our submission. Can we get a second set of eyes on our submission.

aronowski commented 10 months ago

@jason-rodri, I can see the work is being done on the nonx-update branch.

The binary reproduces fine, but there are still the old checksums written in the What is the SHA256 hash of your final SHIM binary? section. Please, update these and let me know, so I can double-check if everything's OK - once it is, simply tag the final revision appropriately and update the GitHub issue initial post.

jason-rodri commented 10 months ago

@aronowski Thank again! I have updated the hashes of the shims in the "What is the SHA256 hash of your final SHIM binary".

aronowski commented 9 months ago

@jason-rodri, it looks like only the original post received this update. I can't see the change being introduced to the Bitbucket repository.

jason-rodri commented 9 months ago

@aronowski Thank you again, The hashes documented in the Readme.md and in the issue should match the checksums in the shim_rpmbuild.log now.

aronowski commented 9 months ago

Great job, the checksums match!

Now, you'll need to wait for another official review, while keeping an eye on the news, in case something like the NX support was to change again.

I can't duplicate myself to write another review, but can try my best to answer questions regarding these aforementioned news, in case some arise.

jason-rodri commented 9 months ago

@THS-on @steve-mcintyre @dennis-tseng99 , Happy new year.

Can we get an estimate on when we will be assigned an additional reviewer?

@aronowski has completed their review and everything seems to be in order.

Thank you in advance!

THS-on commented 9 months ago

My schedule is currently pretty full. I can likely have a look it after FOSDEM in February.

jason-rodri commented 9 months ago

@THS-on Thank you for the reply. Appreciated all the hard work you contribute to review process! Looking forward to your review inputs.

jason-rodri commented 8 months ago

I want to make sure the review team was not waiting on anything from us, since The question tag was still applied.

aronowski commented 8 months ago

Not anymore. ;-)

At least to my current understanding, that is. If the requirements change, another one will be added.

aronowski commented 8 months ago

I suppose we can close this one, as there's the updated 15.8 application?