search

rhboot / shim-review

Reviews of shim
66 stars 128 forks source link

Ctrl IQ, Inc EL7 Shim 15.8 for x64 & ia32 #430

Open jason-rodri opened 2 months ago

jason-rodri commented 2 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/tree/ciqliq-shim-EL7-x64-ia32-20240903

What is the SHA256 hash of your final SHIM binary?

SHA256 (shimx64.efi) = 088610925c2491017f6488f6235c6daec4e7f567dfb6c4e8c55d64d6acaafbae SHA256 (shimia32.efi) = 14822c87e48f9ca65df08a4595ffa8cc6a7564197826521318488178fdf16272

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 Ctrl IQ, Inc EL9 Shim 15.8 for x64 #420

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

dennis-tseng99 commented 2 months ago

review for ciqliq-shim-EL7-x64-ia32-20240702

Pesign checks ::
/shim_result/usr/share/shim/15.8-0.el8/ia32/shimia32.efi 6a87a9f09a15d9da739fa6861ce633308fc7135a74d6a599cbb12b8e93379f0b
/shimia32.efi 6a87a9f09a15d9da739fa6861ce633308fc7135a74d6a599cbb12b8e93379f0b
/shim_result/usr/share/shim/15.8-0.el8/x64/shimx64.efi f1cf5514c601e7abfcfd72b6e2522831761e0396093e6fe92fe7cf0682731a2f
/shimx64.efi f1cf5514c601e7abfcfd72b6e2522831761e0396093e6fe92fe7cf0682731a2f

Removing intermediate container ded74f8d9ca2
 ---> 1c00ef706747
Successfully built 1c00ef706747
#sha256sum shimx64.efi
654d8efe248cd113f7ecb5a1f4fc9c309cc0d65a66b4bb8d9b2991f57f2dbcf6  shimx64.efi
#sha256sum shimia32.efi 
b739423471c03d32f2918906286076ea73c1385ced3f175a60ceeb8fadf009de  shimia32.efi
#objdump -x shimx64.efi | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000

#objdump -x shimia32.efi | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000
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,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.8,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,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.8,mail:it_security@ciq.com

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/
....
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            7c:51:d5:cc:c2:fd:b3:32:81:0d:51:36:18:fc:4d:01
        Signature Algorithm: sha384WithRSAEncryption
        Issuer: C = US, ST = Nevada, L = Reno, O = "Ctrl IQ, Inc.", CN = "Ctrl IQ, Inc. Root CA"
        Validity
            Not Before: Apr 21 15:09:30 2023 GMT
            Not After : Apr 20 23:59:59 2048 GMT
        Subject: C = US, ST = Nevada, L = Reno, O = "Ctrl IQ, Inc.", CN = "Ctrl IQ, Inc. Issuing CA"
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (4096 bit)
For example:
Dockfile in document:
FROM centos:centos7.9.2009
ENV shim_release 15.8-0.el7

Dockfile for real build:
FROM rockylinux:8.8.20230518
ENV shim_release 15.8-0.el8
jason-rodri commented 2 months ago

@dennis-tseng99 Thank you for the review!

Can you please elaborate on the following comment?

  • Comments: It seems that the document you provided is too old in here
For example:
Dockfile in document:
FROM centos:centos7.9.2009
ENV shim_release 15.8-0.el7

Dockfile for real build:
FROM rockylinux:8.8.20230518
ENV shim_release 15.8-0.el8

From what I understand I am building the shim in a centos 7 environment, because the shim is for centos 7. The build log show mock building the shim in a centos 7 environment, correct? Did you see the build the shim being built in a rocky 8.8 env or do you think the shim should be built in a rocky 8.8 environment?

Thank you again for the review and I look forward to your clarification.

SherifNagy commented 2 months ago

@jason-rodri I haven't looked at the review yet, but I think what @dennis-tseng99 means is located here https://github.com/ctrliq/ciq-shim-build/blob/ciqliq-shim-EL7-x64-ia32-20240702/mock-build/ciq_mock_rocky8_static_shim.cfg which what used to build and provide build.log I assume?

However, I have some few questions, now that centos7 is EOL:

