search

rhboot / shim-review

Reviews of shim
66 stars 128 forks source link

Shim 15.8 for Freexian's Debian 10 ELTS (extended support by Freexian) #435

Open epozuelo opened 1 month ago

epozuelo commented 1 month 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/freexian/shim-review/tree/freexian-shim-amd64-i386-20240906

What is the SHA256 hash of your final SHIM binary?

9fd8ca20c5245a38b2a45c2b9c7b48d0ef8eeb0aa42a882b41a892eca3e8b72a shimx64.efi ce30b60229ba5ce5a01d778a93ac2cb1d3600a3049a527d6916e243bc13e940a shimia32.efi

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

N/A

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

aronowski commented 1 month ago

I couldn't find any public key matching the address emilio@freexian.com. Let me know, where can I get it, and I'll send verification emails.

If I may request, please edit the opening post, so that the link to your current application revision matches the text displayed (edit the visible text, which currently looks like a template one).

I'll perform an application review soon.

steve-mcintyre commented 1 month ago

I've sent mail to raphael@freexian.com, but the key for Emilio doesn't include a uid for emilio@freexian.com. Please fix...

rhertzog commented 1 month ago

Identity confirmation email received:

missive schemer petticoat chromed savoring mistaken deliveries consommé darns robin

epozuelo commented 1 month ago

I couldn't find any public key matching the address emilio@freexian.com. Let me know, where can I get it, and I'll send verification emails.

I have added that UID. Please fetch the key again from keyring.debian.org

If I may request, please edit the opening post, so that the link to your current application revision matches the text displayed (edit the visible text, which currently looks like a template one).

Fixed.

I'll perform an application review soon.

Thanks!

steve-mcintyre commented 1 month ago

Contact verification mail sent to emilio@freexian.com now

epozuelo commented 1 month ago

Identity verification:

Ahmad capsuling reposed electrodes aviatrix overdrafts psychologist controverts distrusting joyous

steve-mcintyre commented 1 month ago

Contatc verification complete

es-fabricemarie commented 1 month ago

Questions/notes:

epozuelo commented 3 weeks ago

Questions/notes:

  • shim is built from 15.8. But with patches. This will need more reviews

It's the same patches that were approved for Debian. If necessary, we could remove the arm patches, as we don't actually support SB there. However I'd prefer to keep it for this release (and drop it for the next) to avoid another round-trip.

  • does not yet use ephemeral keys to sign kernel modules. Any particular blocker for this?

Not really, the reason is that we inherited Debian's kernel which didn't support ephemeral keys. However, we've already backported the necessary changes and on our next kernel update we'll switch to ephemeral keys.

richterger commented 3 weeks ago

review for freexian-shim-amd64-i386-20240801

I am not an authorized reviewer, hope this helps anyway

# sha256sum shim_15.8.orig.tar.bz2 
a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9  shim_15.8.orig.tar.bz2
9fd8ca20c5245a38b2a45c2b9c7b48d0ef8eeb0aa42a882b41a892eca3e8b72a  /shim/shimx64.efi
9fd8ca20c5245a38b2a45c2b9c7b48d0ef8eeb0aa42a882b41a892eca3e8b72a  /shim-review/shimx64.efi

ce30b60229ba5ce5a01d778a93ac2cb1d3600a3049a527d6916e243bc13e940a  /shim/shimia32.efi
ce30b60229ba5ce5a01d778a93ac2cb1d3600a3049a527d6916e243bc13e940a  /shim-review/shimia32.efi

# objdump -x /shim/shimx64.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        0000000000001000
DllCharacteristics      00000000

# objdump -x /shim/shimia32.efi   | grep -E 'SectionAlignment|DllCharacteristics'
SectionAlignment        00001000
DllCharacteristics      00000000

# objcopy --only-section .sbat -O binary /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.debian,1,Debian,shim,15.8,https://tracker.debian.org/pkg/shim
shim.freexian,1,Freexian,shim,15.8,https://deb.freexian.com/extended-lts/pool/main/s/shim/

# objcopy --only-section .sbat -O binary /shim/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.debian,1,Debian,shim,15.8,https://tracker.debian.org/pkg/shim
shim.freexian,1,Freexian,shim,15.8,https://deb.freexian.com/extended-lts/pool/main/s/shim/
# openssl x509 -noout -text -inform DER -in Freexian_Secure_Boot_CA.der
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            22:30:8e:07:50:e6:83:e9
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: C = FR, ST = Auvergne-Rh\C3\B4ne-Alpes, L = Sorbiers, O = Freexian, OU = Secure Boot, CN = Freexian Secure Boot Certificate Authority
        Validity
            Not Before: Jun 22 00:00:00 2024 GMT
            Not After : Jun 21 23:59:59 2054 GMT
        Subject: C = FR, ST = Auvergne-Rh\C3\B4ne-Alpes, L = Sorbiers, O = Freexian, OU = Secure Boot, CN = Freexian Secure Boot Certificate Authority
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (2048 bit)
        ...
            X509v3 Basic Constraints: critical
                CA:TRUE
