Shim 15.7 for openSUSE Tumbleweed

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/jsegitz/shim-review/tree/SUSE-openSUSE_tumbleweed-shim-15.7-20230403_tag

What is the SHA256 hash of your final SHIM binary?

pesign --hash --padding --in=./usr/share/efi/x86_64/shim-opensuse.efi hash: 793aff84df52f86aceccc1be0111de4976d9e32fc62b20ac2ef6223f3c0516c1

sha256sum ./usr/share/efi/x86_64/shim-opensuse.efi 6af9b677a91b9f7fc4c06c18cc8ffaec91dd9255468d40e68ceb317af06b5d62 ./usr/share/efi/x86_64/shim-opensuse.efi

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

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

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

The tag used for review is not covered by the rule: tag it with a tag of the form "myorg-shim-arch-YYYYMMDD". It has the word _tag at the end. But I can see the proper one has been used as a branch name.

Does your GRUB2 binary and kernel have the latest NX requirements mentioned here enabled:

Also NX support needs to be added to bootloader and kernel.

In other words: do you have a patch that toggles the NX compatibility flag for the vmlinuz binary somewhere in the build process?

The same question applies to the GRUB2 directory you attached - is there a patch for the NX compatibility flag somewhere that I cannot see?

Despite the fact there's the shim-Enable-the-NX-compatibility-flag-by-default.patch in here, your build process (covered by the build_log.x86_64 file) forces your shim binary to not have the NX compatibility flag enabled -

[...]
[   27s] ./post-process-pe -vv  MokManager.efi
[   27s] set_dll_characteristics():355: Updating DLL Characteristics from 0x0000 to 0x0100
[...]
[   27s] ./post-process-pe -vv  fallback.efi
[   27s] set_dll_characteristics():355: Updating DLL Characteristics from 0x0000 to 0x0100
[...]
[   30s] ./post-process-pe -vv -N shim.efi
[   30s] + grep -q 'openSUSE Secure Boot CA1' shim.efi
[...]

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

The shim-opensuse_x86_64.efi binary you attached has this flag set to disabled either.

*******************************************************************************
### How do you manage and protect the keys used in your SHIM?
*******************************************************************************
The keys are in a specially hardened machine that is in our build environment.

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

The private key must be protected with a hardware cryptography module. This includes but is not limited to HSMs, smart cards, smart card–like USB tokens, and TPMs.
The operating environment must achieve a level of security at least equal to FIPS 140-2 Level 2.

*******************************************************************************
### 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.
*******************************************************************************
yes

[...]

fwupd:
sbat,1,UEFI shim,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
fwupd,1,Firmware update daemon,fwupd,1.7.3,https://github.com/fwupd/fwupd
fwupd-sle,1,SUSE Linux Enterprise,fwupd,1.7.3,https://build.opensuse.org

shim:
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.opensuse,1,The openSUSE project,shim,15.7,mail:security-team@suse.de

Shouldn't fwupd-sle be fwupd.sle?

As far as I'm concerned, shim's SBAT does not need the mail: string. I'm unsure how this implementation is going to perform in regards to e.g. parsing the binary to extract only the email address and the output having the additional prefix.

Why doesn't your review mention the following GRUB2 modules:

crypttab
read
tpm
tpm2
net

despite them being listed in your GRUB2's specfile?

I assume there's one spelling error since the efinet module has been duplicated.

Thank you for your notes.