jason-rodri commented 2 months ago

@jason-rodri I haven't looked at the review yet, but I think what @dennis-tseng99 means is located here https://github.com/ctrliq/ciq-shim-build/blob/ciqliq-shim-EL7-x64-ia32-20240702/mock-build/ciq_mock_rocky8_static_shim.cfg which what used to build and provide build.log I assume?

However, I have some few questions, now that centos7 is EOL:

  • Is CIQ going to maintain and keep patching kernels, grub, etc... for security patches from upstream?

We do plan to maintain and update security patches for centos EL7 including kernel and grub2.

  • Why are you still using rocky under /boot/efi directory?

This was an oversight on my part. I did not update the mock-build directory, the files were still from our EL8 submission. The mock-build directory should now be updated with the correct logs

  • What's your plan for NX for your boot chain for el7?

When NX is approved we will attempt to patch the 3.10 kernel to enable NX support. If it is not feasible to add NX support to 3.10 we will sign a ML 6.X kernel which does support the NX feature

  • Why not just use any of your current signed shim for this release?

We were under the impression that we needed to submit a separate shim request for each EL version we support and could not reuse our EL8 submission for EL7 and EL9. If this is not the case please let us know.

Thank you again @dennis-tseng99 and @SherifNagy for the assistance and pointing out my oversight

SherifNagy commented 2 months ago
  • Why not just use any of your current signed shim for this release?

We were under the impression that we needed to submit a separate shim request for each EL version we support and could not reuse our EL8 submission for EL7 and EL9. If this is not the case please let us know.

The reuse of already signed shim between releases is mainly a vendor choice, personally, I prefer the builds against each release "shim for rocky9 is build on rocky9 and so on ", RHEL, Oracle, Debian, Rocky Linux and others do that, however, I think now ubuntu moved to single shim for all releases and I think fedora do the same since they release twice a year and there might be only one shim release a year for now, it really depends on the vendor's preference / internal polices.

Thanks for the clarification, I will look into the submission early next week

SherifNagy commented 1 month ago

Review of ciqliq-shim-EL7-x64-ia32-20240705

Shim

Shim Comparison, original binary vs. freshly built binaries:

SHA256 sums :: 088610925c2491017f6488f6235c6daec4e7f567dfb6c4e8c55d64d6acaafbae /shimx64.efi 088610925c2491017f6488f6235c6daec4e7f567dfb6c4e8c55d64d6acaafbae /shim_result/usr/share/shim/15.8-0.el7/x64/shimx64.efi 14822c87e48f9ca65df08a4595ffa8cc6a7564197826521318488178fdf16272 /shimia32.efi 14822c87e48f9ca65df08a4595ffa8cc6a7564197826521318488178fdf16272 /shim_result/usr/share/shim/15.8-0.el7/ia32/shimia32.efi

Binary compare (blank output means binaries are the same) ::

Pesign checks :: hash: f6a7e7954e6f935dba9765d9a9393acc1d486a5a9d4738f9699ccb28e47f03e4 hash: f6a7e7954e6f935dba9765d9a9393acc1d486a5a9d4738f9699ccb28e47f03e4 hash: cc849c210c71cbb6d4a4152f5a50ddfcbf949aade1a49834ce070d745be05d8a hash: cc849c210c71cbb6d4a4152f5a50ddfcbf949aade1a49834ce070d745be05d8a

COMMIT --> db6d89112705 db6d891127050389a495fd91b2204010b41e11746b06b298baf0d02d27217997



* 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 centos  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 Centos kernel)

## Note
I would be slightly worried to allow running Centos un-patched / unmaintained grub2 and kernels using the Centos's CA via certwrapper, this would make the OS vulnerable even with secureboot enable.

LGTM
ghost commented 1 month ago

I'm sorry for using my TMP account, but this shim should not be accepted. The fact that it went this far without anyone's massive criticism casts doubt on the community review process.

It's well known that any not NX-enabled bootchain is not review or signed for more than 18 months. Multiple vendors had to recreate their submissions, and multiple submissions were closed because of this requirement.

https://github.com/rhboot/shim-review/issues/307