aronowski commented 1 week ago

Thank you for the patience. I wanted to review this application as thoroughly as possible, so I myself could also get introduced to the tooling used in the Debian family. That said, when there are some things I should be aware of, when doing this review, please let me know, what I missed - I'm here to learn as well.

So first I'd like to comment on the things that didn't go right for me, or I'd like to learn about.

Most importantly, I can't reproduce the build. Here are the logs, what happens on my end (Docker with the buildx plugin on Fedora 40). Note: the same happens when trying to rebuild from Dockerfile.i386:

$ time torsocks docker buildx build -t freexian-shim-amd64-i386-20240801 --progress=plain .
[...]
#20 1.080 make[1]: Leaving directory '/shim'
#20 1.081    dh_clean
#20 1.126  dpkg-source -b .
#20 1.221 dpkg-source: info: using source format '3.0 (quilt)'
#20 1.259 dpkg-source: info: building shim using existing ./shim_15.8.orig.tar.bz2
#20 1.623 dpkg-source: info: using patch list from debian/patches/series
#20 1.625 patch: **** out of memory
#20 1.626 dpkg-source: info: the patch has fuzz which is not allowed, or is malformed
#20 1.626 dpkg-source: info: if patch 'aarch64-gnuefi-old.patch' is correctly applied by quilt, use 'quilt refresh' to update it
#20 1.658 dpkg-source: error: LC_ALL=C patch -t -F 0 -N -p1 -u -V never -E -b -B .pc/aarch64-gnuefi-old.patch/ --reject-file=- < shim.orig.Cfjqwp/debian/patches/aarch64-gnuefi-old.patch subprocess returned exit status 2
#20 1.665 dpkg-buildpackage: error: dpkg-source -b . subprocess returned exit status 2
#20 ERROR: process "/bin/sh -c dpkg-buildpackage -us -uc" did not complete successfully: exit code: 2
------
 > [17/23] RUN dpkg-buildpackage -us -uc:
1.081    dh_clean
1.126  dpkg-source -b .
1.221 dpkg-source: info: using source format '3.0 (quilt)'
1.259 dpkg-source: info: building shim using existing ./shim_15.8.orig.tar.bz2
1.623 dpkg-source: info: using patch list from debian/patches/series
1.625 patch: **** out of memory
1.626 dpkg-source: info: the patch has fuzz which is not allowed, or is malformed
1.626 dpkg-source: info: if patch 'aarch64-gnuefi-old.patch' is correctly applied by quilt, use 'quilt refresh' to update it
1.658 dpkg-source: error: LC_ALL=C patch -t -F 0 -N -p1 -u -V never -E -b -B .pc/aarch64-gnuefi-old.patch/ --reject-file=- < shim.orig.Cfjqwp/debian/patches/aarch64-gnuefi-old.patch subprocess returned exit status 2
1.665 dpkg-buildpackage: error: dpkg-source -b . subprocess returned exit status 2
------
Dockerfile:23
--------------------
  21 |     RUN git checkout buster/updates
  22 |     RUN apt-get build-dep -y .
  23 | >>> RUN dpkg-buildpackage -us -uc
  24 |     WORKDIR /
  25 |     RUN hexdump -Cv /shim/shim*.efi > build
--------------------
ERROR: failed to solve: process "/bin/sh -c dpkg-buildpackage -us -uc" did not complete successfully: exit code: 2

Do I need to use other tooling and/or on a different system?

I couldn't find the shim_15.8-1_amd64.log file. Is this a typo and it should be shim_15.8-1~deb10u2_amd64.build instead?

I compared the GRUB2 modules used with the ones from the recent Debian 10 application:

$ diff debian-modules.md freexian-modules.md 
14a15
> fdt
45a47
> http
51d52
< linuxefi
59a61
> luks2
70a73
> peimage
81a85
> serial
82a87
> smbios

They seem fine, but just wondering, why the difference between the upstream-downstream relation.