tag: For previous submissions I used the same branch and tag name, but that caused issues so now I added the _tag prefix. I can drop this if this isn't okay
NX requirements: I've reached out to our shim, grub2 and kernel maintainers and will get back to you
Key management: See my comments https://github.com/rhboot/shim-review/issues/301#issuecomment-1457671340
SBAT entry: I've reached out to the fwupd and shim maintainer to get this fixed. As for the mail string: I'm also not sure about this. if this is non-default we can change that, but until now this was accepted
grub2 modules: I've thought about this again. What you write is correct, but strictly speaking the list misses a lot more entries. We have a couple of grub2 variants. In the secure boot case one specific variant is used, but nothing really prevents you from using the others since they all get signed the same way. So the correct list would be: acpi adler32 affs afs afsplitter ahci all_video aout appendedsig appended_signature_test appleldr archelp asn1 ata at_keyboard backtrace bfs biosdisk bitmap bitmap_scale blocklist boot_loader_interface boot bsd bswap_test btrfs bufio cat cbfs cbls cbmemc cbtable cbtime chain cmdline_cat_test cmdline cmosdump cmostest cmp cmp_test configfile cpio_be cpio cpuid crc64 cryptodisk crypto crypttab cs5536 ctz_test datehook date datetime diskfilter disk div div_test dm_nv drivemap echo efiemu efifwsetup efi_gop efinet efi_uga ehci elf eval exfat exfctest ext2 extcmd f2fs fat file fixvideo font freedos fshelp functional_test gcry_arcfour gcry_blowfish gcry_camellia gcry_cast5 gcry_crc gcry_des gcry_dsa gcry_idea gcry_md4 gcry_md5 gcry_rfc2268 gcry_rijndael gcry_rmd160 gcry_rsa gcry_seed gcry_serpent gcry_sha1 gcry_sha256 gcry_sha512 gcry_tiger gcry_twofish gcry_whirlpool gdb geli gettext gfxmenu gfxterm_background gfxterm_menu gfxterm gmodule gptsync gzio halt hashsum hdparm hello help hexdump hfs hfspluscomp hfsplus http iorw iso9660 jfs jpeg json keylayouts keystatus ldm legacycfg legacy_password_test linux16 linuxefi linux loadbios loadenv loopback lsacpi lsapm lsefimmap lsefi lsefisystab lsmmap ls lspci lssal lsxen luks2 luks lvm lzopio macbless macho mda_text mdraid09_be mdraid09 mdraid1x memdisk memrw minicmd minix2_be minix2 minix3_be minix3 minix_be minix mmap morse mpi msdospart mul_test multiboot2 multiboot nativedisk net newc nilfs2 normal ntfscomp ntfs ntldr odc offsetio ohci part_acorn part_amiga part_apple part_bsd part_dfly part_dvh part_gpt part_msdos part_plan part_sun part_sunpc parttool password password_pbkdf2 pata pbkdf2 pbkdf2_test pcidump pci pgp pkcs1_v15 plan9 play png prep_loadenv priority_queue probe procfs progress pxechain pxe raid5rec raid6rec random rdmsr read reboot regexp reiserfs relocator romfs scsi search_fs_file search_fs_uuid search_label search sendkey serial setjmp setjmp_test setpci sfs shift_test signature_test sleep sleep_test smbios spkmodem squash4 strtoull_test syslinuxcfg tar terminal terminfo test_asn1 test_blockarg testload test testspeed tftp tga time tpm2 tpm trig tr truecrypt true udf ufs1_be ufs1 ufs2 uhci usb_keyboard usb usbms usbserial_common usbserial_ftdi usbserial_pl2303 usbserial_usbdebug usbtest vbe verifiers vga vga_text video_bochs video_cirrus video_colors video_fb videoinfo video videotest_checksum videotest wrmsr xfs xnu xnu_uuid xnu_uuid_test xzio zfscrypt zfsinfo zfs zstd if I take all of the modules that are shipped in some variant. That's not what you would have in realistic use cases, but it is the attack surface that is exposed. So I tend to add them all to the submission, but will wait for comments before I do this

It's not like the _tag suffix bothers me personally but I can't speak for other entities like the Red Hat Bootloader Team or Microsoft - maybe they happen to use some sort of automation tool that won't work with this suffix.

If I was to use your framework, that is having multiple branches and then tagging specific commits, I would use something like this: for developing specifically Shim 15.7 for openSUSE Tumbleweed I would use the branch SUSE-openSUSE_tumbleweed-shim-15.7-devel and then tag the commit I would want to publish for the review as SUSE-openSUSE_tumbleweed-shim-15.7-20230404.

This way the branch would indicate continuous development and potential shim-review fixes for that specific product and the tag would indicate the date of the commit to be reviewed.

Alright, waiting for the NX updates.

Hint: utlize tools for static analysis to make sure the final artifact has NX support enabled. You can use, for instance, NTCore's CFF Explorer or zed-0xff's pedump.

I do it this way to see the effecive outcome that only matters to me - as seen in the build logs, sometimes just adding a patch won't be enough.

I can see the machine is hardened and certified indeed!

Altough I can't speak for Microsoft and how their requirements will affect this review. Hope everything goes well.

Regarding GRUB2 variants, I understand the distribution allows one to choose the variant they want and therefore all of them are valid.

If this is correct, I would attach such a note in the review and let the official reviewers decide on this.

The NX topic is a bigger item for us. So I'll close this review request here and will reopen once all the necessary changes have been done and tested. Thanks again for your review @aronowski

rhboot / shim-review