VMware Photon OS shim-15.8

nkkuntal commented 7 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/nkkuntal/shim-review/tree/vmware-shim-x86_64-20240418

What is the SHA256 hash of your final SHIM binary?

$ sha256sum shimx64.efi
3c644e2d1f4449fa761c540615f2b59178639ae2fa74c493de71b951afc80e11  shimx64.efi

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

Previous version, based on shim 15.4, was approved here https://github.com/rhboot/shim-review/issues/164

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

N/A as security contacts are not verified recently.

BogdanAriton commented 6 months ago

I'm not an official reviewer, but I'm trying to help as much as I can. This review if for: vmware-shim-x86_64-20240418.

Shim binary can be reproduced using the Dockerfile and matches the Hash supplied in the README.md:

3c644e2d1f4449fa761c540615f2b59178639ae2fa74c493de71b951afc80e11 ./shimx64.efi

Shim sbat section looks good

objcopy --only-section .sbat -O binary ./shimx64_new.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.photon,1,VMware Photon OS,shim,15.8-1.ph5,https://github.com/vmware/photon)

    objdump ./shimx64_new.efi -s -j .sbatlevel

        ./shimx64_new.efi:     file format pei-x86-64

        Contents of section .sbatlevel:
        84000 00000000 08000000 37000000 73626174  ........7...sbat
        84010 2c312c32 30323330 31323930 300a7368  ,1,2023012900.sh
        84020 696d2c32 0a677275 622c330a 67727562  im,2.grub,3.grub
        84030 2e646562 69616e2c 340a0073 6261742c  .debian,4..sbat,
        84040 312c3230 32343031 30393030 0a736869  1,2024010900.shi
        84050 6d2c340a 67727562 2c330a67 7275622e  m,4.grub,3.grub.
        84060 64656269 616e2c34 0a00               debian,4..

NX bit not set and section alignment looks OK:

objdump -fhp ./shimx64.efi | egrep '(SectionAlignment|DllCharacteristics)'
SectionAlignment        00001000
DllCharacteristics      00000000

photon_sb2020.der is loaded directly as part of shim via VENDOR_CERT_FILE valid until Jan 1 06:59:59 2030 GMT, 2048 bit RSA

openssl x509 -inform der -in photon_sb2020.der -text -noout
Certificate:
Data:
    Version: 3 (0x2)
    Serial Number:
        1f:54:cd:7b:51:42:53:99:4c:d5:9a:fc:74:93:e6:0e
    Signature Algorithm: sha256WithRSAEncryption
    Issuer: CN = "VMware, Inc.,O=VMware, Inc.,L=Palo Alto,S=California,C=US"
    Validity
        Not Before: Jul  8 01:53:00 2020 GMT
        Not After : Jan  1 06:59:59 2030 GMT
    Subject: CN = "VMware, Inc.,O=VMware, Inc.,L=Palo Alto,S=California,C=US"
    Subject Public Key Info:
        Public Key Algorithm: rsaEncryption
            Public-Key: (2048 bit)
            Modulus:
                00:9e:24:c6:a5:8d:fe:95:58:47:45:c8:26:51:46:
                f9:5e:2e:f0:89:28:52:5d:d0:aa:cd:7a:e8:58:6f:
                4c:50:6b:88:be:90:3d:7c:de:90:74:a9:ad:13:ed:
                87:5a:4d:bc:59:aa:64:73:8c:64:2a:22:00:c4:4c:
                2c:53:64:21:a2:09:ed:8f:aa:f8:14:45:53:e9:69:
                0b:8b:b1:b3:d7:64:a4:de:6c:30:72:27:02:8a:f8:
                f2:86:d9:04:0f:2f:31:fb:a5:1a:18:ff:7a:2d:ea:
                ab:2d:b1:52:4a:04:60:fd:87:e3:2d:37:6c:62:ee:
                99:d7:9f:3d:c3:43:51:f3:91:c5:68:b5:33:ec:b9:
                90:b5:6b:97:dd:10:25:d8:8a:fa:49:a9:2e:05:44:
                fc:b5:14:ec:41:28:59:86:18:96:61:cc:61:7c:fb:
                1e:ab:e0:fb:a8:a6:10:21:cb:5a:89:47:83:0f:7c:
                32:a7:91:96:d5:73:e1:9e:7a:12:83:ba:b8:0d:2a:
                ab:3b:0e:71:b7:7a:b1:c2:54:1d:27:d6:0b:ab:24:
                11:c4:4d:f8:55:07:86:67:67:29:28:88:62:4d:4d:
                c4:49:fb:cb:88:82:b8:69:7c:a7:66:bf:6b:10:a0:
                c1:ae:f2:5a:ed:f2:d1:67:89:8f:d2:64:d9:03:d2:
                5b:17
            Exponent: 65537 (0x10001)
    X509v3 extensions:
        X509v3 Subject Key Identifier: 
            5F:07:9E:DA:AA:73:76:13:3F:04:5B:9A:D7:29:97:DD:F3:77:78:58
Signature Algorithm: sha256WithRSAEncryption
Signature Value:
    3d:8d:99:ee:5a:8c:1f:94:72:5d:aa:f1:c0:67:66:7c:d1:1e:
    e4:50:f7:15:d1:c5:e8:92:4e:7c:4e:de:a8:cf:26:09:d0:15:
    d4:46:27:c3:ad:a4:7c:47:f4:7a:cb:db:49:20:a2:95:57:9b:
    98:49:22:50:91:5d:4c:84:8b:8e:1c:61:86:df:f9:d3:f2:1f:
    3a:4b:57:25:bf:4f:7e:5f:09:5d:1c:52:39:50:14:04:a0:69:
    33:a5:f1:8e:b1:11:72:83:77:a8:50:82:1d:e3:ba:e6:10:a9:
    3a:9f:9f:95:fd:9a:7f:76:43:f3:23:b9:18:9b:2a:26:16:c1:
    30:bb:01:b2:8f:2b:99:63:09:da:bc:20:8d:5c:d6:78:55:00:
    8e:87:3d:54:5d:da:15:7c:26:14:a2:a3:31:33:9c:22:a2:04:
    95:d2:6a:6b:dd:d7:44:a9:bd:88:45:f0:39:65:f7:ac:66:d4:
    66:f7:b2:27:10:e8:f0:3d:b1:fe:b2:60:1b:7c:e2:bd:93:0e:
    35:0e:5a:b9:7a:3e:61:62:36:ba:fd:c7:01:13:e8:50:7b:2a:
    e8:73:04:52:a9:3a:c4:d9:03:90:c8:d6:4f:1c:aa:c3:14:d9:
    9e:3c:dc:3e:a5:f3:c1:69:e3:6c:51:07:93:74:a9:b4:c6:f4:
    91:09:6f:f6

There are additional 3 Shim patches.
- 0001-Add-provision-to-disable-netboot-and-httpboot-in-shi.patch is clear and I don't see any issues here
- 0001-Enforce-SBAT-presence-in-every-image.patch is clear and I don't see any issues here
- In regards to patch "0001-Introduce-support-for-revocations-build.patch", could you explain (at least for my benefit) how this works?
Linux version v6.1.90 includes the necessary security patches
I'm in the process of reviewing the 3 security patches that help with secure boot and kernel lock-down for version 6.1.90 (specified in the readme), I couldn't find other references for these patches outside of VMWare Linux Photon. There are a lot more patches under the linux/generic folder and not sure if we should review all of these and whether if all the patches are still relevant (since some of them seem to be valid for older Linux versions)
- 0001-kernel-lockdown-when-UEFI-secure-boot-enabled.patch - Looks like the new added definition CONFIG_LOCK_DOWN_KERNEL_IN_SECUREBOOT helps identify if secure boot is present and then calls the lock_kernel_down kernel function (this is achieved pretty much the same way by CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY but this will not check for secure boot and will lock down kernel either way)
- I don't see issues with either of these patches 0002-Add-.sbat-section.patch and 0003-Verify-SBAT-on-kexec.patch , but maybe someone else could also take a glance.

nkkuntal commented 6 months ago

Thank you @BogdanAriton for reviewing the shim-review.

The patch 0001-Introduce-support-for-revocations-build.patch utilizes the following code in shim.c: the function load_revocations_file() reads the provided revocations.efi, if it exists. In the case of valid .sbata and .sbatl section data, it updates the SbatLevel EFI variable by calling set_sbat_uefi_variable().

Consider a use case where Photon OS wants to revoke its grub2/kernel using SBAT. We could inject the aforementioned sections into revocations.efi generated with this patch. During the next boot, shim will update the SbatLevel and enforce the newly updated levels.

