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 Shenxinda Security OS #449

Open
8 tasks done
lichengfxf opened this issue Oct 29, 2024 · 10 comments
Open
8 tasks done

Shim 15.8 for Shenxinda Security OS #449

lichengfxf opened this issue Oct 29, 2024 · 10 comments
Assignees
Labels
3 reviews needed Needs 3 successful reviews before being accepted Accredited review needed Needs a successful review by an accredited reviewer contacts verified OK Contact verification is complete here (or in an earlier submission) new vendor This is a new vendor question Reviewer(s) waiting on response

Comments

@lichengfxf
Copy link

lichengfxf commented Oct 29, 2024

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

  • completed README.md file with the necessary information
  • shim.efi to be signed
  • public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
  • 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
  • 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/lichengfxf/shim-review/tree/shenxinda-shim-x64_ia32_aa64-20241127


What is the SHA256 hash of your final SHIM binary?


6201939687039105a0b53063f9f1a7586b48c5a1a78a44810475cb66d170df1e  shimaa64.efi
00b0dd514f05a269068461b4124cda5a77bc90ee04a86681e22f98f0592f5c58  shimia32.efi
44a44161d9096938d14bb8f62a64bd51bde1a1b255fc16b7fa80f864057efa47  shimx64.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
Copy link
Collaborator

The application is missing some bits:

  • the legal data entries: 5eef3db
  • in the listing of your shim SBAT entries, there's no entry like shim.shenxinda,1

Please, fix them first. Then I'll proceed with reviewing.

@aronowski aronowski self-assigned this Nov 10, 2024
@aronowski aronowski added new vendor This is a new vendor incomplete This submission is missing required bits contact verification needed Contact verification is needed for this review labels Nov 10, 2024
@lichengfxf lichengfxf reopened this Nov 11, 2024
@lichengfxf
Copy link
Author

lichengfxf commented Nov 11, 2024

The application is missing some bits:

  • the legal data entries: 5eef3db
  • in the listing of your shim SBAT entries, there's no entry like shim.shenxinda,1

Please, fix them first. Then I'll proceed with reviewing.

Thanks for finding these issues, I have fixed them, the corrected tag are here: https://github.com/lichengfxf/shim-review/tree/shim15.8-20241111

Thank you again for your review @aronowski

@aronowski aronowski removed the incomplete This submission is missing required bits label Nov 11, 2024
@steve-mcintyre steve-mcintyre added contact verification pending Contact verification emails have been sent, waiting on response and removed contact verification needed Contact verification is needed for this review labels Nov 12, 2024
@steve-mcintyre
Copy link
Collaborator

Contact verification mails on the way

@lichengfxf
Copy link
Author

Hi,

Contact verification for [email protected]

anchorpersons denuded heroes prescriptive handrails Paramaribo teaspoonfuls bother Crockett detractor

@CG790725
Copy link

Hi,

Contact verification for [email protected]

renascence unmasking stagflation teaspoonful ghastliest deliberation hothouse monarchy rank imposing

@steve-mcintyre
Copy link
Collaborator

Contact verification complete

@steve-mcintyre steve-mcintyre added contacts verified OK Contact verification is complete here (or in an earlier submission) 3 reviews needed Needs 3 successful reviews before being accepted Accredited review needed Needs a successful review by an accredited reviewer and removed contact verification pending Contact verification emails have been sent, waiting on response labels Nov 13, 2024
@aronowski
Copy link
Collaborator

Review as per shim15.8-20241111, commit bb35eb62c7e84b0b2df5b318f0807e4a6887e212.

Signed shim needed for a kernel (version 6.6.56) with customized enabled modules, though no additional patches.

Minor nitpick: the tag isn't of the form as README.md mentions, i.e.:

tag it with a tag of the form "myorg-shim-arch-YYYYMMDD"


The first thing that needs to be fixed, is that your vendor certificate is in PEM format. Future shim releases should detect this automatically and complain during the build process, so the thing can get fixed by the time the application gets published. See rhboot/shim#646 for more details.

Once that's fixed, I'll perform another review. In the meantime I'd like to ask the questions below.


*******************************************************************************
### Do you use an ephemeral key for signing kernel modules?
### If not, please describe how you ensure that one kernel build does not load modules built for another kernel.
*******************************************************************************
Yes, We use an ephemeral key to sign kernel modules

[...]

*******************************************************************************
### How do the launched components prevent execution of unauthenticated code?
Summarize in one or two sentences, how your secure bootchain works on higher level.
*******************************************************************************
shim verifies signature of grub, grub verifies signature of kernel. Grub also use a builtin GPG key that ensures that all grub configs and initrd cannot be modified. Kernel is compiled with CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY, CONFIG_MODULE_SIG_FORCE, CONFIG_INTEGRITY_PLATFORM_KEYRING and CONFIG_INTEGRITY_MACHINE_KEYRING. Kernel modules are signed using the same vendor keypair used inside shim image.

So the kernel modules are double-signed? If so, this is wrong - see #362 (comment) for details.


HSM is used - any more details on that? What model is it? Who/what (people, signing nodes, system identities) has access to it?


