search

rhboot / shim-review

Reviews of shim
66 stars 130 forks source link

shim 15.8 for Evren #409

Closed es-fabricemarie closed 2 months ago

es-fabricemarie commented 7 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/entsecure/shim-review/tree/entsecure-shim-x86-20240408 https://github.com/entsecure/shim-review/tree/entsecure-shim-x86-20240619 https://github.com/entsecure/shim-review/tree/entsecure-shim-x86-20240808

What is the SHA256 hash of your final SHIM binary?

dbd63ec4d78e922521f0726c9690c630b410096cf3bc6ffb9965ef22ea94f938 shimx64.efi 955cc405f30458e402e3c8f37e67cb4e741f02f22d4aa622e13a49434433becc shimia32.efi

83b2eca6a857b1c0a1c2190c5f48992646211faa8c63b1cebd759fe99bee7780 shimx64.efi 38aae1b3bb9c31bd2c373071a88c19c46790087db680994f1110e025ca512b8c shimia32.efi

7d176c33b6d59f39f2bfcebb5835a31515a26f93ac69292b6b678fecda4ac367  shimx64.efi
4e5c54f6dcb71f65d8f3a5fce63d73e1792612e33df6f334539b3317a0cb007b  shimia32.efi

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

N/A. This is our first shim review submission

steve-mcintyre commented 6 months ago

Verification mails sent.

es-fabricemarie commented 6 months ago

@steve-mcintyre the contact verification words for fabrice@evren.co are:

hatefully harlequins sorceress locoweeds probated juts broomsticks remaking= proffering soaps

es-keantan commented 6 months ago

@steve-mcintyre the contact verification words for kean.tan@evren.co are:

competes exaggerations moody decree Hussite peopling maharaja drawbridges c= lambers exclusiveness

steve-mcintyre commented 5 months ago

Ummm, no:

The build uses `podman` containers. To reproduce the build:

- ensure podman is installed
- as root, run `reproduce_build.sh`
    - this will create a new log file called `03-build-reproduce.log`

Why are you asking me to run things as root here? What's wrong with a simple Dockerfile like lots of other distros use?

es-fabricemarie commented 5 months ago

@steve-mcintyre my bad, you don't need to be root at all, sorry. It reproduces fine with unprivileged user too, I just checked.

I wanted to store the base image on our servers to ensure that the build would still reproduce many months after submission for ease of review and for auditing purposes too.

es-fabricemarie commented 5 months ago

@steve-mcintyre I've updated our shim-review repo with a new tag to have docker build/run to reproduce to make it easier.

https://github.com/entsecure/shim-review/tree/entsecure-shim-x86-20240619

aronowski commented 4 months ago

Thank you for the patience. Had to do some work on my end to analyze the application.

Running ./reproduce_build.sh seems to do the job just fine. Got confused a bit about the errors at the end, i.e.:

Copying all build files from container to user's work directory
/usr/bin/cp: cannot create regular file '/work2/output/builder-packages.txt': Permission denied
/usr/bin/cp: cannot create directory '/work2/output/x86_64': Permission denied
/usr/bin/cp: cannot create directory '/work2/output/ia32': Permission denied
/usr/bin/cp: preserving times for '/work2/output': Permission denied

although the binaries are reproducible.

The application seems alright, but I'll need further help regarding memtest86+ being a part of the bootloader chain - won't be able to provide further help here.

es-fabricemarie commented 4 months ago

@aronowski secure boot support for memtest86+ was added by @vathpela a while back: https://github.com/memtest86plus/memtest86plus/pull/34 perhaps Peter can comment?

vathpela commented 4 months ago

I've asked around to see if anyone has concerns, and Scott Shell at MS raised one issue - it'd be best for it to keep an internal hash of its .text and re-measure after relocating itself. It's a reasonable "security in depth" request IMO, but I haven't had a chance to circle back and implement it.

I'm not 100% sure it's a deal breaker.

es-fabricemarie commented 4 months ago

Thanks @vathpela for the explanation. What do you suggest I do to remedy the situation first and let the signing of our shim proceed?

THS-on commented 3 months ago

I would suggest not signing memtest86+ until there is an official agreement that signing it is fine. If you already signed something with your current CA, either rotate the signing certificate and add it to the vendor DBX, see VENDOR_DBX_FILE of the shim or switch to a new CA.

Also can you rework your container setup, so that everything can be build using a simple docker build .? This makes reviewing for us much easier.

es-fabricemarie commented 3 months ago

@THS-on @aronowski I have modified the shim as requested:

Let me know if I'm missing anything else, and thanks again for your help.

aronowski commented 3 months ago

Managed to take a look and the yesterday's revision seems alright. I especially appreciate the plain docker build . usage being available now.

I'll remove the bug label. Let's wait for another positive review!

steve-mcintyre commented 3 months ago

Review of shim 15.8 for Evren

Good

General

Shim

GRUB

Linux

fwupd

Issues / queries

None!

steve-mcintyre commented 3 months ago

I'm not seeing any other complete reviews yet here.

@THS-on @aronowski - you've already spent time on this submission. Where are you up to please?

dennis-tseng99 commented 3 months ago

Please allow me to join this review. Hope to share your loading.

=== shim 15.8 for Evren #409 ===

objdump -x shimia32.efi | grep -E 'SectionAlignment|DllCharacteristics' SectionAlignment 00001000 DllCharacteristics 00000000

- sbat looks good:

shimx64.efi : sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md shim,4,UEFI shim,shim,1,https://github.com/rhboot/shim shim.evren,1,Evren,shim,15.8,security@evren.co

shimia32.efi sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md shim,4,UEFI shim,shim,1,https://github.com/rhboot/shim shim.evren,1,Evren,shim,15.8,security@evren.co

grub2 (NTFS vulnerabilities patch not included) sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md grub,3,Free Software Foundation,grub,2.06,https//www.gnu.org/software/grub/ grub.rh,2,Red Hat,grub2,2.06-122.ev39,mailto:secalert@redhat.com grub.evren,1,Evren,grub2,2.06-122.ev39,mailto:security@evren.co


- Certificate Validity: 20 years is ok, but NIST deems RSA 2048 suffficient until 2030. hmm...

openssl x509 -in evren_securebootca_cert.der -inform der -noout -text Certificate: Data: Version: 3 (0x2) Serial Number: 3d:4f:33:b5:db:ab:b7:c6:ee:67:20:d3:72:95:9b:80:47:4b:90:59 Signature Algorithm: sha256WithRSAEncryption Issuer: C = SG, O = Entsecure South East Asia Pte Ltd, OU = Evren, CN = Entsecure Secure Boot CA Validity Not Before: Oct 26 09:15:24 2023 GMT Not After : Oct 26 09:15:24 2043 GMT Subject: C = SG, O = Entsecure South East Asia Pte Ltd, OU = Evren, CN = Entsecure Secure Boot CA Subject Public Key Info: Public Key Algorithm: rsaEncryption RSA Public-Key: (2048 bit)



- comment
  May I suggest you change the answer in the following Q/A of README.md next time:
  **URL for a repo that contains the exact code which was built to result in your binary:**
  your answer: https://github.com/entsecure/shim
  suggested answer: https://github.com/entsecure/shim-review

- This issue has been reviewed by many experts include @steve-mcintyre, @THS-on and @aronowski. Let's accept it. @es-fabricemarie, thanks for your patience.
es-fabricemarie commented 2 months ago

Signed_14405145257527623.zip Shim signed by Microsoft and attached here for audit reasons. Thanks everyone!