- The "will attempt to patch" NX support is not an option.

Considering this submission is not only unfair to all other vendors who had to restart their entire process, but it also reflects a lack of basic integrity in the process and the reviewers involved in this review.

The other red flags include:

steve-mcintyre commented 1 month ago

It's well known that any not NX-enabled bootchain is not review or signed for more than 18 months. Multiple vendors had to recreate their submissions, and multiple submissions were closed because of this requirement.

307

You're out of date here - there is an exemption available, specifically for older systems where the complete boot chain is not NX-compatible.

The rest of your claims are unhelpful and irrelevant to this review.

ghost commented 1 month ago

@steve-mcintyre

I'm deeply concerned that this exemption was not mentioned when reviewers were closing other people's reviews, even though it was well-known for reviewers :heart:

Anyone who looked at the process closely knows it from here -> https://github.com/rhboot/shim-review/issues/305#issuecomment-1344812569. It was an exemption due to security reasons for a company that already had an SB shim. In the end, the exemption made it better for everyone since the systems were already in use, and Oracle Linux using older shims and boot chain parts were less secure without it.

The situation becomes significantly more precarious when we consider accepting an EOLed system and a SHIM build on an EOLed distribution.

Not addressing concerns and gaslighting people into "unhelpful and irrelevant" is not only unhelpful and irrelevant but also toxic.

You admitted as the official reviewer about exemption here, but not when the reviews were closed and pushed into newer versions, and forcing NX-bit is plainly disrespectful.

The willingness to sign an EOLed distro with "third-party" support, not publicly available sources, but using/supporting an open-sourced/freely available distros (CentOS, OL, SL, Red Hat) without the ability to even do proper security research is a testament to incompetence.

EDIT: EOT I won't answer anymore. Everyone who read this knows that review process is useless.

steve-mcintyre commented 1 month ago

At the point that various reviews were closed for not including NX, that was based on the best information we had about requirements at the time. The exemption for NX was negotiated significantly later.

EDIT: EOT I won't answer anymore. Everyone who read this knows that review process is useless.

Uninformed anonymous sniping is clearly helpful.

SherifNagy commented 1 month ago

@jason-rodri Can you clarify if you will still include the EoL Centos CA or not?

jason-rodri commented 1 month ago

@jason-rodri Can you clarify if you will still include the EoL Centos CA or not?

We will not be including the EOL centos CA in our EL7 offering.

richterger commented 3 weeks ago

review for ciqliq-shim-EL7-x64-ia32-20240702

I am not an authorized reviewer, hope this helps anyway

# sha256sum /builddir/build/SOURCES/shim-15.8.tar.bz2
a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9  /builddir/build/SOURCES/shim-15.8.tar.bz2
SHA256 sums ::
088610925c2491017f6488f6235c6daec4e7f567dfb6c4e8c55d64d6acaafbae  /shimx64.efi
088610925c2491017f6488f6235c6daec4e7f567dfb6c4e8c55d64d6acaafbae  /shim_result/usr/share/shim/15.8-0.el7/x64/shimx64.efi
14822c87e48f9ca65df08a4595ffa8cc6a7564197826521318488178fdf16272  /shimia32.efi
14822c87e48f9ca65df08a4595ffa8cc6a7564197826521318488178fdf16272  /shim_result/usr/share/shim/15.8-0.el7/ia32/shimia32.efi
# objdump -x shimx64.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        0000000000001000
DllCharacteristics      00000000

# objdump -x shimia32.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000
# 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,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.8,mail:it_security@ciq.com

# 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,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ciq,1,Ctrl IQ Inc,shim,15.8,mail:it_security@ciq.com
# openssl x509 -noout -inform DER -in /builddir/build/SOURCES/ciq_sb_ca.der -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            7c:51:d5:cc:c2:fd:b3:32:81:0d:51:36:18:fc:4d:01
    Signature Algorithm: sha384WithRSAEncryption
        Issuer: C=US, ST=Nevada, L=Reno, O=Ctrl IQ, Inc., CN=Ctrl IQ, Inc. Root CA
        Validity
            Not Before: Apr 21 15:09:30 2023 GMT
            Not After : Apr 20 23:59:59 2048 GMT
        Subject: C=US, ST=Nevada, L=Reno, O=Ctrl IQ, Inc., CN=Ctrl IQ, Inc. Issuing CA
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (4096 bit)
        ...
            X509v3 Basic Constraints: critical
                CA:TRUE, pathlen:0
