search

rhboot / shim-review

Reviews of shim
67 stars 127 forks source link

shim 15.7 (NX Patched) for SUSE Euler Linux 2.1 #322

Closed parheliamm closed 6 months ago

parheliamm 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://github.com/parheliamm/shim-review/tree/sel-2.1-shim-20230330

What is the SHA256 hash of your final SHIM binary?

aarch64: pesign --hash --padding --in ./shim-sel_aarch64.efi hash: 7f7409b5892ef2cceaf6b3c49841b9868409ae800396d434cfcb4c6911fda78c sha256sum ./shim-sel_aarch64.efi 29a4ab0db9bbb2428ce166772fcb7567ebc0fde6ec7926e9af59ee28a5e64df3 ./shim-sel_aarch64.efi

x86_64: pesign --hash --padding --in=./shim-sel_x86_64.efi hash: a5f7876e09efe0ede04de0ccfb43b2492c98112e4e99d4545afbdcb183e43b6e sha256sum ./shim-sel_x86_64.efi 8f9bbbd6470c57de1a5ead3b88d7b3aa5b979106937e631968fa8ebc0a403d96 ./shim-sel_x86_64.efi

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

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

parheliamm commented 1 year ago

@frozencemetery : Would you please review this request?

Any feedback would be appreciated.

Chenxi

aronowski commented 1 year ago

While I'm not an official reviewer, I can see a few curiosities:

The shim does not seem to reproduce with the specified commands. This is the listing I made from the inside of running containers:

x86_64:

# find / -name '*shim*.efi' | xargs sha256sum 
find: '/proc/tty/driver': Permission denied
717016b17ea050aaeab1bbfbd19248cb150260bec33f970e4376308038799766  /root/rpmbuild/BUILD/shim-15.7/shim-opensuse.efi
68436a9feb680e1b1826d3ad1553ed4f2a6ba0853b0dcae93582fe3b384dcd4e  /root/rpmbuild/BUILD/shim-15.7/shim-sel.efi
78bcfd4a63484672d645ecf8d52285d6852bffb05614d56cb7729895ff68d48c  /root/rpmbuild/BUILD/shim-15.7/shim-sles.efi
68436a9feb680e1b1826d3ad1553ed4f2a6ba0853b0dcae93582fe3b384dcd4e  /root/rpmbuild/BUILD/shim-15.7/shim.efi
717016b17ea050aaeab1bbfbd19248cb150260bec33f970e4376308038799766  /shim/usr/lib64/efi/shim-opensuse.efi
68436a9feb680e1b1826d3ad1553ed4f2a6ba0853b0dcae93582fe3b384dcd4e  /shim/usr/lib64/efi/shim-sel.efi
78bcfd4a63484672d645ecf8d52285d6852bffb05614d56cb7729895ff68d48c  /shim/usr/lib64/efi/shim-sles.efi
68436a9feb680e1b1826d3ad1553ed4f2a6ba0853b0dcae93582fe3b384dcd4e  /shim/usr/lib64/efi/shim.efi
717016b17ea050aaeab1bbfbd19248cb150260bec33f970e4376308038799766  /shim/usr/share/efi/x86_64/shim-opensuse.efi
68436a9feb680e1b1826d3ad1553ed4f2a6ba0853b0dcae93582fe3b384dcd4e  /shim/usr/share/efi/x86_64/shim-sel.efi
78bcfd4a63484672d645ecf8d52285d6852bffb05614d56cb7729895ff68d48c  /shim/usr/share/efi/x86_64/shim-sles.efi
68436a9feb680e1b1826d3ad1553ed4f2a6ba0853b0dcae93582fe3b384dcd4e  /shim/usr/share/efi/x86_64/shim.efi

aarch64:

 find / -name '*shim*.efi' | xargs sha256sum
find: '/proc/tty/driver': Permission denied
6d5662d2e1b2c5b1bc1781d4044e38d9ca69f71104a83b66f10c9a2629392990  /shim/usr/share/efi/aarch64/shim-sel.efi
3652042cc661c4562f9931cf12ee3a777443d03bddc8b551bb8287582b787947  /shim/usr/share/efi/aarch64/shim-sles.efi
42e1036c325d319685642cee099f14d233ff85bd1dbec81b56503306efa9ccff  /shim/usr/share/efi/aarch64/shim-opensuse.efi
6d5662d2e1b2c5b1bc1781d4044e38d9ca69f71104a83b66f10c9a2629392990  /shim/usr/share/efi/aarch64/shim.efi
6d5662d2e1b2c5b1bc1781d4044e38d9ca69f71104a83b66f10c9a2629392990  /root/rpmbuild/BUILD/shim-15.7/shim-sel.efi
3652042cc661c4562f9931cf12ee3a777443d03bddc8b551bb8287582b787947  /root/rpmbuild/BUILD/shim-15.7/shim-sles.efi
42e1036c325d319685642cee099f14d233ff85bd1dbec81b56503306efa9ccff  /root/rpmbuild/BUILD/shim-15.7/shim-opensuse.efi
6d5662d2e1b2c5b1bc1781d4044e38d9ca69f71104a83b66f10c9a2629392990  /root/rpmbuild/BUILD/shim-15.7/shim.efi

