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.8 for elux-20230102 (updated to unicon-shim-amd64-20240403) #309

Closed
6 tasks done
christoph-at-unicon opened this issue Jan 2, 2023 · 57 comments
Closed
6 tasks done
Labels
accepted Submission is ready for sysdev new vendor This is a new vendor

Comments

@christoph-at-unicon
Copy link

christoph-at-unicon commented Jan 2, 2023

Confirm the following are included in your repo, checking each box:

  • completed README.md file with the necessary information
  • shim.efi to be signed (as shimx64.efi)
  • public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
  • [n/a] binaries, for which hashes are added to vendor_db ( if you use vendor_db and have hashes allow-listed )
  • any extra patches to shim via your own git tree or as files
  • [n/a] any extra patches to grub via your own git tree or as files
  • build logs
  • 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/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)?


#277
#124

@christoph-at-unicon
Copy link
Author

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 frozencemetery added new vendor This is a new vendor contact verification needed Contact verification is needed for this review labels Feb 16, 2023
@frozencemetery
Copy link
Member

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
Copy link

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
Copy link
Author

@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
Copy link
Member

Both verified

@frozencemetery frozencemetery removed the contact verification needed Contact verification is needed for this review label Feb 17, 2023
@frozencemetery
Copy link
Member

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.

@frozencemetery frozencemetery added the bug Problem with the review that must be fixed before it will be accepted label Feb 17, 2023
@christoph-at-unicon
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

If that is not sufficient, let us know.

It is not.

@christoph-at-unicon
Copy link
Author

Updates:

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

  • Add entry to SBAT definition

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

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

  • Provide SBAT definitions used
  • Updated hashsum of shimx64.efi as another rebuild was needed
  • Updated answer to "If your SHIM launches any other components (...)"

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 #309 accordingly.

@frozencemetery frozencemetery removed the bug Problem with the review that must be fixed before it will be accepted label Mar 6, 2023
@christoph-at-unicon
Copy link
Author

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
Copy link
Author

Another month later, gentle ping?

@steve-mcintyre
Copy link
Collaborator

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
Copy link
Collaborator

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 steve-mcintyre changed the title shim 15.7 for elux-20230102 shim 15.7 for elux-20230102 (updated to unicon-shim-amd64-20230306) Sep 3, 2023
@steve-mcintyre
Copy link
Collaborator

steve-mcintyre commented Sep 3, 2023

Review of Elux unicon-shim-amd64-20230306

OK

  • Contact verification already done
  • shim build reproduces here
  • Builds from 15.7 upstream, with 3 patches applied from upstream - OK
  • Includes a CA key with ~9 years left, ok
  • No previous shims signed, so revocation is easy
  • HSM for key management, good
  • following Debian's grub packages, fine
  • list of grub modules is fine (for now - we may be tweaking what's acceptable soon)
  • SBAT looks ok for grub and fwupd

Issues / queries

  • shim SBAT data is wrong - "grub2" in the shim SBAT data?
  • You mention "out-of-tree modules" for the kernel - more details please!

@steve-mcintyre steve-mcintyre added bug Problem with the review that must be fixed before it will be accepted question Reviewer(s) waiting on response labels Sep 3, 2023
@christoph-at-unicon
Copy link
Author

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
Copy link
Collaborator

OK, SBAT data looks good now

@steve-mcintyre steve-mcintyre removed the bug Problem with the review that must be fixed before it will be accepted label Sep 7, 2023
@steve-mcintyre
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

THS-on commented Mar 11, 2024

Looks like a good test plan 👍

@christoph-at-unicon
Copy link
Author

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

@steve-mcintyre
Copy link
Collaborator

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

@christoph-at-unicon christoph-at-unicon changed the title shim 15.7 for elux-20230102 (updated to unicon-shim-amd64-20240308) shim 15.8 for elux-20230102 (updated to unicon-shim-amd64-20240308) Mar 18, 2024
@christoph-at-unicon
Copy link
Author

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

@THS-on
Copy link
Collaborator

THS-on commented Apr 2, 2024

Review for unicon-shim-amd64-20240308

Shim

  • Build from 15.8 source without NX enabled
  • Certificate
    • Is a CA certificate, has Keysigning/Codesigning attributes not set
    • Matches the organisation
    • Valid till: Aug 16 10:45:51 2032 GMT (ok)
  • SBAT looks fine
  • Does not build because it points to old pem file:
cert.S: Assembler messages:
cert.S:42: Error: file not found: uc-sb-signing.crt.pem
cert.S:55: Warning: setting incorrect section attributes for .note.GNU-stack
make: *** [/shim/Make.rules:39: cert.o] Error 1
Error: building at STEP "RUN make VENDOR_CERT_FILE=uc-sb-signing.crt.pem": while running runtime: exit status 2

Kernel

  • Based on 6.1 and 6.6
  • Uses emphermal key signing for modules
  • Is build with out-of-tree modules
  • Has the two major lockdown patches applied (the arm and mtd one are missing)

GRUB2

No new comments

Questions and comments

@christoph-at-unicon
Copy link
Author

christoph-at-unicon commented Apr 3, 2024

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.

@christoph-at-unicon christoph-at-unicon changed the title shim 15.8 for elux-20230102 (updated to unicon-shim-amd64-20240308) shim 15.8 for elux-20230102 (updated to unicon-shim-amd64-20240403) Apr 3, 2024
@bombadil
Copy link

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
Copy link
Collaborator

THS-on commented Apr 15, 2024

@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.

@THS-on THS-on added extra review wanted and removed bug Problem with the review that must be fixed before it will be accepted question Reviewer(s) waiting on response labels Apr 15, 2024
@bombadil
Copy link

@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
Copy link
Collaborator

THS-on commented Apr 29, 2024

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
Copy link
Author

christoph-at-unicon commented Apr 30, 2024

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
Copy link

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
Copy link
Author

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

@steve-mcintyre
Copy link
Collaborator

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.

@steve-mcintyre steve-mcintyre added accepted Submission is ready for sysdev and removed extra review wanted labels May 27, 2024
@christoph-at-unicon
Copy link
Author

Thank you :-)

