rhboot / shim-review

Reviews of shim
66 stars 130 forks source link

Shim 15.8 for UOS Linux (x86_64) #431

Open kyrie-z opened 4 months ago

kyrie-z commented 4 months 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/kyrie-z/shim-review/tree/uos-shim-15.8-amd64-20240711 https://github.com/kyrie-z/shim-review/tree/uos-shim-15.8-amd64-20240806


What is the SHA256 hash of your final SHIM binary?


958987f06da4438ab43a873e2c0dd65478299b284ad6e53ca2528524e3a069dd shimx64.efi


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


[UOS shim 15.4 for x86_64 #173 ]


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

steve-mcintyre commented 4 months ago

verification emails sent

kyrie-z commented 4 months ago

@steve-mcintyre Sorry for the late reply, but I want to confirm what went wrong. I did not receive the verification email about "shim review". The contact email is zhouzilong@uniontech.com. Please confirm again whether it has been sent. Thanks!

jclab-joseph commented 3 months ago

Review of reproducibility for uos-shim-15.8-amd64-20240711

review helper : https://github.com/jclab-joseph/other-shim-reviews/tree/master/20240730-uos-shim-15.8-amd64-20240711

shim

certificate

grub

steve-mcintyre commented 3 months ago

Re-sent verification mails to

steve-mcintyre commented 3 months ago

And also sending to zhouzilong@uniontech.com

Not sure where I found the yanbowen mail - maybe an older review. Sorry.

kyrie-z commented 3 months ago

Contact verification for zhouzilong@uniontech.com:

gages schoolboy raving preview diagramming holds results cicatrix linger sulphuring

steve-mcintyre commented 3 months ago

Just waiting on the response from lichenggang@uniontech.com now.

Zeno-sole commented 3 months ago

Just waiting on the response from lichenggang@uniontech.com now.

The old key has expired. Can I use a new key? new key:https://github.com/kyrie-z/shim-review/blob/uos-shim-15.8-amd64-20240711/key/ChenggangLi.pub

steve-mcintyre commented 3 months ago

Just waiting on the response from lichenggang@uniontech.com now.

The old key has expired. Can I use a new key? new key:https://github.com/kyrie-z/shim-review/blob/uos-shim-15.8-amd64-20240711/key/ChenggangLi.pub

The mail I sent was encrypted to this key, which does not appear to have expired:

pub   rsa3072/B4EE92960BB8C880 2021-04-23 [SC]
      B711456DD79BDCA3100EE9B6B4EE92960BB8C880
uid                 [ unknown] lichenggang <lichenggang@uniontech.com>
sub   rsa3072/66A6A001ED9D8D69 2021-04-23 [E]

The new key you're suggesting I use does not match the email address lichenggang@uniontech.com:

pub   rsa4096/A757694FF3D0B626 2024-07-11 [SC]
      61AE69171770E71B39D842F1A757694FF3D0B626
uid                 [ unknown] lichenggang <lichenggang@deepin.org>
sub   rsa4096/0EC1F8845EC8DD6B 2024-07-11 [E]

Please fix this.

Could you also please explain for us: what is the relationship between:

Some consistency in UIDs and keys here is necessary.

kyrie-z commented 3 months ago

Could you also please explain for us: what is the relationship between:

  • UnionTech Software Technology (uniontech.com)
  • UOS (chinauos.com)
  • Deepin (deepin.org)

I apologize for any confusion regarding the names. Please allow me to clarify: Deepin Technology Co., Ltd. ("Deepin Technology") is a wholly-owned subsidiary of UnionTech Software Technology Co., Ltd. ("UnionTech Software"). Deepin Technology owns the product "deepin" (product website: https://www.deepin.org/), while UnionTech Software owns the product "UOS" (product website: https://www.chinauos.com/).

kyrie-z commented 3 months ago

@steve-mcintyre I have updated the secondary contact email address to keep the email address consistent with the UID. Please use the new key for contact verification. Looking forward to hearing from you, thanks! https://github.com/kyrie-z/shim-review/blob/uos-shim-15.8-amd64-20240711/README.md#who-is-the-secondary-contact-for-security-updates-etc

steve-mcintyre commented 3 months ago

Mail on the way. As you've updated your submission in git, please also add a new tag and update the issue here with that new tag.

kyrie-z commented 3 months ago

I have created a new tag and updated the issue. New tag: https://github.com/kyrie-z/shim-review/tree/uos-shim-15.8-amd64-20240806

kyrie-z commented 3 months ago

By the way, the tags uos-shim-15.8-amd64-20240711 and uos-shim-15.8-amd64-20240806 are associated with the same commit (https://github.com/kyrie-z/shim-review/commit/02e5eb2ab6ad48fdfd0be8fa53c7d09dbbb96e07). I believe that jclab-joseph's review https://github.com/rhboot/shim-review/issues/431#issuecomment-2257380652 is very useful, so I'm mentioning this to avoid duplicate review efforts. I hope this helps with your review. Thank you.

Zeno-sole commented 3 months ago

Contact verification for lichenggang@deepin.org:

puritan segregate expatriating Alnitak homily daffodils Avalon bountiful blurted Hecuba

Zeno-sole commented 2 months ago

Contact verification for lichenggang@deepin.org:

puritan segregate expatriating Alnitak homily daffodils Avalon bountiful blurted Hecuba

@steve-mcintyre hello, Can you help review

evilteq commented 1 month ago

Pretty clean and by-the-book.

Not much else to review from our side, 👍

costinchen commented 1 month ago

I'm not an official reviewer, but I want to help speed up reviewing.

grub: 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.debian,4,Debian,grub2,2.06-3~deb10u4,https://tracker.debian.org/pkg/grub2 grub.uos,1,Uos,grub2,2.06.1-1,mail:secureboot@uniontech.com


- NX bit is disabled: `DllCharacteristics      00000000`
- Build with official shim tarball with no patches, pretty clean

All looks good from my perspective!👍
THS-on commented 3 weeks ago

Review for uos-shim-15.8-amd64-20240806

General

Shim

GRUB

Kernel

Notes and comments

costinchen commented 3 weeks ago
  • Embedded cert is CA cert:

    • Subject: C = CN, O = Uniontech, OU = Uniontech OS, CN = Uniontech UEFI Bootloader Publisher 2024
    • Valid begin: Jan 16 03:11:23 2024 GMT, until: Jan 16 00:00:00 2054 GMT (30 years)
    • 4096 bit RSA key
    • Key is kept in a red zone room, compliant with FIPS 140-2 Level 2 security standards

Sorry for my carelessness when review the certificate in https://github.com/rhboot/shim-review/issues/431#issuecomment-2437370594 , I missed the simple CA:FALSE field which means it's not a CA certificate. Here are the complete X509v3 extensions filed in your certificate :

X509v3 extensions:
    X509v3 Key Usage: critical
        Digital Signature
    X509v3 Extended Key Usage: 
        Code Signing
    X509v3 Basic Constraints: critical
        CA:FALSE
    X509v3 Subject Key Identifier: 
        2D:D8:CD:70:0A:34:9E:1B:2B:52:4F:87:D3:B1:24:D1:C7:B9:6B:0D
    X509v3 Authority Key Identifier: 
        D0:5D:4C:E6:4E:1B:9D:C0:C5:85:7B:C0:17:C6:51:0C:7B:5C:CB:17
    X509v3 Certificate Policies: 
        Policy: 1.2.156.115230.9.8.7.1
          CPS: http://www.uosca.cn/policy/
        Policy: 1.2.156.115230.9.8.7.1
          CPS: https://pki.uniontech.com/ca/cps
kyrie-z commented 2 weeks ago

Embedded certificate is not a CA with codesiging and digital signature key usage set

  • Valid till Jan 16 00:00:00 2054 GMT (fine)
  • Any reason that you not include an intermediate CA?

Here is an end-entity certificate, which is used solely for signing verification of the Grub and kernel code. It is not intended to issue certificates, so it does not have CA signing authority.

kyrie-z commented 2 weeks ago
  • What is the deepin_gfxmode module?

The deepin_gfxmode module is used to accurately obtain the resolution at which Grub runs, helping the system better adjust the GRUB display settings.

  • Do you really need the following modules: minix minix2 reiserfs?

Previously, many modules were added to ensure Grub had the latest features and support, but now it appears that some of these modules are outdated. We plan to gradually remove them in the future.

kyrie-z commented 1 week ago
  • Please make it possible to build your container using docker build . --progress plain --no-cache and not via wrapper script next time

Thanks for the reminder. we'll take it on board next.

  • Looking at the GRUB SBAT entry you are basing on Debian 10 which LTS is already EOL. On which Debian version is UOS based?

UOS was based on Debian 10 before Debian 10 reached EOL. After that, it transitioned to being based on the Deepin system. It is currently using GRUB version 2.06, and GRUB will be updated to version 2.12 in the future.

  • Please describe how you enforce lockdown when Secure Boot is enabled

The two functions of secure boot and lockdown are independent of each other. Enabling secure boot does not force lockdown.

THS-on commented 1 week ago

The two functions of secure boot and lockdown are independent of each other. Enabling secure boot does not force lockdown.

Technically these are two separately features, but not enabling lockdown when booting in secure boot allows the user to circumvent the protections assumed by secure boot easily. The consensus is that no kernels should be signed that not force lockdown when booted with secure boot enabled.

kyrie-z commented 1 hour ago

Thanks for your suggestion, @THS-on . We will try to incorporate CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT into the kernel and ensure that no kernel without this feature will be signed.