search

rhboot / shim-review

Reviews of shim
67 stars 129 forks source link

SHIM-15.4 for Univention GmbH #213

Closed pmhahn closed 2 years ago

pmhahn commented 2 years ago

Make sure you have provided the following information:

What organization or people are asking to have this signed:

Univention GmbH

What product or service is this for:

Univention Corporate Server 4.4

Please create your shim binaries starting with the 15.4 shim release tar file:
https://github.com/rhboot/shim/releases/download/15.4/shim-15.4.tar.bz2
This matches https://github.com/rhboot/shim/releases/tag/15.4 and contains
the appropriate gnu-efi source.
Please confirm this as the origin your shim.

https://github.com/univention/shim/tree/4.4 is based on https://salsa.debian.org/efi-team/shim/-/tags/debian/15.4-7_deb10u1 is baed on https://github.com/rhboot/shim/releases/tag/15.4

What's the justification that this really does need to be signed for the whole world to be able to boot it:

UCS is a Debian based Linux distribution for enterprise customers. It is used by many companies, governments, ministries, banks and schools. Many require Secure Boot to be compliant. As they use standard hardware, where the "Microsoft Corporation UEFI CA" is pre-installed. Secure Boot must work for them out-of-the-box.

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

The private key is stored on a hardware token. The token is locked away until a binary needs to be signed. In addition it requires a password to unlock the functionality. Only two people have access to the token and its password.

Do you use EV certificates as embedded certificates in the SHIM?

Yes - see in/univention-uefi-ca.der

If you use new vendor_db functionality, are any hashes allow-listed, and if yes: for what binaries ?

We do not use vendor_db.

Is kernel upstream commit 75b0cea7bf307f362057cc778efe89af4c615354 present in your kernel, if you boot chain includes a Linux kernel ?

We will only load Linux v4.9.x where x ist currently 272. The Lockdown features was only introduced with Linux v5.4, which the Debian and our kernel does not include.

if SHIM is loading GRUB2 bootloader, are CVEs CVE-2020-14372,
CVE-2020-25632, CVE-2020-25647, CVE-2020-27749, CVE-2020-27779,
CVE-2021-20225, CVE-2021-20233, CVE-2020-10713, CVE-2020-14308,
CVE-2020-14309, CVE-2020-14310, CVE-2020-14311, CVE-2020-15705,
( July 2020 grub2 CVE list + March 2021 grub2 CVE list )
and if you are shipping the shim_lock module CVE-2021-3418
fixed ?

yes, we're booting GRUB2

"Please specifically confirm that you add a vendor specific SBAT entry for SBAT header in each binary that supports SBAT metadata
( grub2, fwupd, fwupdate, shim + all child shim binaries )" to shim review doc ?
Please provide exact SBAT entries for all SBAT binaries you are booting or planning to boot directly through shim

shim,fb,mm: objcopy -O binary --only-section=.sbat out/shimx64.efi ./tmp && cat ./tmp ; rm -f ./tmp:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,1,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.debian,1,Debian,shim,15.4,https://tracker.debian.org/pkg/shim
shim.univention,1,Univention,shim,15.4-7~deb10u1,https://forge.univention.org/bugzilla/

grub: objcopy -O binary --only-section=.sbat grubx64.efi ./tmp && cat ./tmp ; rm -f ./tmp

objcopy -O binary --only-section=.sbat x/usr/lib/grub/x86_64-efi/monolithic/grubx64.efi ./tmp && cat ./tmp ; rm -f ./tmp
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,1,Free Software Foundation,grub,2.02,https://www.gnu.org/software/grub/
grub.debian,1,Debian,grub2,2.02+dfsg1-20+deb10u4,https://tracker.debian.org/pkg/grub2
grub.univention,1,Univention,grub2,2.02+dfsg1-20+deb10u4A~4.4.0.202110052241,https://forge.univention.org/bugzilla

kernel.efi: not SBAT yet

Were your old SHIM hashes provided to Microsoft ?

Previously we used a v0.7 signed my Microsoft 2014 with hash 495300790e6c9bf2510daba59db3d57e9d2b85d7d7640434ec75baa3851c74e5. This hash is alredy contained in DBX available https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin. We use no other SHIM ever signed by Microsoft.

The new SHIM uses a new certificate, so there is no chain to our old signed binaries.