The upstream commits, about which the application form asks, (and especially the one fixed with Linux 5.19) should have been fixed as part of Debian-provided kernel 5.10.120-1, and yes - the code from eadb2f47a3ced5c64b23b90fd2a3463f63726066 is there. I downloaded the sources from https://deb.freexian.com/extended-lts/pool/main/l/linux-5.10/linux-5.10_5.10.218.orig.tar.xz and confirmed it.

How do you manage and protect the keys used in your shim?

I assume the air-gapped computers are also stored in a secure manner or had their storage securely wiped, so as to prevent the recovery of the private keys. OK.

Our current kernel (the one in Debian 10 buster) does not use ephemeral keys. However we're preparing to switch to that on the next kernel update. For old modules signed by the Debian key, we rely on their revocations for now.

OK, as per https://github.com/steve-mcintyre/shim-review/tree/debian-10-shim-amd64-20240528

And some final thoughts:

Please also add some hints/links about the patches used, so it helps the people who are not that knowledgeable about the Debian systems family. I got lucky, as the patches have already been reviewed at https://github.com/rhboot/shim-review/issues/429#issuecomment-2257466728.

Also, when listing the SBAT entries, please surround these with three backticks (```), otherwise the rendering on GitHub makes it harder to read without resorting to reading in plaintext.

richterger commented 1 week ago

I am not a docker expert. For me building with the included Dockerfile worked without problem for 64Bit (didn't tried 32 Bit). I don't used buildx . What shows up for me is #20 1.625 patch: **** out of memory . Maybe there is some config setting on your machine that restricts memory?

epozuelo commented 1 week ago

Thank you for the patience. I wanted to review this application as thoroughly as possible, so I myself could also get introduced to the tooling used in the Debian family. That said, when there are some things I should be aware of, when doing this review, please let me know, what I missed - I'm here to learn as well.

So first I'd like to comment on the things that didn't go right for me, or I'd like to learn about.

Most importantly, I can't reproduce the build. Here are the logs, what happens on my end (Docker with the buildx plugin on Fedora 40). Note: the same happens when trying to rebuild from Dockerfile.i386:

$ time torsocks docker buildx build -t freexian-shim-amd64-i386-20240801 --progress=plain .
[...]
#20 1.080 make[1]: Leaving directory '/shim'
#20 1.081    dh_clean
#20 1.126  dpkg-source -b .
#20 1.221 dpkg-source: info: using source format '3.0 (quilt)'
#20 1.259 dpkg-source: info: building shim using existing ./shim_15.8.orig.tar.bz2
#20 1.623 dpkg-source: info: using patch list from debian/patches/series
#20 1.625 patch: **** out of memory
#20 1.626 dpkg-source: info: the patch has fuzz which is not allowed, or is malformed
#20 1.626 dpkg-source: info: if patch 'aarch64-gnuefi-old.patch' is correctly applied by quilt, use 'quilt refresh' to update it
#20 1.658 dpkg-source: error: LC_ALL=C patch -t -F 0 -N -p1 -u -V never -E -b -B .pc/aarch64-gnuefi-old.patch/ --reject-file=- < shim.orig.Cfjqwp/debian/patches/aarch64-gnuefi-old.patch subprocess returned exit status 2
#20 1.665 dpkg-buildpackage: error: dpkg-source -b . subprocess returned exit status 2
#20 ERROR: process "/bin/sh -c dpkg-buildpackage -us -uc" did not complete successfully: exit code: 2
------
 > [17/23] RUN dpkg-buildpackage -us -uc:
1.081    dh_clean
1.126  dpkg-source -b .
1.221 dpkg-source: info: using source format '3.0 (quilt)'
1.259 dpkg-source: info: building shim using existing ./shim_15.8.orig.tar.bz2
1.623 dpkg-source: info: using patch list from debian/patches/series
1.625 patch: **** out of memory
1.626 dpkg-source: info: the patch has fuzz which is not allowed, or is malformed
1.626 dpkg-source: info: if patch 'aarch64-gnuefi-old.patch' is correctly applied by quilt, use 'quilt refresh' to update it
1.658 dpkg-source: error: LC_ALL=C patch -t -F 0 -N -p1 -u -V never -E -b -B .pc/aarch64-gnuefi-old.patch/ --reject-file=- < shim.orig.Cfjqwp/debian/patches/aarch64-gnuefi-old.patch subprocess returned exit status 2
1.665 dpkg-buildpackage: error: dpkg-source -b . subprocess returned exit status 2
------
Dockerfile:23
--------------------
  21 |     RUN git checkout buster/updates
  22 |     RUN apt-get build-dep -y .
  23 | >>> RUN dpkg-buildpackage -us -uc
  24 |     WORKDIR /
  25 |     RUN hexdump -Cv /shim/shim*.efi > build
--------------------
ERROR: failed to solve: process "/bin/sh -c dpkg-buildpackage -us -uc" did not complete successfully: exit code: 2

Do I need to use other tooling and/or on a different system?

I'm not sure what's going on there. As @richterger mentioned, those out of memory errors look odd. That seems to be dpkg-source extracting the source package and applying the patch files. Can you attach a full build log? Also can you try to build directly with docker build, without the buildx plugin?

I couldn't find the shim_15.8-1_amd64.log file. Is this a typo and it should be shim_15.8-1~deb10u2_amd64.build instead?

Yes. same for the i386 build. Do you want me to fix this and make a new tag and update the link in this issue?

I compared the GRUB2 modules used with the ones from the recent Debian 10 application:

$ diff debian-modules.md freexian-modules.md 
14a15
> fdt
45a47
> http
51d52
< linuxefi
59a61
> luks2
70a73
> peimage
81a85
> serial
82a87
> smbios

They seem fine, but just wondering, why the difference between the upstream-downstream relation.

Hmm, they should be the same, because we don't have any changes to grub. The grub build prints the modules that it's bundling. I have built grub2 2.06-3~deb10u4 on amd64 and it gave me this:

Including modules all_video boot btrfs cat chain configfile echo efifwsetup efinet ext2 fat font f2fs gettext gfxmenu gfxterm gfxterm_background gzio halt help hfsplus iso9660 jfs jpeg keystatus loadenv loopback linux ls lsefi lsefimmap lsefisystab lssal memdisk minicmd normal ntfs part_apple part_msdos part_gpt password_pbkdf2 png probe reboot regexp search search_fs_uuid search_fs_file search_label sleep squash4 test true video xfs zfs zfscrypt zfsinfo cpuid linuxefi play tpm cryptodisk 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 luks lvm mdraid09 mdraid1x raid5rec raid6rec in obj/monolithic/grub-efi-amd64/grubx64.efi

The only difference with my previous list is the lack of tftp, which is only included in net binaries (e.g. grubnetx64.efi, but not in e.g. grubx64.efi)

The upstream commits, about which the application form asks, (and especially the one fixed with Linux 5.19) should have been fixed as part of Debian-provided kernel 5.10.120-1, and yes - the code from eadb2f47a3ced5c64b23b90fd2a3463f63726066 is there. I downloaded the sources from https://deb.freexian.com/extended-lts/pool/main/l/linux-5.10/linux-5.10_5.10.218.orig.tar.xz and confirmed it.

How do you manage and protect the keys used in your shim?

I assume the air-gapped computers are also stored in a secure manner or had their storage securely wiped, so as to prevent the recovery of the private keys. OK.

Our current kernel (the one in Debian 10 buster) does not use ephemeral keys. However we're preparing to switch to that on the next kernel update. For old modules signed by the Debian key, we rely on their revocations for now.

OK, as per https://github.com/steve-mcintyre/shim-review/tree/debian-10-shim-amd64-20240528

Note that we have backported the ephemeral key patch set and when we start signing kernels those will have ephemeral keys.

And some final thoughts:

Please also add some hints/links about the patches used, so it helps the people who are not that knowledgeable about the Debian systems family. I got lucky, as the patches have already been reviewed at #429 (comment).

Also, when listing the SBAT entries, please surround these with three backticks (```), otherwise the rendering on GitHub makes it harder to read without resorting to reading in plaintext.

Ack, will do.

aronowski commented 3 days ago

It looks like the Docker issue was something I managed to already fix on my end (unconfirmed, but might have been about to the LimitNOFILE option). Everything reproduces now.

I'd like to ask if you'd like to update the application for the shim_15.8-1~deb10u2_amd64.build file and the mention of ephemeral keys. Otherwise I think we're good to go!

epozuelo commented 3 days ago

It looks like the Docker issue was something I managed to already fix on my end (unconfirmed, but might have been about to the LimitNOFILE option). Everything reproduces now.

Great!

I'd like to ask if you'd like to update the application for the shim_15.8-1~deb10u2_amd64.build file and the mention of ephemeral keys. Otherwise I think we're good to go!

I have fixed the build log filenames, improved the formatting for SBAT entries, and added a clarification for the kernel ephemeral keys. I think it was already explained that we don't have them yet because the Debian kernel didn't have them, and that we'll switch to it in the next update. However that update is already prepared and uploaded to our staging repository, so I have added a note for that. Let me know if there's anything that is not clear.

aronowski commented 2 days ago

Looks alright! Accepting!