search

rhboot / shim-review

Reviews of shim
67 stars 127 forks source link

shim 15.8 (64bit and 32bit) for baramundi Management Suite #422

Open MarkusSpier opened 3 months ago

MarkusSpier commented 3 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/baramundisoftware/ShimReview_2024/tree/baramundi_ShimReview_2024_05_24

What is the SHA256 hash of your final SHIM binary?

shimx64.efi: 8caa3da0defbfa7d9fbb33f573a4d0f308712957883de46e21c9f4292cd0ceb9 shimia32.efi: 1fe41f9d836f64d299d790d7b6ba14d7f5c93f73014d08468814f307d86de4a8

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

MarkusSpier commented 3 months ago

Identity confirmation (Markus Spier): kiddy demarcation washer reptile Capra Evan weepiest covetous copulate Monteverdi

MarkusSpier commented 3 months ago

@steve-mcintyre Just for info, my colleagues (the other contacts mentiont in the review) are currently on holiday until 10 June. So it will be a while before they reply.

steve-mcintyre commented 3 months ago

@steve-mcintyre Just for info, my colleagues (the other contacts mentiont in the review) are currently on holiday until 10 June. So it will be a while before they reply.

Understood, thanks for letting us know

baramundi-vmoor commented 3 months ago

Identity confirmation (Viktor Moor): sprout interspersing promontories locutions Sachs determination spoor oath tuckering characteristically

baramundi-cbonfert commented 3 months ago

Identity confirmation (Christian Bonfert): inaccessibility uninstall bazaars swop wanna archivists fanny mindfully nightcap lychees

THS-on commented 1 month ago

I have some questions before doing a full review.

MarkusSpier commented 1 month ago

I have some questions before doing a full review.

* `Old shims hashes will be submitted for revocation as soon as the new bootloaders were shipped to our customers.` Have you had a shim signed in the past?

Yes.

* As you are using the peimage patches, there is a new version as the old one had an issue.

I assume you are asking this question because we use the grub.peimage,1 flag in our sbat.csv. In fact, we do not deliver this module. Unfortunately, we overlooked to remove the entry from the sbat.csv. 

* As you are not booting Linux, enabling the NX flag might be a real option here, assuming the latest GRUB2 version now fully works on NX enabled hardware.

Thanks for the hint. Is setting the NX-flag relevant for passing this review?

* Can you publish an up-to-date copy of your GRUB2 sources and explain in detail what your module does?

You can find the sources here: https://github.com/baramundisoftware/grub2_2024

Description what our module does:

In the first step, our module "bblefi" determines the UUID of the system and the MAC address of the network adapter. Then it reads (loads) a configuration file with simple control options from the TFTP server. Afterwards it reads (loads) a menu file with boot options from the TFTP server specified in the configuration file. This menu file is located in a path/folder that contains the system UUID and the MAC of the network adapter. In the next step it offers the boot options in a TUI (Text based User Interface) and waits for selection by the user if F8 has been pressed. Otherwise the configured default option is selected. Finally, the selected option is executed (load file via TFTP, boot from HDD, open GRUB console). The Shim Verifier ensures that files loaded for execution are signed by Microsoft or baramundi and can therefore be trusted.

es-fabricemarie commented 1 month ago

Questions:

MarkusSpier commented 1 month ago

Hi @es-fabricemarie thanks for working on our submission.

Questions:

* Was the CA cert rotated because of a security flaw?

  * Do you have any link to the previous review ticket?
    (since you mentioned "Old shims hashes will be submitted for revocation as soon as the new bootloaders were shipped to our customers."

No, the old EV Code Signing cert had simply expired. In the meantime, the legal form has also changed (AG => GmbH), so that a new EV Code Signing certificate made sense from our point of view.

Unfortunately, a link to the old review-process (from 2015) does not exist.

* NX bit not set even though it only boots Windows with Grub2.
  Not sure if grub2 supports NX properly yet or not.
  If it does, NX bit should be set.
  If grub2 doesn't support it yet then it's fine for now.

I would prefer not to set the NX bit, as we are considering (mid/long-term) booting Linux via grub2 in the future.

Hopefully I was able to answer your questions. How can I help to push the review process forward?

es-fabricemarie commented 1 month ago

Makes sense. Thanks.

I suggest you upload your old certificate and your old signed-shim to your current shim review repo since you can't produce the old shim review. This way, everyone will be able to see that the cert has expired.

MarkusSpier commented 1 month ago

Makes sense. Thanks.

I suggest you upload your old certificate and your old signed-shim to your current shim review repo since you can't produce the old shim review. This way, everyone will be able to see that the cert has expired.

Thanks @es-fabricemarie for this hint.

The requested files can be found here in our current shim review repo: https://github.com/baramundisoftware/ShimReview_2024/tree/main/OldReviewContent

es-fabricemarie commented 1 month ago

I can confirm that your cert expired in 2018 when examining the cert extracted from your old shim:

objcopy -j .vendor_cert -O binary shim_old.efi expired_vendor_cert.der
openssl x509 -inform der -in expired_vendor_cert.der -noout -text -fingerprint

[...]
        Issuer: C=US, O=DigiCert Inc, OU=www.digicert.com, CN=DigiCert EV Code Signing CA (SHA2)
        Validity
            Not Before: Mar 11 00:00:00 2015 GMT
            Not After : Mar 14 12:00:00 2018 GMT
        Subject: businessCategory=Private Organization, jurisdictionC=DE, jurisdictionST=Augsburg, serialNumber=HRB 2064, street=Beim Glaspalast 1, postalCode=86153, C=DE, ST=Bayern, L=Augsburg, O=baramundi software AG, CN=baramundi software AG
SHA1 Fingerprint=11:CB:BF:10:1A:8E:D5:8F:FF:EF:36:CD:9D:DF:9B:7F:FF:4E:7B:16
[...]

Just to be thorough: it means that since March 2018 you have no signed shim?

MarkusSpier commented 1 month ago

I can confirm that your cert expired in 2018 when examining the cert extracted from your old shim:

objcopy -j .vendor_cert -O binary shim_old.efi expired_vendor_cert.der
openssl x509 -inform der -in expired_vendor_cert.der -noout -text -fingerprint

[...]
        Issuer: C=US, O=DigiCert Inc, OU=www.digicert.com, CN=DigiCert EV Code Signing CA (SHA2)
        Validity
            Not Before: Mar 11 00:00:00 2015 GMT
            Not After : Mar 14 12:00:00 2018 GMT
        Subject: businessCategory=Private Organization, jurisdictionC=DE, jurisdictionST=Augsburg, serialNumber=HRB 2064, street=Beim Glaspalast 1, postalCode=86153, C=DE, ST=Bayern, L=Augsburg, O=baramundi software AG, CN=baramundi software AG
SHA1 Fingerprint=11:CB:BF:10:1A:8E:D5:8F:FF:EF:36:CD:9D:DF:9B:7F:FF:4E:7B:16
[...]

Just to be thorough: it means that since March 2018 you have no signed shim?

The old signed shim recently provided in the current shim review was signed by Microsoft with the certificate "Microsoft Corporation UEFI CA 2011", which has not yet been revoked. Therefore, the signing of our old shim is still valid.

MarkusSpier commented 1 month ago

@THS-on Are there any outstanding issues that we can help with to support and drive forward the review process?

THS-on commented 1 month ago

@MarkusSpier as you implement a lot new functionality as a single GRUB2 module, we are currently in discussion how to treat this from a review perspective, as this is closer to a custom bootlaoder than vanilla GRUB2.

@steve-mcintyre is there something @MarkusSpier can already do?

MarkusSpier commented 1 month ago

@MarkusSpier as you implement a lot new functionality as a single GRUB2 module, we are currently in discussion how to treat this from a review perspective, as this is closer to a custom bootlaoder than vanilla GRUB2.

@THS-on Thanks for the quick reply.

The only changes are the updates of Grub2 and Shim to the most recent versions to address security issues and the transfer of the UUID.

As the previous bootchain (currently signed shim + grub2 with bblefi module) is almost identical to the bootchain described in this review, the functionality is not new in our opinion.

steve-mcintyre commented 1 week ago

Review of shim 15.8 (64bit and 32bit) for baramundi Management Suite

Good

General

Shim

GRUB

Linux

fwupd

Issues / queries

We have no view of what the bbl code looks like, which makes it impossible for us to assign trust there. It's up to Microsoft to decide if they're happy to sign this shim based on that.

Technical review of your shim says that it is fine, just two outstanding queries for me:

MarkusSpier commented 1 week ago

Thanks @steve-mcintyre for your review so far.

The extracted SBAT from the GRUB2 binary: 

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,@UPSTREAM_VERSION@,https://www.gnu.org/software/grub/
grub.ubuntu,1,Ubuntu,grub2,@DEB_VERSION@,https://www.ubuntu.com/
grub.peimage,1,Canonical,grub2,@DEB_VERSION@,https://salsa.debian.org/grub-team/grub/-/blob/master/debian/patches/secure-boot/efi-use-peimage-shim.patch
grub.baramundi,1,Baramundi,grub2,2.12-1ubuntu1-bblefi1,https://github.com/baramundisoftware/grub2

It´s identical to the templated data before build that we included in the readme. Caused by our build process, the placeholders are not replaced.

steve-mcintyre commented 1 week ago

Thanks @steve-mcintyre for your review so far.

The extracted SBAT from the GRUB2 binary:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,@UPSTREAM_VERSION@,https://www.gnu.org/software/grub/
grub.ubuntu,1,Ubuntu,grub2,@DEB_VERSION@,https://www.ubuntu.com/
grub.peimage,1,Canonical,grub2,@DEB_VERSION@,https://salsa.debian.org/grub-team/grub/-/blob/master/debian/patches/secure-boot/efi-use-peimage-shim.patch
grub.baramundi,1,Baramundi,grub2,2.12-1ubuntu1-bblefi1,https://github.com/baramundisoftware/grub2

It´s identical to the templated data before build that we included in the readme. Caused by our build process, the placeholders are not replaced.

That definitely needs fixing then. You have the information to be able to list proper versions here.

MarkusSpier commented 1 week ago

Thanks @steve-mcintyre for your review so far. The extracted SBAT from the GRUB2 binary:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,@UPSTREAM_VERSION@,https://www.gnu.org/software/grub/
grub.ubuntu,1,Ubuntu,grub2,@DEB_VERSION@,https://www.ubuntu.com/
grub.peimage,1,Canonical,grub2,@DEB_VERSION@,https://salsa.debian.org/grub-team/grub/-/blob/master/debian/patches/secure-boot/efi-use-peimage-shim.patch
grub.baramundi,1,Baramundi,grub2,2.12-1ubuntu1-bblefi1,https://github.com/baramundisoftware/grub2

It´s identical to the templated data before build that we included in the readme. Caused by our build process, the placeholders are not replaced.

That definitely needs fixing then. You have the information to be able to list proper versions here.

We have adapted the build. The placeholders are now replaced. The new extracted SBAT from the GRUB2 binary:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,2.12,https://www.gnu.org/software/grub/
grub.ubuntu,1,Ubuntu,grub2,2.12-1ubuntu1,https://www.ubuntu.com/
grub.peimage,1,Canonical,grub2,2.12-1ubuntu1,https://salsa.debian.org/grub-team/grub/-/blob/master/debian/patches/secure-boot/efi-use-peimage-shim.patch
grub.baramundi,1,Baramundi,grub2,2.12-1ubuntu1-bblefi1,https://github.com/baramundisoftware/grub2
steve-mcintyre commented 1 week ago

Approved - next step is with Microsoft