Skip to content
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

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

Closed
8 tasks done
jason-rodri opened this issue Jun 28, 2024 · 17 comments
Closed
8 tasks done

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

jason-rodri opened this issue Jun 28, 2024 · 17 comments
Labels
accepted Submission is ready for sysdev contacts verified OK Contact verification is complete here (or in an earlier submission) easy to review This submission might be a good place to start for an inexperienced reviewer

Comments

@jason-rodri
Copy link

jason-rodri commented Jun 28, 2024

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

  • completed README.md file with the necessary information
  • shim.efi to be signed
  • public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
  • binaries, for which hashes are added to vendor_db ( if you use vendor_db and have hashes allow-listed )
  • any extra patches to shim via your own git tree or as files
  • any extra patches to grub via your own git tree or as files
  • build logs
  • 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/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

@steve-mcintyre steve-mcintyre added the contacts verified OK Contact verification is complete here (or in an earlier submission) label Jul 1, 2024
@dennis-tseng99
Copy link
Collaborator

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

  • code is reproducible (ok)
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
  • Hash is matched (ok)
#sha256sum shimx64.efi
654d8efe248cd113f7ecb5a1f4fc9c309cc0d65a66b4bb8d9b2991f57f2dbcf6  shimx64.efi
#sha256sum shimia32.efi 
b739423471c03d32f2918906286076ea73c1385ced3f175a60ceeb8fadf009de  shimia32.efi
  • NX flag is disable: (ok)
#objdump -x shimx64.efi | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000

#objdump -x shimia32.efi | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000
  • sbat seems ok for shim and grub.
    NTFS fixing is not needed, so grub,3 is reasonable.
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:[email protected]

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:[email protected]

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/
....
  • use an ephemeral key for signing kernel modules (good)

  • Certificate Validity: 25 years + 4096 bit size for RSA Public-Key (good)
    openssl x509 -in ciq_sb_ca.der -inform der -noout -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
                RSA Public-Key: (4096 bit)
  • 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
  • Need more reviewers to take a look.

@jason-rodri
Copy link
Author

@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
Copy link
Collaborator

@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?
  • Why are you still using rocky under /boot/efi directory?
  • What's your plan for NX for your boot chain for el7?
  • Why not just use any of your current signed shim for this release?

@jason-rodri
Copy link
Author

@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
Copy link
Collaborator

  • 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 SherifNagy self-assigned this Jul 24, 2024
@SherifNagy SherifNagy added extra review wanted easy to review This submission might be a good place to start for an inexperienced reviewer labels Jul 24, 2024
@SherifNagy
Copy link
Collaborator

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

  • Security contacts looks good, didn't change since last successful submission
  • Keys are stored in FIPS HSM

Shim

  • Uses upstream 15.8 and source hashes matches original hashes
  • SBAT entries from shim looks fine
  • No patches added on top of upstream shim
  • Vendor SBAT entry is at 1
  • Binaries are reproducible using the container image
STEP 18/18: RUN chmod 0755 /root/shim-compare.sh;  /root/shim-compare.sh

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

This comment was marked as abuse.

@steve-mcintyre

This comment was marked as outdated.

@ghost

This comment was marked as abuse.

@steve-mcintyre

This comment was marked as outdated.

@rhboot rhboot deleted a comment Aug 4, 2024
@SherifNagy
Copy link
Collaborator

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

@jason-rodri
Copy link
Author

@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
Copy link

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

I am not an authorized reviewer, hope this helps anyway


  • shim-15.8.tar.bz2 sha256sum matches (extracted from shim-unsigned-x64-15.8-0.el8.src.rpm during build inside container)
# sha256sum /builddir/build/SOURCES/shim-15.8.tar.bz2
a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9  /builddir/build/SOURCES/shim-15.8.tar.bz2
  • build of shimx64.efi & shimia32.efi is reproducible and matches build log
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 not set
# objdump -x shimx64.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        0000000000001000
DllCharacteristics      00000000

# objdump -x shimia32.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000
  • shim sbat ok
# 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:[email protected]

# 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:[email protected]
  • shim not patched (only one patch that remove -f option from dos2unix)

  • unmodified upstream GRUB from CentOS 7, should be fine

  • kernel form CentOS 7, should be fine

    • patches are applied to kernel, but not related to shim/uefi boot verification
    • ephemral key is used
  • Certificate

    • is a ca certificate
    • valid for 25 years
    • 4096 Bit RSA Key
    • Keys of the issued certificates are stored in a HSM
# 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                
  • Note: I am not a CentOS user and didn't dig into the source of CentOS, so I did not personally
    verify the security state of grub and kernel of CentOS 7, but I would assume that, as stated in the README
    all revelant security patches are applied.

@SherifNagy SherifNagy removed their assignment Aug 21, 2024
@aronowski aronowski self-assigned this Sep 2, 2024
@zeetim
Copy link

zeetim commented Sep 3, 2024

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

I am not an authorized reviewer but I want to help

  • shim sources sha256 sum matches:
# sha256sum /builddir/build/SOURCES/shim-15.8.tar.bz2 
a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9  /builddir/build/SOURCES/shim-15.8.tar.bz2
  • build is reproducible:
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
  • 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:[email protected]

# 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:[email protected]
  • 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:[email protected]
grub.ciq_centos7,1,Centos Linux 7 (CIQ build),grub2,1:2.02-0.87.el7.14,mailto:[email protected]

I am not a CentOS user but I assume that both grub and kernel have been hardened to ensure OS security.

@jason-rodri
Copy link
Author

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
Copy link
Collaborator

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!

@aronowski aronowski added accepted Submission is ready for sysdev and removed extra review wanted labels Sep 7, 2024
@aronowski aronowski removed their assignment Sep 7, 2024
@jason-rodri
Copy link
Author

Signed binaries returned by MSFT.

@THS-on THS-on closed this as completed Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Submission is ready for sysdev contacts verified OK Contact verification is complete here (or in an earlier submission) easy to review This submission might be a good place to start for an inexperienced reviewer
Projects
None yet
Development

No branches or pull requests

8 participants