I can't see any entries like these that are mentioned in the README:

Despite the fact the backport-shim-Enable-the-NX-compatibility-flag-by-default.patch patch has been introduced, both the shim-sel_x86_64.efi and shim-sel_aarch64.efi binaries attached in this review seem to not have the Image is NX compatible DllCharacteristic set.

*******************************************************************************
### How do you manage and protect the keys used in your SHIM?
*******************************************************************************
The key is installed in a machine with restricted physical and system access.
Shim binaries do not include private portions of the key.

Please, tell us more on how does your environment implement the following Microsoft signing requirements:

-------------------------------------------------------------------------------
### Which modules are built into your signed grub image?
*******************************************************************************
grub-core all_video boot cat configfile echo true font gfxmenu gfxterm gzio halt iso9660 jpeg minicmd normal part_apple part_msdos part_gpt password password_pbkdf2 png reboot search search_fs_uuid search_fs_file search_label sleep test video fat loadenv chain efifwsetup efinet linuxefi btrfs ext2 xfs jfs reiserfs tftp http efinet luks gcry_rijndael gcry_sha1 gcry_sha256 mdraid09 mdraid1x lvm serial

There seems to be a stylistic error as there are dashes rather than asterisks in the first line.

Also, according to yout grub2.spec file:

grub2.spec:

CD_MODULES="all_video boot cat configfile echo true \
                [...] \
                search_fs_file search_label sleep test video fat loadenv loopback"

grub2.changes:

-------------------------------------------------------------------
Mon Oct 24 01:58:08 UTC 2022 - Michael Chang <mchang@suse.com>

