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 Navix 9 #413

Closed
8 tasks done
leejun9503 opened this issue May 2, 2024 · 21 comments
Closed
8 tasks done

Shim 15.8 for Navix 9 #413

leejun9503 opened this issue May 2, 2024 · 21 comments
Assignees
Labels
accepted Submission is ready for sysdev contacts verified OK Contact verification is complete here (or in an earlier submission) easy to review This submission might be a good place to start for an inexperienced reviewer

Comments

@leejun9503
Copy link

leejun9503 commented May 2, 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/NaverCloudPlatform/shim-review/tree/navix9-shim-x86_64-20240723


What is the SHA256 hash of your final SHIM binary?


20c570a0995f07ed06cf3da856795ab4c968b86b9ef866611b4dd993f9f0ee30 shimx64.efi


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


#370


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)?


#346

@SherifNagy SherifNagy self-assigned this May 14, 2024
@steve-mcintyre steve-mcintyre added the contacts verified OK Contact verification is complete here (or in an earlier submission) label May 29, 2024
@steve-mcintyre steve-mcintyre added the extra review wanted Initial review(s) look good, another review desired label Jun 18, 2024
@steve-mcintyre
Copy link
Collaborator

Review of Shim 15.8 for Navix 9

OK

  • Contact verification done - OK
  • NX bit not set - OK
  • Builds from 15.8 upstream, with no patches - OK
  • shim build reproduces here using the Dockerfile for x64 - OK
  20c570a0995f07ed06cf3da856795ab4c968b86b9ef866611b4dd993f9f0ee30  shimx64.efi
  • key management using an HSM - OK
  • Kernel modules signed with ephemeral key - OK
  • Includes a CA cert, expiring in August 2033 - OK
  Serial Number: 1 (0x1)
  Signature Algorithm: sha256WithRSAEncryption
  Issuer: C = KO, ST = Gyeonggi, O = NAVER Cloud Corp., OU = Linux Engineering, CN = Navix Secure Boot CA, emailAddress = dl_le@navercorp.com
  Validity
      Not Before: Aug 24 12:14:02 2023 GMT
      Not After : Aug 21 12:14:02 2033 GMT
  Subject: C = KO, ST = Gyeonggi, O = NAVER Cloud Corp., OU = Linux Engineering, CN = Navix Secure Boot CA
  • SBAT data looks fine for shim, GRUB,fwupd and UKI - OK
  • Using GRUB 2.06-70 straight from RHEL 9 / OpenELA
    • looks OK for fixes and patches and module list - OK.
    • GRUB SBAT is still 3, which is fine for now - no NTFS module
    • Revocation story looks fine, using SBAT.
  • kernel 5.14.0-362.8.1 with RH-style lockdown patches - OK
  • Submitted before (in shim 15.7 for Navix 8 #346 and shim 15.8 for Navix 8 #370),

Issues / queries

  • One minor issue in the fwupd-efi SBAT. You claim to be using 1.8.10 in
    your code, but the upstream version for fwupd-efi is still listed as
    1.4. Not blocking on this as it's only informational, but please
    double-check and maybe fix this in future.

@steve-mcintyre steve-mcintyre added new vendor This is a new vendor easy to review This submission might be a good place to start for an inexperienced reviewer labels Jun 18, 2024
@steve-mcintyre
Copy link
Collaborator

steve-mcintyre commented Jun 18, 2024

Two more reviews needed, I think. AFAICS previous submissions were never signed so you're still a new vendor by our normal rules. Feel free to correct me if I've missed something here!

@leejun9503
Copy link
Author

Two more reviews needed, I think. AFAICS previous submissions were never signed so you're still a new vendor by our normal rules. Feel free to correct me if I've missed something here!

We finally enrolled MS HW Dev program, so we are going to process previous(Navix 8) shim submission ASAP.
Thanks for the initial review!

@SherifNagy
Copy link
Collaborator

Review of navix9-shim-x86_64-20240502

  • Security contacts looks good, however, better cross signing between contacts would be ideal
  • Keys are stored in FIPS HSM

Shim

  • Uses upstream 15.8 and source hashes matches original hashes
  • SBAT entries from shim looks fine
  • Vendor SBAT entry is at 1
  • Binaries are reproducible using the container image
STEP 15/15: RUN sha256sum /usr/share/shim/15.8-1.el9/x64/shimx64.efi &&     sha256sum shimx64.efi
20c570a0995f07ed06cf3da856795ab4c968b86b9ef866611b4dd993f9f0ee30  /usr/share/shim/15.8-1.el9/x64/shimx64.efi
20c570a0995f07ed06cf3da856795ab4c968b86b9ef866611b4dd993f9f0ee30  shimx64.efi
  • NX flag is not set, because the chain is not yet ready
  • Self signed 2048 bit cert and valid for almost 9 years

GRUB2

  • SBAT looks fine (keeps upstream RHEL/OpenELA grub2)
  • Version currently does not include NTFS patches, but the signed versions also not include the NTFS module so sbat grub,3
  • Module list sound fine

Kernel

  • Ephemeral keys are used for signing kernel modules
  • Lockdown patches are included (keeps upstream RHEL kernel)

Notes

The UKI entries aren't correct, the following are all redundant, one of those is enough and it make sense to be the vendor's entry

linux,1,Red Hat,linux,5.14.0-362.8.1.el9.x86_64,https://bugzilla.redhat.com/
linux,1,OpenELA,linux,5.14.0-362.8.1.el9.x86_64,https://bugs.openela.org/
linux,1,Navix,linux,5.14.0-362.8.1.el9.x86_64,dl_le@navercorp.com

For the following section, why are you keeping centos and not rhel? also openELA isn't a distro as far as I understand, so not sure if it make sense to keep it here as well, will need some feedback from @jsetje and maybe @bluca

linux.centos,1,Red Hat,linux,5.14.0-362.8.1.el9.x86_64,https://bugzilla.redhat.com/
linux.openela,1,OpenELA,linux,5.14.0-362.8.1.el9.x86_64,https://bugs.openela.org/
linux.navix,1,Navix,linux,5.14.0-362.8.1.el9.x86_64,dl_le@navercorp.com
kernel-uki-virt.centos,1,Red Hat,kernel-uki-virt,5.14.0-362.8.1.el9.x86_64,https://bugzilla.redhat.com/
kernel-uki-virt.openela,1,OpenELA,kernel-uki-virt,5.14.0-362.8.1.el9.x86_64,https://bugs.openela.org/"
kernel-uki-virt.navix,1,Navix,kernel-uki-virt,5.14.0-362.8.1.el9.x86_64,dl_le@navercorp.com

Other than those few notes, LGTM, we will need one more reviewer

@leejun9503
Copy link
Author

leejun9503 commented Jul 1, 2024

The UKI entries aren't correct, the following are all redundant, one of those is enough and it make sense to be the vendor's entry
also openELA isn't a distro as far as I understand

Actually, I was following the guideline - https://github.com/rhboot/shim-review/blob/main/docs/submitting.md#31-sbat-data

If you are deriving your source from another vendor's build, also include their SBAT vendor data. This helps the community track the provenance of changes (and bugs), and allows us to efficiently deal with publishing revocations in the future.

So that's why there is openELA's SBAT data and additional redundant entries.
But I also thought it needs feedback from other reviewers.


For the following section, why are you keeping centos and not rhel?

Sorry for the confusion. It was mistake.
I was also checking out CentOS Stream kernel spec file, and somehow I thought "that makes sense" and edited SBATsuffix string from openELA's kernel spec file.
+ Additional Explanation: I test and build shim/kernel/grub on separate machine so that our main build system is clean as possible. the CentOS Stream's SBATsuffix was the only local change by me and that is the mistake.

Thanks for the review!

@jason-rodri
Copy link

Not an official reviewer

Review of navix9-shim-x86_64-20240502


  • Contacts are verified
  • CA good for 9 years
    Not Before: Aug 24 12:14:02 2023 GMT
    Not After : Aug 21 12:14:02 2033 GMT
  • Protection configuration is acceptable

Build analysts


  • Build is reproducible
  • no additional shim patches
  • sha256sum shim-15.8.tar.bz2
    a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9 shim-15.8.tar.bz2
  • sha256sum shimx64.efi - matches
    20c570a0995f07ed06cf3da856795ab4c968b86b9ef866611b4dd993f9f0ee30 shimx64.efi

Patches


  • Kernel patches and lockdown configurations is acceptable
  • Kernel Patches are obtained from the upstream
  • Grub2 does not load the NTFS module so is not vulnerable to CVE-2023-4692/4693

sbats


  • sbat entries look good for shim
  • Is the grub.navix entry suppose to be set to 2, was a previous version revoked?
    grub.navix,2,Navix,grub2,2.06-70.el9_3.1,mailto:dl_le@navercorp.com

Other than the grub sbat entry everything looks good, please provide response to the vendor grub2 sbat entry for my education

@SherifNagy
Copy link
Collaborator

The UKI entries aren't correct, the following are all redundant, one of those is enough and it make sense to be the vendor's entry
also openELA isn't a distro as far as I understand

Actually, I was following the guideline - https://github.com/rhboot/shim-review/blob/main/docs/submitting.md#31-sbat-data

If you are deriving your source from another vendor's build, also include their SBAT vendor data. This helps the community track the provenance of changes (and bugs), and allows us to efficiently deal with publishing revocations in the future.

So that's why there is openELA's SBAT data and additional redundant entries. But I also thought it needs feedback from other reviewers.

For this part, you only need single entry of linux,NUMBER since whatever after the number is just information and comment more or less, we will work a bit on sorting the docs to make it more clearing, but basically , those are all redundant, one is enough "usually the vendor's one", we made same mistake with Rocky but we have plans on fixing that

For the following section, why are you keeping centos and not rhel?

Sorry for the confusion. It was mistake. I was also checking out CentOS Stream kernel spec file, and somehow I thought "that makes sense" and edited SBATsuffix string from openELA's kernel spec file. + Additional Explanation: I test and build shim/kernel/grub on separate machine so that our main build system is clean as possible. the CentOS Stream's SBATsuffix was the only local change by me and that is the mistake.

Thanks for the review!

Cool, can you sort those in the README? we need to track those in #397

@leejun9503
Copy link
Author

Thank you for your review jason!

Other than the grub sbat entry everything looks good, please provide response to the vendor grub2 sbat entry for my education

We will fix our SBAT generation to 1, as this is first shim for Navix 9 and we did not revoked our grub2 using shim.
Thanks for pointing out.


For this part, you only need single entry of linux,NUMBER since whatever after the number is just information and comment more or less, we will work a bit on sorting the docs to make it more clearing, but basically , those are all redundant, one is enough "usually the vendor's one", we made same mistake with Rocky but we have plans on fixing that
Cool, can you sort those in the README? we need to track those in #397

Thank you. I'll update and fix the SBAT data.
But do we have to keep the OpenELA's SBAT? For now, here is our updated SBAT with OpenELA's SBAT.
(Updated kernel's release because my mock config for kernel dist was el9.)

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
systemd,1,The systemd Developers,systemd,252,https://systemd.io/
systemd.navix,1,Navix,systemd,252-18.el9,https://bugs.navercorp.com/
linux,1,Red Hat,linux,5.14.0-362.8.1.el9_3.x86_64,https://bugzilla.redhat.com/
linux.rhel,1,Red Hat,linux,5.14.0-362.8.1.el9_3.x86_64,https://bugzilla.redhat.com/
linux.openela,1,OpenELA,linux,5.14.0-362.8.1.el9_3.x86_64,https://bugs.openela.org/
linux.navix,1,Navix,linux,5.14.0-362.8.1.el9_3.x86_64,dl_le@navercorp.com
kernel-uki-virt.rhel,1,Red Hat,kernel-uki-virt,5.14.0-362.8.1.el9_3.x86_64,https://bugzilla.redhat.com/
kernel-uki-virt.openela,1,OpenELA,kernel-uki-virt,5.14.0-362.8.1.el9_3.x86_64,https://bugs.openela.org/
kernel-uki-virt.navix,1,Navix,kernel-uki-virt,5.14.0-362.8.1.el9_3.x86_64,dl_le@navercorp.com

also openELA isn't a distro as far as I understand, so not sure if it make sense to keep it here as well, will need some feedback from @jsetje and maybe @bluca


But I also thought it needs feedback from other reviewers.

As you said in previous comment, I'm also not sure about keeping OpenELA's SBAT data. so additional feedback is welcome.


Thank you all for your review and corrections.

@leejun9503
Copy link
Author

Two more reviews needed, I think. AFAICS previous submissions were never signed so you're still a new vendor by our normal rules. Feel free to correct me if I've missed something here!

We finally enrolled MS HW Dev program, so we are going to process previous(Navix 8) shim submission ASAP. Thanks for the initial review!

Update : We got our signed shim for Navix 8 - #370

@SherifNagy SherifNagy removed the new vendor This is a new vendor label Jul 22, 2024
@SherifNagy
Copy link
Collaborator

@leejun9503 Can you update the issue / tag itself with your final SBAT entires for your EFI components?

@leejun9503
Copy link
Author

Updated.
And please give us opinion about OpenELA's SBAT data.
https://github.com/NaverCloudPlatform/shim-review/tree/navix9-shim-x86_64-20240723

@SherifNagy
Copy link
Collaborator

It's okay for keep the OpenELA SBAT data, as long as your upstream has them.

@THS-on
Copy link
Collaborator

THS-on commented Jul 29, 2024

@SherifNagy does this still need a review?

@SherifNagy
Copy link
Collaborator

@THS-on Might as well :) more eyes is better, if that's okay with you

@THS-on
Copy link
Collaborator

THS-on commented Jul 29, 2024

Review of navix9-shim-x86_64-20240723

As there are already a decent amount of reviews, only focusing on the main parts.

  • Security contacts have been verified
  • keys are stored in HSM

Shim

  • Based on 15.8 with no patches
  • Embedded CA valid till 2033
  • NX is disabled
  • Is reproducible using Dockerfile:
#19 [15/15] RUN sha256sum /usr/share/shim/15.8-1.el9/x64/shimx64.efi &&     sha256sum shimx64.efi
#19 0.274 20c570a0995f07ed06cf3da856795ab4c968b86b9ef866611b4dd993f9f0ee30  /usr/share/shim/15.8-1.el9/x64/shimx64.efi
#19 0.277 20c570a0995f07ed06cf3da856795ab4c968b86b9ef866611b4dd993f9f0ee30  shimx64.efi
#19 DONE 0.3s

GRUB2 and fwupd

  • Grub based on RHEL
  • SBAT level at 3 (because no NTFS patches, but also is not in module list)
  • module list looks fine

Kernel

  • ephemeral key signing is used
  • Based on 5.14 with the usual lockdown patches

Notes

  • As @steve-mcintyre already mentioned there is a version mismatch in your fwupd version entries. Nothing major, but should be ideally fixed
  • Did you sign any GRUB2s with grub.navix set to 2 before decreasing it'? If so just keep in mind that you might need to bump it to 3 for revoking all old versions.

That shouldn't block the review, so LGTM

@SherifNagy
Copy link
Collaborator

@leejun9503 as @THS-on mentioned, decreasing the grub2.navix from 2 to 1 is really a bad idea especially since you are already using grub2.navix,2 for #370 and #346 , it should be grub2.navix,2 for this submission

@SherifNagy
Copy link
Collaborator

ah no wait, other submission are also grub2.navix,1 as far as I can tell, so if you didn't sign / release any grub2.navix,2 , then it is ok to leave it at grub2.navix,1

But if you did release a signed grub with grub2.navix,2, then this submission also needs to have grub2.navix,2

@SherifNagy SherifNagy removed the extra review wanted Initial review(s) look good, another review desired label Jul 29, 2024
@leejun9503
Copy link
Author

leejun9503 commented Jul 30, 2024

version mismatch in your fwupd version entries

We saw other EL vendor's SBAT entry for reference - Because this is first time we integrating secure boot to Navix 8 and 9. - and they have similar SBAT entry for fwupd and we followed their structure.
Do we missing something other then RHEL's SBAT record?


But if you did release a signed grub

Nope, For now we never released signed grub with grub2.navix,2 to public.

@THS-on
Copy link
Collaborator

THS-on commented Jul 30, 2024

We saw other EL vendor's SBAT entry for reference - Because this is first time we integrating secure boot to Navix 8 and 9. - and they have similar SBAT entry for fwupd and we followed their structure.
Do we missing something other then RHEL's SBAT record?

sbat,1,UEFI shim,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
fwupd-efi,1,Firmware update daemon,fwupd-efi,1.4,https://github.com/fwupd/fwupd-efi
fwupd-efi.navix,1,Navix,fwupd,1.8.10,dl_le@navercorp.com

The upstream entry points to version 1.4 and yours to 1.8.10. In most cases this should be the same. Might be that by importing the package, your version number does not get increased automatically.

Nope, For now we never released signed grub with grub2.navix,2 to public.

Ok then this is fine.

@SherifNagy SherifNagy added the accepted Submission is ready for sysdev label Jul 30, 2024
@SherifNagy
Copy link
Collaborator

Marking accepted

@leejun9503
Copy link
Author

Got signed shim binary from Microsoft. Thank you everyone for reviewing our application.

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 contacts verified OK Contact verification is complete here (or in an earlier submission) easy to review This submission might be a good place to start for an inexperienced reviewer
Projects
None yet
Development

No branches or pull requests

5 participants