By utilizing this mechanism, we avoid changing SbatLevel in the shim source include/sbat_var_defs.h and avoid getting a new shim signed to revoke vendor binaries grub.photon and linux.photon.

BogdanAriton commented 6 months ago

Thank you @BogdanAriton for reviewing the shim-review.

The patch 0001-Introduce-support-for-revocations-build.patch utilizes the following code in shim.c: the function load_revocations_file() reads the provided revocations.efi, if it exists. In the case of valid .sbata and .sbatl section data, it updates the SbatLevel EFI variable by calling set_sbat_uefi_variable().

Consider a use case where Photon OS wants to revoke its grub2/kernel using SBAT. We could inject the aforementioned sections into revocations.efi generated with this patch. During the next boot, shim will update the SbatLevel and enforce the newly updated levels.

By utilizing this mechanism, we avoid changing SbatLevel in the shim source include/sbat_var_defs.h and avoid getting a new shim signed to revoke vendor binaries grub.photon and linux.photon.

@nkkuntal - thanks for clarifying that for me

dennis-tseng99 commented 5 months ago

Hi @nkkuntal, Because the global generation number has been bumped to 4 from 1, may I ask you a question about whether the product-specific minimum generation number is 1 or 2 in README.md:

grub2

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,2.06,https//www.gnu.org/software/grub/
grub.photon,2,VMware Photon OS,grub2,2.06-16.ph5,https://github.com/vmware/photon

I know your photon OS has been upgraded from 3.0 to 5.0 which can perfectly configure MACVLAN, VxLAN, macvtap .... I'm not sure this is your reason for 2.

In your previous shim-15.4 review:

grub2

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,1,Free Software Foundation,grub,2.06~rc1,https//www.gnu.org/software/grub/
grub.photon,1,VMware Photon OS,grub2,2.06~rc1-1.ph4,https://github.com/vmware/photon/tree/4.0/SPECS/grub2/

Please correct me if I'm wrong. Thank you very much.

vbrahmajosyula1 commented 5 months ago

Hi @dennis-tseng99

Thanks for taking time to review.

We chose to keep grub.photon to gen 2 as an indicator that our grub2 has transitioned from upstream grub2 to fedora downstream grub2.

dennis-tseng99 commented 5 months ago

Review for VMware Photon OS shim-15.8 #412 based on tag vmware-shim-x86_64-20240418

Binaries are producible by running "docker build ." (Ok) ............. ---> 2e97eb30b7a5 Successfully built 2e97eb30b7a5
Hash value is matched (ok) 3c644e2d1f4449fa761c540615f2b59178639ae2fa74c493de71b951afc80e11 /shimx64.efi 3c644e2d1f4449fa761c540615f2b59178639ae2fa74c493de71b951afc80e11 /usr/share/shim/shimx64.efi

sbat looks good objcopy --only-section .sbat -O binary /usr/share/shim/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.photon,1,VMware Photon OS,shim,15.8-1.ph5,https://github.com/vmware/photon

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,2.06,https//www.gnu.org/software/grub/
grub.photon,2,VMware Photon OS,grub2,2.06-16.ph5,https://github.com/vmware/photon

ephemeral key is used for kernel signing (good)
NX is disabled (Ok) objdump -x shimx64.efi | grep -E 'SectionAlignment|DllCharacteristics' SectionAlignment 00001000 DllCharacteristics 00000000
Certificate Validity (Ok) 10 years is good enough 2048 bits long is good enough before 2030. NITST deems RSA 2048 sufficient until 2030.

openssl x509 -in photon_sb2020.der -inform der -noout -text

Validity
            Not Before: Jul  8 01:53:00 2020 GMT
            Not After : Jan  1 06:59:59 2030 GMT
Subject: CN = "VMware, Inc.,O=VMware, Inc.,L=Palo Alto,S=California,C=US"
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (2048 bit)

comments: Marked as an easy to review This issue also reviewed by BogdanAriton Previous approved reviewing is here - https://github.com/rhboot/shim-review/issues/164 The usage of patch 0001-Introduce-support-for-revocations-build.patch is explained. VMware has done a good job for reviewing. Accepting.

THS-on commented 3 months ago

@nkkuntal did you get a signed shim back?

vbrahmajosyula1 commented 3 months ago

Sorry we forgot to mention it here. We ran into a logistical issue and were stuck for sometime. A couple of weeks ago we got the signed shim back. Thanks every one who helped review this.

rhboot / shim-review