zeetim commented 6 days ago

Review of ciqliq-shim-EL7-x64-ia32-20240705

I am not an authorized reviewer but I want to help

SHA256 sums :: 088610925c2491017f6488f6235c6daec4e7f567dfb6c4e8c55d64d6acaafbae /shimx64.efi 088610925c2491017f6488f6235c6daec4e7f567dfb6c4e8c55d64d6acaafbae /shim_result/usr/share/shim/15.8-0.el7/x64/shimx64.efi 14822c87e48f9ca65df08a4595ffa8cc6a7564197826521318488178fdf16272 /shimia32.efi 14822c87e48f9ca65df08a4595ffa8cc6a7564197826521318488178fdf16272 /shim_result/usr/share/shim/15.8-0.el7/ia32/shimia32.efi

- NX is not set:

objdump -x /shimx64.efi | grep DllCharacteristics

DllCharacteristics 00000000

objdump -x /shimia32.efi | grep DllCharacteristics

DllCharacteristics 00000000

- shim sbat looks fine

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,4,UEFI shim,shim,1,https://github.com/rhboot/shim shim.ciq,1,Ctrl IQ Inc,shim,15.8,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,4,UEFI shim,shim,1,https://github.com/rhboot/shim shim.ciq,1,Ctrl IQ Inc,shim,15.8,mail:it_security@ciq.com

- Certificate:
    * valid for 25 years
    * RSA 4096 bits key
    * Is a CA certificate

openssl x509 -noout -inform DER -in /builddir/build/SOURCES/ciq_sb_ca.der -text

Certificate: Data: Version: 3 (0x2) Serial Number: 7c:51:d5:cc:c2:fd:b3:32:81:0d:51:36:18:fc:4d:01 Signature Algorithm: sha384WithRSAEncryption Issuer: C=US, ST=Nevada, L=Reno, O=Ctrl IQ, Inc., CN=Ctrl IQ, Inc. Root CA Validity Not Before: Apr 21 15:09:30 2023 GMT Not After : Apr 20 23:59:59 2048 GMT Subject: C=US, ST=Nevada, L=Reno, O=Ctrl IQ, Inc., CN=Ctrl IQ, Inc. Issuing CA Subject Public Key Info: Public Key Algorithm: rsaEncryption Public-Key: (4096 bit)

- grub sbat seems correct considering the NTFS vulnerability:

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.rhel7,2,Red Hat Enterprise Linux 7,grub2,1:2.02-0.87.el7.14,mail:secalert@redhat.com grub.ciq_centos7,1,Centos Linux 7 (CIQ build),grub2,1:2.02-0.87.el7.14,mailto:secureboot@ciq.com


---
I am not a CentOS user but I assume that both grub and kernel have been hardened to ensure OS security.
jason-rodri commented 5 days ago

Updated our submission README.md to reflect current stance on certwrapper usage for EL7.

N/a, at one point we considered using certwrapper, but decided against it considering the CentOS 7 CA is expiring and the upstream components are receiving no new updates.

aronowski commented 2 days ago

Considering the worries about arising security issues, the shim community can't possibly track everything on its own and has to trust the organizations to some extent - I'd say that if the worries are so strong about whether this shim should be signed or not, it's up to Microsoft's Hardware Dev Center branch to decide on that, especially considering future shims and the whole chain's NX support.

This is also reinforced by how trust gets earned and how easy it can be lost - I doubt that anyone would be willing to risk doing insecure operations here intentionally for a potential reputation loss, i.e.: I myself trust that CIQ Bridge is made secure by design, so that the whole chain remains secure both during the customers' upgrade period and after it, and it's not my responsibility to use it on my own and keep track of everything it does.

Things look alright to me and the build does reproduce. Thanks to everyone for the help with reviews!