Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shim 15.6 for ECOS Technology GmbH #243

Closed
9 tasks done
ecos-platypus opened this issue Jun 8, 2022 · 18 comments
Closed
9 tasks done

Shim 15.6 for ECOS Technology GmbH #243

ecos-platypus opened this issue Jun 8, 2022 · 18 comments
Labels
accepted Submission is ready for sysdev new vendor This is a new vendor

Comments

@ecos-platypus
Copy link
Contributor

ecos-platypus commented Jun 8, 2022

Updates:

  • 2022-06-27: Build shim without ENABLE_SHIM_CERT=1
  • 2022-06-28: Remove duplicate entries from sbat

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/ecos-platypus/shim-review/tree/ECOS_Technology_GmbH-shim-x64-20220627


What is the SHA256 hash of your final SHIM binary?


54b18f8114d41c1488204fd91d3df3d902b505ebdf40aacf39f639eb3dc75e3e

@miray-tf
Copy link

Regarding your use of 'ENABLE_SHIM_CERT=1':

As I understand it the MokManager and fallback binaries still need a valid signature that shim trusts.
So if you don't use ENABLE_SHIM_CERT then MokManager and fallback need to be signed with the embedded EV-Certificate instead.
By using 'ENABLE_SHIM_CERT=1' MokManager and fallback can be signed by either the embedded EV-Certificate or the autogenerated certificate.

Or am I missing something here?

@ecos-platypus
Copy link
Contributor Author

@miray-tf You are right, the fallback and MokManager binaries also have to be signed when ENABLE_SHIM_CERT is not enabled. Our goal is that only binaries signed by us are loaded. However, this is already ensured by 100_disable_allowlist.patch so ENABLE_SHIM_CERT=1 is not required. I must have missed that so thank you very much for your input!

I removed the ENABLE_SHIM_CERT aspects from our shim and updated the submission. Without ENABLE_SHIM_CERT it now reproduces as the binary does not include different randomly generated shim certificates :-)

@miray-tf
Copy link

The current shimx64.efi duplicates the first 2 sbat lines:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,2,UEFI shim,shim,1,https://github.com/rhboot/shim
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,2,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.ecos,2,ECOS Technology GmbH,shim,15.6,mail:security@ecos.de

The reason is most likely that the shim binary already supplies the first 2 lines and your sbat.ecos.csv adds them again.

@ecos-platypus
Copy link
Contributor Author

@miray-tf Thank you again for your feedback. I removed the duplicate lines from sbat.ecos.csv, rebuilt shim and updated the submission.

@miray-tf
Copy link

Disclaimer: I am not an authorized reviewer

This Review is for tag 'ECOS_Technology_GmbH-shim-x64-20220628', sha1: df5dbae

Remaining Steps:

  • Contact verification by an authorized reviewer.
  • The GRUB module list includes the PGP module. I currently don't known how multiple verifiers interact, so somebody else might want to have another look at that.

Questions (Answer in a comment should be sufficient):

  • Debian still carries 4 secureboot / lockdown patches. One of them autmoatically locks down the kernel if secureboot is active. Could you please verify that the 5.4, 5.14 and 5.17 are locked down when secureboot is active?
  • Does Gentoo include any Gentoo specific SBAT information when Grub is built? If that is the case you might want to include it in the Grub SBAT section.

The vendor had a shim signed by Microsoft before, the shim is linked in #228:

  • Old shim has a now expired certificate for "ECOS Technology GmbH" embedded.
  • Current shim has a new certificate for "ECOS Technology GmbH"

Security contacts have a email address at the company and the PGP keys are cross-signed.
Contact verification by an authorized reviewer has not been attempted yet.

Shim is build from 15.6 with additional patches. The additional patches force the use of the default loader, disable whitelists and force shim to always run secureboot checks.
Build is reproducable and sha256 matches value 54b18f8114d41c1488204fd91d3df3d902b505ebdf40aacf39f639eb3dc75e3e

All questions were ansered in ISSUE and README.md
Reason why a signed shim is necessary sound reasonable.

Embedded certificate matches organisation name
Certificate is stored on a hardware token
Certificate has a duration of 3 years which is sensible.
The embedded certificate matches the certificate at https://github.com/ecos-platypus/shim-review/blob/ECOS_Technology_GmbH-shim-x64-20220627/ECOS_Tech_Code_signing_Certificate_Globalsign_2022.cer

SBAT is present and matches values in README
SBAT extension 'ecos' is sensible
SBAT versions values are sensible

Grub is build based on grub:2.06-r2 from Gentoo
My knowledge of Gentoo is limited, but the ebuild file seems to include a patch set from
https://dev.gentoo.org/~floppym/dist/grub-2.06-backports-r1.tar.xz
which includes the security related patches for the latest round
Patches for Grub look fine to me.

Used Linux kernels: 4.14, 5.4, 5.14, 5.17.
4.14 uses lockdown patches from Debian
lockdown functionality was merged in upstream 5.4
According to the README the relevant security patches for Linux are applied

Current Shim is built with a new certificate, so previous Grub versions will not be loaded.

@ecos-platypus
Copy link
Contributor Author

@miray-tf Thank you for your extensive review!

* The GRUB module list includes the PGP module. I currently don't known how multiple verifiers interact, so somebody else might want to have another look at that.

The GRUB verifier framework allows the use of multiple verifier modules in parallel. While it is possible for a verifier module to defer verification to another verifier and skip its own verification, the shim_lock verifier does not allow that. Instead, it insists on verifying all appropriate files (Kernel, EFI chainloader image, ...) by itself. Therefore, those binaries always have to be verified by the shim_lock verifier to be loaded.

* Debian still carries 4 secureboot / lockdown patches. One of them autmoatically locks down the kernel if secureboot is active. Could you please verify that the 5.4, 5.14 and 5.17 are locked down when secureboot is active?

Lockdown is supported by our kernels as they are built with CONFIG_SECURITY_LOCKDOWN_LSM=y and CONFIG_LSM contains lockdown. For the kernels with upstream lockdown support, we activate lockdown via the kernel commandline. GRUB adds lockdown=integrity to the kernel command line before loading a kernel. As outlined in README.md, we secure GRUB by disabling dynamic module loading, embedding grub.cfg in the GRUB binary and requiring a bootloader password for the GRUB command line and editor. Furthermore, the signature of the GRUB binary is verified against our EV certificate before shim loads GRUB.

* Does Gentoo include any Gentoo specific SBAT information when Grub is built? If that is the case you might want to include it in the Grub SBAT section.

Gentoo as source-only distribution only supplies the definition for building. GRUBs built on Gentoo by default do not include any sbat data. Therefore, I did not add Gentoo as intermediate entry. Apart from the latest security patches there are currently 5 short patches that Gentoo applies to upstream GRUB. You could argue that those custom patches may introduce vulnerabilities. So I am also happy to add an intermediate entry for Gentoo if that is preferred.

@miray-tf
Copy link

Until Gentoo chooses a SBAT entry for their distribution it not really possible to add a SBAT entry for a Grub based on their patch set. So I think it is ok to leave that out.

@frozencemetery frozencemetery added new vendor This is a new vendor contact verification needed Contact verification is needed for this review labels Jul 6, 2022
@frozencemetery
Copy link
Member

I am going to send you some words. Please post them here once you receive them.

@ecos-platypus
Copy link
Contributor Author

Thank you. I received the words

bueformetest 
smurningene 
prestasjonsangstene 
nivåhevingen 
utforbakkene 
upåviseligste 
apparatskapa 
bråfort 
kjærtegnet 
utstillingsvarene 
ukjenneligste 
kasseroller 
trøndskene 

@ecos-platypus
Copy link
Contributor Author

Gerald Richter received the words

samboererklæringene
hjelpeløse
leirdeltakerne
stagnasjonen
BNP
oppkreves
synsbilder
multiplere
dødsoner
levneta
profetert
pesticidet
trygderettens

@frozencemetery
Copy link
Member

Those are the correct words; verified on both.

@frozencemetery frozencemetery removed the contact verification needed Contact verification is needed for this review label Jul 7, 2022
@ecos-platypus
Copy link
Contributor Author

ecos-platypus commented Jul 20, 2022

Small update regarding CVE-2022-21505, a kernel lockdown bypass bug using ima_appraise=log (https://seclists.org/oss-sec/2022/q3/57).

It should not affect secure boot assuming that IMA prevents setting ima_appraise=log when booted via secure boot but even if IMA may fail to do so, we are not affected by this bug as

  1. ima_appraise=enforce is always set on the ECOS SBS and
  2. an attacker cannot modify the kernel commandline in GRUB due to the security measures explained in README.md

Regardless, we will incorporate the fix with the next kernel updates.

@ecos-platypus
Copy link
Contributor Author

@frozencemetery I am sorry to ask for your time but would you please consider conducting a final review for our shim? @miray-tf kindly conducted a thorough review which lead to two small issues being resolved.

We started the review process for a new shim in February (#225) with the goal to receive a signed version of our updated shim before the EV certificate embedded in our old shim expired. As a company with a certification from the German Federal Office of Information Security, our business directly depends on being able to quickly deploy security fixes to our customers. Unfortunately, the old EV certificate expired at the end of May. This prevents us from shipping shim and GRUB security updates to our customers and leaves us in a tight spot.

@julian-klode julian-klode added bug Problem with the review that must be fixed before it will be accepted and removed bug Problem with the review that must be fixed before it will be accepted labels Aug 1, 2022
@steve-mcintyre
Copy link
Collaborator

Good stuff:

  • Build reproduces here ok
  • Embedded EV cert looks ok, expires in 2025 (not a CA cert)
  • contact verification is done already
  • shim patches look fine
  • SBAT entries look fine
  • extra grub patches look ok to me
  • AFAICS the gentoo grub you're using has all the CVE fixes needed

Stuff to follow up on:

  • Please try to push your grub patches upstream where sensible!
  • You have iorw in your signed grub build. I'm curious - what are you using it for?

@ecos-platypus
Copy link
Contributor Author

@steve-mcintyre Thank you very much for your review!

Most of our GRUB patches are customized for our usage and disable functionality that other people may need. However, I will take another look at generic patches like gettext-multiple-translations.patch and check whether they add value to upstream.

The iorw module seems to be a dependency from past times that was still defined in our build script. I rebuilt GRUB without it and tested it without issues. I removed the module from our dependency list so that it will not make it into future GRUB builds.

@steve-mcintyre
Copy link
Collaborator

Cool. :-)

I was totally confused why you might have iorw, and it's something I'd be a little worried about from from a SB perspective. It's just too open to random (ab)use AFAICS. But if you have lockdown enabled then it's fine.

I think you're good to go, marking as accepted.

@steve-mcintyre steve-mcintyre added the accepted Submission is ready for sysdev label Aug 8, 2022
@ecos-platypus
Copy link
Contributor Author

Thank you very much! I will close this issue when we have received the signed shim from Microsoft.

Out of curiosity, I did some digging regarding iorw: It was used in the past for disabling certain graphics cards that caused issues in Linux. As the functionality is not being used anymore its a good idea to remove the iorw module to reduce the attack surface. So thanks for catching that :-)

@ecos-platypus
Copy link
Contributor Author

We received the signed shim back from Microsoft. Thanks to anyone involved in making this possible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Submission is ready for sysdev new vendor This is a new vendor
Projects
None yet
Development

No branches or pull requests

5 participants