- Include loopback into signed grub2 image (jsc#PED-2150)

-------------------------------------------------------------------

Which patch inside your GRUB2 sources archive forces the final binary to have the Image is NX compatible DllCharacteristic set?

parheliamm commented 1 year ago

@aronowski : Thanks for your review and comments, we updated the shim binary without signature, it can be reproduced via docker environment. Below are my comments:

Reproducibility

[Chenxi]: According to docker rebuild, the pesign hash data are the same.

NX compatible:

[Chenxi]: Please try below command:

Apply below patch to shim source code to build post-process-pe

https://github.com/rhboot/shim/pull/531

Then verify the NX flag via below command:

~/post-process-pe -vv shim-sel_aarch64.efi shim-sel_x86_64.efi
set_dll_characteristics():358: Updating DLL Characteristics from 0x0000 to 0x0100
ms_validation():373: NX-Compat-Flag: PASS
ms_validation():378:   4K-Alignment: PASS
ms_validation():392: Section-Wr-Exe: PASS
fix_checksum():444: Updating checksum from 0x000efd4f to 0x000efe55
set_dll_characteristics():358: Updating DLL Characteristics from 0x0000 to 0x0100
ms_validation():373: NX-Compat-Flag: PASS
ms_validation():378:   4K-Alignment: PASS
ms_validation():392: Section-Wr-Exe: PASS
fix_checksum():444: Updating checksum from 0x000e9bbf to 0x000e9cc1

So we can make sure NX patched successfully on shim.

GRUB2 patch for NX compatible:

[Chenxi]: Currently, Grub2 doesn't support NX feature, we will try to implement this feature in the future.

aronowski commented 1 year ago

When it comes to the reproducibility, I always treated the reviews as them having the shim binary with the certificate embedded in it the one and only desired outcome.

In other words: I should be able to run the podman command you provided and it should output the shim-sel_x86_64.efi binary with your public CA certificate.

I don't understand, why strip the public CA certificate if it's public after all.

Also, when it comes to NX support in your shims, it's still not present. The patch you mentioned toggles the NX compatility flag from disabled to enabled once the post-process-pe program is ran. Your listing even says that the patched post-process-pe program toggles the flag here:

set_dll_characteristics():358: Updating DLL Characteristics from 0x0000 to 0x0100

So to qualify for signing, the shim binaries you provide need to already have the flag set to enabled. You can apply the patch you mentioned yourself, rebuild the binaries and update them in your review.

For implementing NX support in GRUB2 binary, maybe you can use this Fedora/RHEL patch for inspiration.

I can see there has been an update regarding GRUB2 modules but it still doesn't seem right. The change claims the following entries have been removed:

but except the grub-core entry, these are right there in your specfile implemented as the variable FS_MODULES, which is then used later:

GRUB_MODULES="[...] ${FS_MODULES} [...]"

Why? Is there a patch somewhere that removes them despite them being listed or there's some other unknown action taking place during the building process?

There has been no update regarding the keys management and protection which Microsoft requires.

parheliamm commented 1 year ago

@aronowski ： For NX compatible test:

As I mentioned previously, to test NX flag, you need to apply one more patch based on 15.7 source code. Our shim source code didn't apply this patch, you need to apply and build the post-process-pe on your side, then execute the binary and do the verification. The patch is: https://github.com/rhboot/shim/pull/531

Grub2: I will update the missing fs modules in the next submission. Thanks your comments.

Shim-binary: Because our docker build cannot support signature, so we striped the signature from shim binary. SLES and openSUSE has the same way to submit the review, so SUSE Euler follow the same policy.

Chenxi

aronowski commented 1 year ago

Alright, so as far as I understand, I should be able to attach your public certificate (called Signature in this comment) after the shim binaries have been built via podman. Is that right?

If so, what's the Standard Opearting Procedure for that? Then, why can't the reproducible building procedure apply this one automatically?

OK, waiting for the GRUB2 update. Also, it would be the best if you could implement NX support for the GRUB2 binary as well for this update.

As I mentioned previously, to test NX flag, you need to apply one more patch based on 15.7 source code.

Sorry, this is not true. Let me explain.

The rhboot's shim 15.7 release does not have the NX flag enabled by default. The support has been introduced in this pull request. This implementation's core functionality is the line that changes the set_nx_compat boolean to true in post-process-pe.c.

During the build process, the post-process-pe program shall be invoked and, with this patch in mind, toggle the NX compat flag to enabled by default. However, for shim, it does not do that according to your build logs:

[...]
[  129s] ./post-process-pe -vv  MokManager.efi
[  129s] set_dll_characteristics():354: Updating DLL Characteristics from 0x0000 to 0x0100
[...]
[  129s] ./post-process-pe -vv  fallback.efi
[  129s] set_dll_characteristics():354: Updating DLL Characteristics from 0x0000 to 0x0100
[...]
[  135s] ./post-process-pe -vv -N shim.efi
[  135s] + grep -q 'Yunche Secure Boot Signing' shim.efi
[...]

Somehow your build runs the program with the -N flag which means Disable the NX compatibility flag.

I got confused in my earlier comment - the patch you mention only attempts to perform some checks for Microsoft requirements rather than toggle the flag (mistaken it for PR #530). The invocation you mentioned here is still going to manually toggle the flag if you have the one from PR #530 applied since it's supposed to do so by default (and it does) rather than disabling the flag as your build logs say.

Since now we're talking about invoking a program that does modify the shim binary, I'd recommend different tools for static analysis. You can use, for instance, NTCore's CFF Explorer or zed-0xff's pedump to see for yourself that the shim binary you attached has the NX compatibility flag set to disabled. There are lots of other tools for analyzing PE binaries so the final decision on the tool to use is up to you.

There has been no update regarding the keys management and protection which Microsoft requires.

steve-mcintyre commented 1 year ago

@aronowski ： For NX compatible test:

As I mentioned previously, to test NX flag, you need to apply one more patch based on 15.7 source code. Our shim source code didn't apply this patch, you need to apply and build the post-process-pe on your side, then execute the binary and do the verification. The patch is: rhboot/shim#531

Sorry, that's not acceptable. The source code you're publishing needs to give us the same binaries that you want signing. Reviewers should not have to do extra work here to massage the build.

parheliamm commented 1 year ago

@aronowski ： For NX compatible test: As I mentioned previously, to test NX flag, you need to apply one more patch based on 15.7 source code. Our shim source code didn't apply this patch, you need to apply and build the post-process-pe on your side, then execute the binary and do the verification. The patch is: rhboot/shim#531

Sorry, that's not acceptable. The source code you're publishing needs to give us the same binaries that you want signing. Reviewers should not have to do extra work here to massage the build.

@steve-mcintyre : Thanks for your comments. Frankly, patch https://github.com/rhboot/shim/pull/531 is used for verifying NX flag only, this patch is used to prove that we applied nx-flags patch. This patch no need to deliver to the official source code. Actually, reviewers don't need to do extra work to verify the binary, because we can prove the binary is the same as the container build. The align data is the reason why sha256sum is different. Meanwhile, we prove the pesign hash data are all same.

Chenxi

parheliamm commented 1 year ago

@aronowski: Somehow your build runs the program with the -N flag which means Disable the NX compatibility flag. [Chenxi]: Our gurb2 is not ready for NX flag, so we have to disable it at this point. I am not sure whether we need to enable it by default.

aronowski commented 1 year ago

[Chenxi]: Our gurb2 is not ready for NX flag, so we have to disable it at this point.

In this context I'm talking only about the shim binaries, not GRUB2.

I already showed the listing of the build logs you attached, which prove that your shim is being built with NX compatibility being set to disabled.

Verifying the shim binary also means inspecting the artifact you attached in your review. As I mentioned earlier, there are tools that prove it does not have NX support currently.

When it comes to GRUB2:

[Chenxi]: Our gurb2 is not ready for NX flag, so we have to disable it at this point. I am not sure whether we need to enable it by default.

The required support has been described in issue #307

THS-on commented 6 months ago

@parheliamm can you either update this submission to 15.8 or create a new submission for that?