*******************************************************************************
### URL for a repo that contains the exact code which was built to result in your binary:
Hint: If you attach all the patches and modifications that are being used to your application, you can point to the URL of your application here (*`https://github.com/YOUR_ORGANIZATION/shim-review`*).

You can also point to your custom git servers, where the code is hosted.
*******************************************************************************
https://github.com/lichengfxf/shim-review

Since you're not using any patches for shim, I imagined the answer being just the upstream URL straight from your Dockerfile:

ARG SHIM_ARCHIVE_URL=https://github.com/rhboot/shim/releases/download/15.8/shim-15.8.tar.bz2

Maybe it's worth rewriting, if it causes confusion. That could be a contribution, which helps with reviewing too (less confusion for future applicants).

@aronowski aronowski added bug Problem with the review that must be fixed before it will be accepted question Reviewer(s) waiting on response labels Nov 18, 2024
@lichengfxf
Copy link
Author

@aronowski Thank you again for your review!

  1. I have changed the certificate to DER format, in the new tag: https://github.com/lichengfxf/shim-review/tree/shenxinda-shim-x64_ia32_aa64-20241121

  2. Kernel modules are NOT double-signed.
    There might be some misunderstanding on my part regarding the ephemeral key. Let me describe the steps we take to sign the kernel modules and see if there is any issue:
    We enable kernel module signature verification using CONFIG_MODULE_SIG and specify the key file for signing kernel modules using CONFIG_MODULE_SIG_KEY, when make modules_install it will automatically sign all kernel modules.
    We use pesign to sign the kernel image file vmlinux (excluding kernel modules), and GRUB is responsible for verifying the signature of the kernel image (excluding kernel modules). When the kernel loads the kernel modules, it verifies the signatures of the kernel modules.

  3. The model of the HSM is SafeNet eToken 5110 CC (940). Only the CTO can access it.

  4. Indeed, it was my carelessness; the correct answer should be: https://github.com/rhboot/shim/releases/download/15.8/shim-15.8.tar.bz2

@aronowski
Copy link
Collaborator

Review as per shenxinda-shim-x64_ia32_aa64-20241121.


I have changed the certificate to DER format, in the new tag: https://github.com/lichengfxf/shim-review/tree/shenxinda-shim-x64_ia32_aa64-20241121

That's OK. Although a bit worried about the certificate's strength vs lifecycle - it's a 2048-bit RSA key valid for the next 30 years.


Kernel modules are NOT double-signed.
There might be some misunderstanding on my part regarding the ephemeral key. Let me describe the steps we take to sign the kernel modules and see if there is any issue:
We enable kernel module signature verification using CONFIG_MODULE_SIG and specify the key file for signing kernel modules using CONFIG_MODULE_SIG_KEY, when make modules_install it will automatically sign all kernel modules.
We use pesign to sign the kernel image file vmlinux (excluding kernel modules), and GRUB is responsible for verifying the signature of the kernel image (excluding kernel modules). When the kernel loads the kernel modules, it verifies the signatures of the kernel modules.

That's OK. Thanks for clarifying!


We are downloading grub sources from official website: http://ftp.gnu.org/gnu/grub We are using the version: 2.06 .

Alright. This is the GRUB2 SBAT entry mentioned in the application (also OK):

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,2.06,https://www.gnu.org/software/grub/
grub.shenxinda,1,shenxinda,grub,2.06,mail:[email protected]

But the list of modules shipped with GRUB2 is:

part_gpt part_msdos fat memdisk squash4 iso9660 cpio loopback keylayouts at_keyboard all_video gfxterm terminal font gettext echo regexp cat gcry_sha256 gcry_sha512 gcry_dsa gcry_rsa password_pbkdf2 pbkdf2 efinet tftp http linux boot halt reboot minicmd sleep test gzio normal configfile peimage

In particular, the peimage module is listed. As far as I understand, this is a backport from the Debian/Ubuntu patchset used for the respective downstream GRUB 2.12 builds.

In this case I'm wondering, if the SBAT entry should also mention grub.peimage like in Canonical's build, below grub.shenxinda. After all, CVE-2024-2312 was the reason, why both product-specific generation numbers got bumped.


I'll remove the "bug" label, and ask other reviewers for help.

@aronowski aronowski removed the bug Problem with the review that must be fixed before it will be accepted label Nov 26, 2024
@lichengfxf
Copy link
Author

lichengfxf commented Nov 27, 2024

@aronowski Thanks for finding this issue

In particular, the peimage module is listed. As far as I understand, this is a backport from the Debian/Ubuntu patchset used for the respective downstream GRUB 2.12 builds.

This is my mistake; in fact, we did not use peimage, we used chain. I got it wrong, and I will correct it later.


Got it fixed in the new tag: https://github.com/lichengfxf/shim-review/tree/shenxinda-shim-x64_ia32_aa64-20241127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 reviews needed Needs 3 successful reviews before being accepted Accredited review needed Needs a successful review by an accredited reviewer contacts verified OK Contact verification is complete here (or in an earlier submission) new vendor This is a new vendor question Reviewer(s) waiting on response
Projects
None yet
Development

No branches or pull requests

4 participants