@THS-on
Copy link
Collaborator

THS-on commented Jun 21, 2024

@christoph-at-unicon did you get a signed shim back?

@christoph-at-unicon
Copy link
Author

christoph-at-unicon commented Jun 25, 2024

No, we haven't received anything yet.

There is an information about a Microsoft Account in submitting.md, it seems that bit eluded us. While our organization does have an account there, it seems to be necessary to sign into the Hardware Developer Program. And we were not aware the signed shim would be distributed via a portal at Microsoft. All this came somewhat as a surprise.

Additionally, signing up is currently not possible, we're seeing the problems described in the thread at https://techcommunity.microsoft.com/t5/partner-led-questions-tech/can-t-register-for-microsoft-hardware-developer-program/m-p/4109547#M165 - despite the positive message from May 1st, it seems to be broken again.

If you can help resolving this, that would be great. At the moment I cannot even think how Microsoft could link this shim review request here to our organization's account. It might have been possible using the domain name, but I'd expect to be asked for an account ID at some time.

We've also contacted Microsoft about this, support request 2406250040004058

@THS-on
Copy link
Collaborator

THS-on commented Jun 28, 2024

There is an information about a Microsoft Account in submitting.md, it seems that bit eluded us. While our organization does have an account there, it seems to be necessary to sign into the Hardware Developer Program. And we were not aware the signed shim would be distributed via a portal at Microsoft. All this came somewhat as a surprise.

The signing is done via the Hardware Dev program from Microsoft as they are managing the Secureboot CA. So yes you will need to sign up for that and fill out the questionnaire that they will send you.

Additionally, signing up is currently not possible, we're seeing the problems described in the thread at https://techcommunity.microsoft.com/t5/partner-led-questions-tech/can-t-register-for-microsoft-hardware-developer-program/m-p/4109547#M165 - despite the positive message from May 1st, it seems to be broken again.

While you already have a ticket open with them, might make sense to contact them via [email protected].

If you can help resolving this, that would be great. At the moment I cannot even think how Microsoft could link this shim review request here to our organization's account. It might have been possible using the domain name, but I'd expect to be asked for an account ID at some time.

You will need to provide your shim binary signed with an EV certificate that proofs that you are the actual legal entity submitting this shim via the Hardware Dev portal. Then they are cross referencing this with this submission here.

@THS-on
Copy link
Collaborator

THS-on commented Jul 29, 2024

@christoph-at-unicon did you have success resolving the issue and were able to get a signed shim back?

@christoph-at-unicon
Copy link
Author

@THS-on Thanks for asking, unfortunately we're not there yet. Our IT department is still working on enrolling for the Hardware Dev program, at least signing up was possible again. Still, there's nothing you can do about it as far as I can see.

@christoph-at-unicon
Copy link
Author

@THS-on We've received the signed shim a few minutes ago.

@THS-on
Copy link
Collaborator

THS-on commented Aug 22, 2024

@christoph-at-unicon ah great closing this now.

@THS-on THS-on closed this as completed Aug 22, 2024
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

7 participants