search

rhboot / shim-review

Reviews of shim
66 stars 128 forks source link

shim 15.8 for elux-20230102 (updated to unicon-shim-amd64-20240403) #309

Closed christoph-at-unicon closed 2 weeks ago

christoph-at-unicon 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/UniconSoftware/shim-review/releases/tag/unicon-shim-amd64-20240308

What is the SHA256 hash of your final SHIM binary?

78a979f518efdcc0c43f7cc8b49c369bf601182dd37f9d3ade9f139befe3b1ef

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

https://github.com/rhboot/shim-review/issues/277 https://github.com/rhboot/shim-review/issues/124

christoph-at-unicon commented 1 year ago

The answer to "If your GRUB2 launches any other binaries that are not the Linux kernel in SecureBoot mode, please provide further details on what is launched and how it enforces Secureboot lockdown." needs an update: We will also start the fwupdx64.efi binary to do firmware updates. We will use the program as provided by Ubuntu's fwupd-signed package. We might re-sign it after verification in case there's a technical need for that.

Besides that, can we do anything to give this issue more progress?

frozencemetery commented 1 year ago

There's a nonbreaking space in the fingerprint for Jan... not a problem really but might be worth looking into how that got there. Other than that, I've sent words; post here when received.

bombadil commented 1 year ago

I got the following shim words

fjeskende
stemmegivinger
ankesakene
åtejakten
førstedanserinnene
inntakslister
storaktørens
Mykens
julestillheter
statsløyser
omfangene
monologene
bomkasta
christoph-at-unicon commented 1 year ago

@frozencemetery Thanks for spotting this, must have happened when Jan sent the fingerprint to me, possibly some mail user agent wanted to do something good. Oh well.

Apart from that, Jan's words were:

avgiftsforvaltning
høgnivåa
lover
ferdselsårene
overskuddskraft
kalkstein
boggivognen
pålegginger
blautkoker
kruggeteste
horaklen
lunsjtilbuda
fargemetode
frozencemetery commented 1 year ago

Both verified

frozencemetery commented 1 year ago

The shim repo you're using isn't based on upstream's. As a reviewer, what I need to accomplish is diffing the upstream repo to yours and reviewing all the changes - right now this is difficult.

In response to SBAT, you said "We do not use this feature." SBAT is not optional. Stopping review.

christoph-at-unicon commented 1 year ago

Given the question "Were these binaries created from the 15.7 shim release tar?" we assumed our repository should be based on that (unpacked) tarball as well. FWIW, the import was done into the "upstream" branch as "Import release shim-15.7".

About SBAT, the wording might have been a bit unlucky. As stated, we just use the grub package as provided by Debian with adding a few more modules as the only deviation. Therefore we include Debian's SBAT file. If that is not sufficient, let us know.

frozencemetery commented 1 year ago

Given the question "Were these binaries created from the 15.7 shim release tar?" we assumed our repository should be based on that (unpacked) tarball as well.

That is the intent of that question, yes, except that you unpacked it into git and checked that in for some reason. Either start from the tarball and apply patches, or start from a proper fork, please.

christoph-at-unicon commented 1 year ago

To resolve that situation, I've rebased the repository on top of the 15.7 tag. As this requires a rebuild of the shim binary anyway, I also included

commit 89972ae25c133df31290f394413c19ea903219ad
CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper

as it seemed wise to do so.

The repository at https://github.com/UniconSoftware/shim-review was updated accordingly (README.md, shimx64.efi, build logs).

frozencemetery commented 1 year ago

If that is not sufficient, let us know.

It is not.

christoph-at-unicon commented 1 year ago

Updates:

In https://github.com/UniconSoftware/shim:

In https://github.com/UniconSoftware/shim-review:

(tag: https://github.com/UniconSoftware/shim-review/releases/tag/unicon-shim-amd64-20230306)

There's also a new tag "unicon-shim-amd64-20230306" that covers all the recent changes. Let me know if should update the initial request at https://github.com/rhboot/shim-review/issues/309 accordingly.

christoph-at-unicon commented 1 year ago

Hello, it's been a while since the last update on this story. Is there anything to do on our side to keep the process going?

christoph-at-unicon commented 1 year ago

Another month later, gentle ping?

steve-mcintyre commented 1 year ago

Apologies for delays...

Picking this up now, I can't reproduce your checksums; it seems that Ubuntu Focal has moved on and is now using

gcc-9 (9.4.0-1ubuntu1~20.04.2)

which is probably to blame. :-( Could you update your submission to match please?

Carrying on with the rest of the review here in anticipation of that...

steve-mcintyre commented 1 year ago

Apologies for delays...

Picking this up now, I can't reproduce your checksums; it seems that Ubuntu Focal has moved on and is now using

gcc-9 (9.4.0-1ubuntu1~20.04.2)

which is probably to blame. :-( Could you update your submission to match please?

Carrying on with the rest of the review here in anticipation of that...

In fact, ignore that - I was still on the old tag based on the title of your submission. Moving forwards to unicon-shim-amd64-20230306, things reproduce here.

steve-mcintyre commented 1 year ago

Review of Elux unicon-shim-amd64-20230306

OK

Issues / queries

christoph-at-unicon commented 1 year ago

Hello,

SBAT was indeed wrong. I rebuild the shim binary and updated https://github.com/UniconSoftware/shim-review/releases/tag/unicon-shim-amd64-20230306 accordingly.

About out-of-tree modules, assuming you want to know which ones:

We are aware out-of-tree kernel modules are likely to be frowned upon as they are not under the same amount of critical review as the Linux kernel, and therefore increase the surface for an attack against the entire concept of secure boot. So we limit this to the modules above who are in wide-spread use, and are also provided by the big distros. Additionally, we monitor these sources to learn about critical updates in a timely manner.

For the same reasons we exempt some other out-of-tree modules from signing. This is about modules that come from other places or directly from a vendor. An according exclude list has already been implemented in our signing script.

steve-mcintyre commented 1 year ago

OK, SBAT data looks good now

steve-mcintyre commented 1 year ago

We are aware out-of-tree kernel modules are likely to be frowned upon as they are not under the same amount of critical review as the Linux kernel, and therefore increase the surface for an attack against the entire concept of secure boot. So we limit this to the modules above who are in wide-spread use, and are also provided by the big distros. Additionally, we monitor these sources to learn about critical updates in a timely manner.

As you guessed, there is distinct worrying about signing of third-party modules. How are you implementing that signing at the moment? It would reduce risk substantially if you're using a limited-use key for those signatures; e.g. per-system, per-department or (maybe best in your case?) a per-kernel build key.

That would make it much easier to control things later via revocation / SBAT if that becomess necessary.

Maybe @vathpela could expand more on this...

christoph-at-unicon commented 12 months ago

Current signing implementation was inspired by the description how Debian is doing this. So there is a dedicated box, with the key on a hardware token.

About the alternatives you've suggested: Per-system is not an option if this requires manually enrolling an extra certificate into each machine's store.

So one approach might be: We'll introduce a second signing key and use that one to sign these out-of-tree modules. That key will be handled with the same precaution as the first one (HSM, limited physical access). And we'll add it the our kernel's trust store so for the users things work just fine. Is that acceptable for you?

christoph-at-unicon commented 11 months ago

About two messages above, you suggested to use per-build keys for the kernel modules. After I've learned about more advantages of such an approach: We could change our build system accordingly for both in-tree and out-of-tree modules. As this will be some work on our side I'd have to justify: Would that settle your concerns?

THS-on commented 11 months ago

@christoph-at-unicon have a look at https://github.com/rhboot/shim-review/issues/292. We had also the issue, that we need some out-of-tree modules.

In the end your current approach is include a new temporary self signed certificate for each kernel build which is used for signing and bump the ABI name for each Kernel build.

THS-on commented 10 months ago

@christoph-at-unicon we discussed on how we are going forward. If you can use per-build keys for kernel modules this should be fine. Let me know if you have any specific questions.

christoph-at-unicon commented 10 months ago

Okay, we'll got that way then. The documentation on this looks pretty straight-forward, still I'll let you know if I think I could use some help.

christoph-at-unicon commented 9 months ago

Status: We are in the final steps of converting our buld process to use ephemeral keys to sign kernel modules. Is there anything else left to do on our side?

julian-klode commented 9 months ago

So I added the question about ephemeral keys but it doesn't necessarily provide guidance to do that safely and a CA key and MODVERSIONS may be safer for many people.

There are two tricky parts you need to make absolutely sure of with ephemeral keys which can be hard:

We need to follow-up and revisit the question to make sure that people using ephemeral keys take that into account.

People using CA keys in a HSM for example don't need to care about these things at all.

christoph-at-unicon commented 8 months ago

the kernel build environment must have enough entropy for the key to be random enough

Our build infrastructure is based on Ubuntu 22.04, and I strongly doubt lack of entropy is an issue there: The Linux kernel re-worked entropy availability somewhen in 2022, and it was backported to all stable kernel versions, and verdors picked that up as well, including for the kernel we're using. Since then, I've never ever seen entropy getting even slightly drained, and if that was possible to do so in a way this becomes an issue, I'd like to learn about it.

you must make sure the key doesn't remain after you build, on disk or in memory such that it could be extracted at a later point.

About file system and build artifacts, that's an important point, and it had already been taken care of.

Our build infrastructure is ephemeral by design by using file system snapshots (btrfs), they are discarded immediately after a build. Other measures include restricting access: The machine is not reachable from the internet, network access is possible via a proxy, and log in is possible for a small subset of the staff only.

THS-on commented 6 months ago

@christoph-at-unicon what is the current status?

Please in the meantime update this issue to 15.8 or create a new submission for 15.8

christoph-at-unicon commented 6 months ago

Quite frankly, we have implemented the changes wrt ephemeral signing keys, and have been waiting for a reaction on your side. About forwarding to 15.8, I am already working on it and will update the issue ASAP.

THS-on commented 6 months ago

Ah sorry, I still thought this was work-in-progress. I'll have a look again once updated to 15.8

christoph-at-unicon commented 6 months ago

Submission now updated to 15.8, check out https://github.com/UniconSoftware/shim-review/releases/tag/unicon-shim-amd64-20240301

Kind regards,

THS-on commented 6 months ago

Review of unicon-shim-amd64-20240301

Shim

GRUB2

We haven't released a shim yet, so possibly this doesn't apply anyway.

The general idea about security updates is: By using the latest sources provided by Ubuntu/Debian (see above), we assume we are not affected by any of these issues listed above.

This still applies to you because old GRUB2 versions will be revoked at some point, which then also affects you.

Similarly don't assume that patches are included directly in the distributions. Historically patchsets of the major distributions can vary quite a bit from upstream and each other.

The used bootloader is grub2, sources are taken from, as mentioned above:

  • "current" Debian 11 ("bullseye"), version 2.06-3~deb11u5.
  • "future" Ubuntu 22.04 ("jammy"), version 2.06-2ubuntu14.4.

Ths section still contains the old version numbers.

Questions and Notes

Does your SHIM load any loaders that support loading unsigned kernels (e.g. GRUB2)? Yes, we load grub2.

The wording of the question might not be ideal, but currently you are stating that you use a GRUB2 that supports unsigned kernels, which is concerning.

christoph-at-unicon commented 6 months ago 
* Shim includes a PEM encoded CA certificate. Pretty sure this will not work, aborting review at this point. See e.g. with `objcopy shimx64.efi /dev/null --dump-sect .vendor_cert=/dev/stdout`

Gotcha. I've rebuilt the shim and set a new tag (unicon-shim-amd64-20240308)

GRUB2

We haven't released a shim yet, so possibly this doesn't apply anyway. The general idea about security updates is: By using the latest sources provided by Ubuntu/Debian (see above), we assume we are not affected by any of these issues listed above.

This still applies to you because old GRUB2 versions will be revoked at some point, which then also affects you.

Noted, and I strongly doubt I'll miss that moment. As this will affect both Debian bullseye and Ubuntu jammy as well, and we'll learn it once they upgrade.

Similarly don't assume that patches are included directly in the distributions. Historically patchsets of the major distributions can vary quite a bit from upstream and each other.

Yes, I've inspected the patches carried in Debian and Ubuntu and noticed they are not just numerous but also differ widely.

Still, the basic idea of following these was to keep maintenance work on our side low (just rebuild upon security updates), and create minimal reason for concerns and questions on yours (as you've already known them).

The used bootloader is grub2, sources are taken from, as mentioned above:

  • "current" Debian 11 ("bullseye"), version 2.06-3~deb11u5.
  • "future" Ubuntu 22.04 ("jammy"), version 2.06-2ubuntu14.4.

Ths section still contains the old version numbers.

Just an editorial error, fixed in README.md

Questions and Notes

* Please update initial comment and title to reflect the update

Done

* How do you now handle signing of out-of-tree modules?

This is now as described on Jan 9th and earlier:

We are using an ephemeral signing key that is invalidated as part of the build process, as soon as it is now longer needed.

Our build infrastructure is ephemeral by design by using file system snapshots (btrfs), they are discarded immediately after a build. Other measures include restricting access: The machine is not reachable from the internet, outbound network access is possible via a proxy, and log in is possible for a small subset of the staff only.

* Have you actually tested your Shim + GRUB2 + Kernel setup?

Yest, test procedure included signing the shim and adding the signing certificate to the BIOS' DB store.

Does your SHIM load any loaders that support loading unsigned kernels (e.g. GRUB2)? Yes, we load grub2.

The wording of the question might not be ideal, but currently you are stating that you use a GRUB2 that supports unsigned kernels, which is concerning.

Well, I was following the question literally, and since grub2 does in general support loading unsigned kernels, "yes" is the only legitimate answer.

Now guessing, the question might be rather whether any of the components we use in the boot process allows breaking out of the trusted execution boot chain, for example loading an unsigned kernel while booting in secure boot mode: I am certain grub2 does not support this as it would defeat the whole purpose of secure boot, and the answer is "no" then.

THS-on commented 6 months ago

Still, the basic idea of following these was to keep maintenance work on our side low (just rebuild upon security updates), and create minimal reason for concerns and questions on yours (as you've already known them).

Yes this is definitely a very good strategy to just follow the major distributions. Just make sure on what general patch level the distribution is.

Yest, test procedure included signing the shim and adding the signing certificate to the BIOS' DB store.

Ah now I see how this happened. The shim first tries to validate the next component (i.e. GRUB2) via the DB entries and then via its own CA. You would need to sign the shim with a different key and certificate then GRUB etc. The Gentoo wiki has good documentation on how to setup a UEFI with completely custom keys: https://wiki.gentoo.org/wiki/Secure_Boot

Well, I was following the question literally, and since grub2 does in general support loading unsigned kernels, "yes" is the only legitimate answer.

Now guessing, the question might be rather whether any of the components we use in the boot process allows breaking out of the trusted execution boot chain, for example loading an unsigned kernel while booting in secure boot mode: I am certain grub2 does not support this as it would defeat the whole purpose of secure boot, and the answer is "no" then.

It is possible to build GRUB2 without the lockdown features, or if you are using a really old version that is not Secure Boot aware, hence this question.

Can you verify that the chain works with the Shim CA not in the DB? If so, I'll take a closer look at the updated submission.

christoph-at-unicon commented 6 months ago

Yest, test procedure included signing the shim and adding the signing certificate to the BIOS' DB store.

Ah now I see how this happened. The shim first tries to validate the next component (i.e. GRUB2) via the DB entries and then via its own CA. You would need to sign the shim with a different key and certificate then GRUB etc. The Gentoo wiki has good documentation on how to setup a UEFI with completely custom keys: https://wiki.gentoo.org/wiki/Secure_Boot

Test plan boils to

Can you verify that the chain works with the Shim CA not in the DB? If so, I'll take a closer look at the updated submission.

Let me know if the above is not sufficient to you.

THS-on commented 6 months ago

Looks like a good test plan :+1:

christoph-at-unicon commented 5 months ago

Thanks :) If there's anything else you need from us, tell us right away.

steve-mcintyre commented 5 months ago

If you're on 15.8, could you update the title of your issue too please?

christoph-at-unicon commented 5 months ago

Sure thing. (Already did on Monday but I'm not sure whether you've received a notification about it.)

THS-on commented 5 months ago

Review for unicon-shim-amd64-20240308

Shim

Kernel

GRUB2

No new comments

Questions and comments

christoph-at-unicon commented 5 months ago

Hello,

you mentioned the missing arm patch, so as a clarification: We only build kernels for the x86_64 aka amd64 architecture.

About your questions:

How is your certificate structure for signing, i.e what leaf certificates are you using for GRUB2 and kernel?

We did not build a chain with intermediates here. The certificate is the one related to the key on the HSM.

Your Dockerfile still points to the old pem file

Sorry for that, pushed the missing change now.

Please fix the anweser to "Does your SHIM load any loaders that support loading unsigned kernels (e.g. GRUB2)?"

Done.

As I considered moving the git tag a bad idea, I set a new tag, and updated this issue here. The shimx64.efi itself did not change, checked via a rebuild.

You might also want to pick this lockdown patch: https://salsa.debian.org/kernel-team/linux/-/blob/master/debian/patches/features/all/lockdown/mtd-disable-slram-and-phram-when-locked-down.patch?ref_type=heads

Good to know. CONFIG_MTD is disabled in our kernels so it's not needed here.

Still, we're working on a check that fails a build should a particular option ever change (we need that feature for some other options as well), and that option is on the list now.

Make sure that you actually build with LOCK_DOWN_IN_EFI_SECURE_BOOT set, because picking the patches is not enough. Is your kernel repo somewhere public?

We were aware of that and so that option was already set all the time. Also, we will enforce that as above.

The kernel repository is not public, I'm afraid.

In other news, I'll be mostly out of office the next days. Please accept answers from @bombadil as well, that's Micha Lenk, the secondary contact in the README.md. Feel free to verify his identify using openpgp.

bombadil commented 5 months ago

Hello,

if I understand correctly all questions and comments you had have been answered now.

I see the bug and question labels are still present. Is there anything else left for us to fix or answer?

Kind regards, Micha

THS-on commented 4 months ago

@bombadil @christoph-at-unicon sorry for only getting back to you now.

How is your certificate structure for signing, i.e what leaf certificates are you using for GRUB2 and kernel?

We did not build a chain with intermediates here. The certificate is the one related to the key on the HSM.

You should consider to not sign the actual components with the CA certificate, because then you need to change the CA certificate every time you want to deprecate your signing key etc.

The tag unicon-shim-amd64-20240403 is reproducible and the things open are now clarified.

I see the bug and question labels are still present. Is there anything else left for us to fix or answer?

No, besides the CA usage. So I'll remove them and lets get some more reviews from others.

bombadil commented 4 months ago

@THS-on, okay, on CA usage we discussed this internally, and we can see the point why we would better use a separate signing key instead of signing with the CA key directly. However, I think doing so is more a procedural change on our end and will not change any public key material relevant for the shim review, correct?

If I misunderstood something, and there is additional need for changes from our end, please let us know.

We're patiently awaiting the rest of the review then...

THS-on commented 4 months ago

However, I think doing so is more a procedural change on our end and will not change any public key material relevant for the shim review, correct?

Yes, assuming you not have released anything signed with the CA keys.

christoph-at-unicon commented 4 months ago

Yes, assuming you not have released anything signed with the CA keys.

For clarification, so far we have not released anything of that kind. Edit: We've updated our signing process and will use an intermediate signing certificate now.

realnickel commented 3 months ago

I am not an official reviewer, looking at latest tag: https://github.com/UniconSoftware/shim-review/tree/unicon-shim-amd64-20240403 I can confirm that:

Contact verification already done Shim sources match to upstream 15.8 with patches introducing VENDOR_CERT and vendor specific SBAT section Build reproduces binary with same sha256sum and containing same cert and SBAT. Includes a CA key valid until Aug 16, 2032 No previous shims signed (mentioned previous submissions were obsoleted before signing) HSM for key management, good Following Debian's grub packages, fine. List of grub modules contains 'ntfs'. SBAT looks ok for grub and fwupd. Have a question for shim SBAT below.

Question: I noticed that versions of grub and fwupd-efi follow the version-release naming scheme in SBAT. But shim uses only version. Is it intentional? Do eLux release a shim package?

Nitpick: eLux shim source code repository not forked from rhboot/shim, while contains same code as base. Guess it could be slightly easier/faster to check a fork.

Additional question duplicating previous reviewer: is your kernel repo somewhere public?

christoph-at-unicon commented 3 months ago

Greetings, I noticed the "extra review wanted" label. Is there anything in particular you're waiting for from our side?

steve-mcintyre commented 3 months ago

I think we're way overdue just accepting things here. I don't think there's anything important outstanding at this point.

I've just rechecked and things reproduce OK.

Accepted.

christoph-at-unicon commented 3 months ago

Thank you :-)