Did you change your certificate strategy, so that affected by CVE-2020-14372, CVE-2020-25632, CVE-2020-25647, CVE-2020-27749,
CVE-2020-27779, CVE-2021-20225, CVE-2021-20233, CVE-2020-10713,
CVE-2020-14308, CVE-2020-14309, CVE-2020-14310, CVE-2020-14311, CVE-2020-15705 ( July 2020 grub2 CVE list + March 2021 grub2 CVE list )
grub2 bootloaders can not be verified ?

yes, we're now using a different certificate.

What exact implementation of Secureboot in grub2 ( if this is your bootloader ) you have ?
Upstream grub2 shim_lock verifier or Downstream RHEL/Fedora/Debian/Canonical like implementation ?

We're using the downstream Debian implementation.

What is the origin and full version number of your bootloader (GRUB or other)?
If your SHIM launches any other components, please provide further details on what is launched

SHIM is only used to launch GRUB2.

If your GRUB2 launches any other binaries that are not Linux kernel in SecureBoot mode,
please provide further details on what is launched and how it enforces Secureboot lockdown

GRUB2 is only used to load Linux kernel.

If you are re-using a previously used (CA) certificate, you
will need to add the hashes of the previous GRUB2 binaries
exposed to the CVEs to vendor_dbx in shim in order to prevent
GRUB2 from being able to chainload those older GRUB2 binaries. If
you are changing to a new (CA) certificate, this does not
apply. Please describe your strategy.

We use a new EV certificate.

How do the launched components prevent execution of unauthenticated code?

We chain load SHIM → GRUB2 → Linux kernel. The later only loads kernel modules signed by us.

Does your SHIM load any loaders that support loading unsigned kernels (e.g. GRUB)?

No, we only use static build of GRUB2.

What kernel are you using? Which patches does it includes to enforce Secure Boot?

Linux 4.9.272 or later with Debian patches

What changes were made since your SHIM was last signed?
What is the SHA256 hash of your final SHIM binary?
sha256sum out/shimx64.efi
# 5803f5e0fc873779529045bdc3d55494421a67c08d8e0708261d9256f01539f1  out/shimx64.efi

pesign -i out/shimx64.efi -h
# hash: 3cf97eba6a6d2c6a971974af4ff5cbefa4faf212f317cd06f21f9156e93cf515
tSU-RooT commented 2 years ago

I have confirmed below view points while refferring reviewer-guidelines:

pmhahn commented 2 years ago

This is SHIM / GRUB2 from Debian-10-Buster, but our UCS-4.4 is still based on Debian-9-Stretch, which only has gcc-6. We don't want to update GCC for our old distribution as this has introduced other incompatibilities in the past (different symbol mangling, especially for C++). If this is not okay it should be possible to only compile SHIM and GRUB with gcc-8, but keep the rest (old) at gcc-6. But this becomes tricky very fast as GRUB then also needs other dependencies, which we do not want to pull into our old release.

tSU-RooT commented 2 years ago

This is SHIM / GRUB2 from Debian-10-Buster, but our UCS-4.4 is still based on Debian-9-Stretch

Ah I see. I was mistaken Debian-10 buster based.

pmhahn commented 2 years ago

@tSU-RooT Thank you so far for your review

Is there anything we need to do or what are the next steps?

julian-klode commented 2 years ago

I've been looking at previous submissions and I only see #2, which has not been accepted. With the EV cert changed, and the entire review process having changed since 0.7, going to tag this as new vendor, and will look back when reviewing new vendors.

frozencemetery commented 2 years ago

While beginning email verification, I noticed that the fingerprint for Philipp Hahn's key appears malformed; please fix.

pmhahn commented 2 years ago

While beginning email verification, I noticed that the fingerprint for Philipp Hahn's key appears malformed; please fix.

@frozencemetery Fixed and pushed as new tag 4.4-9.1

frozencemetery commented 2 years ago

Thanks. I've sent you each some words; please post them here when you receive them.

pmhahn commented 2 years ago

Thanks. I've sent you each some words; please post them here when you receive them.

@frozencemetery Here are my words:

jadegrønere
lønnsdiskriminerte
prøveeksemplaret
landsbyliva
særønska
innholdsløsheten
pluralene
grettent
bråtebrenninga
hittfolka
krybbedødsfallene
systemdriften
vevlingen
edamrose commented 2 years ago

@frozencemetery you sent me the following words:

straffevilkår
fergeflåte
portikusen
rakkingen
rakstejenter
vidunderligere
mopedopplæringen
pondus
innsatsøyeblikkene
reiseskipets
abdikasjonenes
håndballspelleren
tankeproblem
frozencemetery commented 2 years ago

Both verified, thanks.

frozencemetery commented 2 years ago

Looking at tag 4.4-9.1:

Per README.md, shims are requested to start with the tarball and apply patches. The fork based on Debian you're using is not easily comparable to that: 674 commits ahead, 197 commits behind rhboot:main. Please build your shim in the requested way.

pmhahn commented 2 years ago

Looking at tag 4.4-9.1:

Per README.md, shims are requested to start with the tarball and apply patches. The fork based on Debian you're using is not easily comparable to that: 674 commits ahead, 197 commits behind rhboot:main. Please build your shim in the requested way.

According to the release page 15.4 is the latest version, which Debians and our version is based on. That release was imported as univention/shim@8119f718 tagged as upstream/15.4 and merged with univention/shim@8874f2ef2c53e4d297113bc9dc5da54b1d45e7c8 into the Debian git repository. So the list of changes boils down to the following:

# git log --oneline 8874f2ef2c53e4d297113bc9dc5da54b1d45e7c8..46d33604006b1d301581ad8daa2b29bd62446075 # --numstat
46d33604 (HEAD -> 4.4, knut/4.4, github/4.4) Bug #51748: Use Univention Secure Boot CA
8fa49497 (tag: debian/15.4-7_deb10u1, debian/buster/updates) Tweak how we call grub-install; don't abort on error
d2ffa630 (tag: debian/15.4-6_deb10u1) Release 15.4-6~deb10u1
c3e6f97d In insecure mode, don't abort if we can't create the MokListXRT var
dfbead40 Add arm64 patch to tweak section layout and stop crashing problems
a92474d9 (tag: debian/15.4-5_deb10u1) Add defensive code around calls to db_get
bca31a61 Fix up the template maintainer scripts
1adb4c7a (tag: debian/15.4-3_deb10u1) Add maintainer scripts to the template packages
e8befadf (tag: debian/15.4-2_deb10u1) Use a better version number for the buster build
c9718489 Add changelog for 15.4-1_deb10u2 with new patches
d42ee06f Don't call QueryVariableInfo() on EFI 1.10 machines
83bb0402 Fix handling of ignore_db and user_insecure_mode
6abb2734 (tag: debian/15.4-1_deb10u1) Stop hardcoding the release version in the rules file
ed0f7974 Clean more things
92314289 Prep for releasing based on 15.4
2098d57b allocate MOK config table as BootServicesData
f5e45657 Add one more patch from upstream to fix i386 binary relocations
5f7ef185 Print sha256 checksums of the EFI binaries when the build is done
21af506f Update to the 15.4 release

Most of them only touch the files required for Debian packaging, but the following actually cherry-pick patches from shim@main and puts them into debian/patches/, which get applied on top of shim_15.4.orig.tar.bz2 during package built. Quoting from README.md

Please create your shim binaries starting with the 15.4 shim release tar file:

So this is what Debian and we did. We do not prefer starting with just some untagged "git branch of the day" for stability reasons. So if the process is changed, please update the REAME.md and ISSUE_TEMPLATE.md to match the new process. Otherwise how should anybody know what's expected. We already has to delay out release two times because this process already takes nearly 2 months by now, which is very frustating.

pmhahn commented 2 years ago

Any news on this? If there anything we can do to get this moving forward?

pmhahn commented 2 years ago

To hopefully simplify the review I have created a 2nd git-branch https://github.com/univention/shim/tree/4.4-9

Hopefully this will allow you to continues with the review. If anything else is missing or required, please ask.

julian-klode commented 2 years ago

I'd just ignored frozencemetry there. The previous submission was entirely fine, same as the other Debian based submissions, and changing the submission is not helping you or me very much. Literally the guidelines say to start from the tarball, not the git, so any check needs to check that shim is built from the tarball, not vs the git repository - because the git repository is incomplete and needs gnu-efi pulled in and stuff.

julian-klode commented 2 years ago

Rejected. Kernel misses the lockdown patches necessary for signing.

This shim is otherwise fine to accept if any kernels without lockdown trusted by it were revoked by dbx

To be fixed: