search

rhboot / shim-review

Reviews of shim
67 stars 131 forks source link

iPXE shim (new submission) #319

Open mcb30 opened 1 year ago

mcb30 commented 1 year ago

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

UPDATED: see below at https://github.com/rhboot/shim-review/issues/319#issuecomment-2007813215

What is the link to your tag in a repo cloned from rhboot/shim-review?

https://github.com/ipxe/shim-review/tree/ipxe-shim-x64-aa64-20230217

What is the SHA256 hash of your final SHIM binary?

shimx64.efi 26ec898a8e801fceafec29577386286687efe55d68fa96dc068adb4f5f02060e shimaa64.efi be492d53adff1720e130347a68fb1df3231b7bce824ef3d41cdb311b26d2e024

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

N/A

mcb30 commented 7 months ago

Reviewing with high priority as promised. Although had stuff going on in my life and was absent for about a week.

Thank you!

There's something weird - the README says that NX bit is set, but it actually isn't:

$ objdump -x shimx64.efi | grep DllCharacteristics
DllCharacteristics      00000000

Good catch! I trusted the shim build process to produce an NX-compatible binary since I was aware that this was added shortly after the 15.7 release (specifically, in commit https://github.com/rhboot/shim/commit/7c7642530fab73facaf3eac233cfbce29e10b0ef) and the shim build instructions still state that the NX compatibility flag must be explicitly disabled.

However, it looks as though @vathpela changed this default behaviour in commit https://github.com/rhboot/shim/commit/be8ff7c2680fed067cdd76df0afc43138c24cc0d without updating any documentation. So the 15.8 shim build is still NX incompatible by default, contrary to shim's own documentation and the shim-review stated requirements.

I'd say that we should leave it not set and modify the README, as some bootloader chains may not be NX-compatible. For example when iPXE loads a shim without NX bit set.

iPXE is already built with the NX flag set (as of commit https://github.com/ipxe/ipxe/commit/a9e89787d). It uses the firmware's LoadImage() to load any subsequent UEFI binaries, and so an NX-compatible iPXE can happily load an NX-incompatible subsequent shim (as long as the firmware's platform policy permits NX-incompatible binaries to be loaded at all).

It looks as though we should still be able to use POST_PROCESS_PE_FLAGS to override this default choice and force the NX-compatible bit to be set. I'll respin the submission with this added.

Considering the updated patch, I have no competence in reviewing C code, but I do trust that it has been tested out thoroughly and that will be confirmed in another Microsoft Tech Community Hub entry.

Thanks!

Continued in this issue, so as not to lose track of the fact that this submission has already been waiting over one year.

👍

Although I wonder if it's possible to update the original post as well, so when new people come here, they will immediately be informed about the latest entry.

Will do.

mcb30 commented 7 months ago

Updated submission (previously https://github.com/rhboot/shim-review/issues/319#issuecomment-1967954690, changed to enable NX compatibility as discussed in https://github.com/rhboot/shim-review/issues/319#issuecomment-1978545387):

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/ipxe/shim-review/tree//ipxe-shim-x64-aa64-20240305

What is the SHA256 hash of your final SHIM binary?

shimx64.efi 621f74e64a49b0b35d6e9eac99cd29146551b4069b94dd66ea5d4bb13ab012e4 shimaa64.efi fd793ecec822cbe19372fd75503fa2b706226667512dbec5344e57a9cb38ad6a

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

Continued in this issue, so as not to lose track of the fact that this submission has already been waiting over one year.

mcb30 commented 7 months ago

@aronowski I've updated the submission (see above in https://github.com/rhboot/shim-review/issues/319#issuecomment-1978633328) to set the NX flag as discussed above.

I've also updated the original post to link to this latest version of the submission, as suggested.

Could you please review and see if this answers all of your outstanding questions?

Thanks!

aronowski commented 7 months ago

Thanks for the updates!

iPXE is already built with the NX flag set (as of commit ipxe/ipxe@a9e89787d). It uses the firmware's LoadImage() to load any subsequent UEFI binaries, and so an NX-compatible iPXE can happily load an NX-incompatible subsequent shim (as long as the firmware's platform policy permits NX-incompatible binaries to be loaded at all).

Alright!

But I wrote that earlier comment in regard to the Microsoft NX Exception we have as nevertheless some part of the bootloader chain may not be NX-compatible.

Still, even if this understanding is correct and the NX bit should not be set, it's a matter of changing the -n flag to -N, recompiling the binaries, updating an entry in the README and pushing the changes here in minutes - the build does reproduce for me after all.

Please, ping another reviewer and ask for further review, in case there's something else that requires attention - I'm happy with the current state and if it's just about changing the NX bit in shim, it's trivial.

Since I'm stubborn on the understanding, that it should not be set, I'll add the "extra review wanted label" after it's unset or another official reviewer convinces me that it should be set.

mcb30 commented 7 months ago

Please, ping another reviewer and ask for further review, in case there's something else that requires attention - I'm happy with the current state and if it's just about changing the NX bit in shim, it's trivial.

Since I'm stubborn on the understanding, that it should not be set, I'll add the "extra review wanted label" after it's unset or another official reviewer convinces me that it should be set.

Thank you very much for the review!

I am happy either way with the NX flag: my understanding is that we are fine to leave it set, but I don't have any particularly strong opinion on it. Rather than waiting for another reviewer to argue that it should be set, I'll just clear it and push a new submission.

mcb30 commented 7 months ago

Updated submission (previously https://github.com/rhboot/shim-review/issues/319#issuecomment-1978633328, reverted to disable NX compatibility and README.md updated as requested in https://github.com/rhboot/shim-review/issues/319#issuecomment-1981230322):

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/ipxe/shim-review/tree//ipxe-shim-x64-aa64-20240306

What is the SHA256 hash of your final SHIM binary?

shimx64.efi 909737489b19e1f95617ef24013e648656a2ff477fc8f9e163c253878f199234 shimaa64.efi 0c4eb9dedc34a0d62af67d21c2dc0f17cffeae5c3aff4ffe579b351138c88538

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

Continued in this issue, so as not to lose track of the fact that this submission has already been waiting over one year.

mcb30 commented 7 months ago

I am happy either way with the NX flag: my understanding is that we are fine to leave it set, but I don't have any particularly strong opinion on it. Rather than waiting for another reviewer to argue that it should be set, I'll just clear it and push a new submission.

@aronowski New submission is pushed - please let me know if there are any further changes you'd like made. Thanks again!

aronowski commented 7 months ago

Thanks!

I'm happy with the application in its current form and hope the further reviewing goes well. I've done my part.

Ping another reviewer in case there's no activity for some time - we know the drill.

mcb30 commented 7 months ago

I'm happy with the application in its current form and hope the further reviewing goes well. I've done my part.

Fantastic; thank you very much for all of your help so far. It's great to see this finally making some forward movement.

Ping another reviewer in case there's no activity for some time - we know the drill.

@frozencemetery @BurningC4 @kfortner Would any of you please be able to carry out the extra review as requested by @aronowski?

steve-mcintyre commented 7 months ago

Review of Shim 15.8 for ipxe: ipxe-shim-x64-aa64-20240306

OK

Niggles:

~- There's plenty of discussion in the issue about the NX bit. The final statement here is that the NX bit is not set, but your README.md still says it is. Not a blocker, just raising this as a minor nit!~

Looks good, accepting!

mcb30 commented 7 months ago

Looks good, accepting!

@steve-mcintyre Thank you very much for reviewing and accepting!

Niggles:

  • There's plenty of discussion in the issue about the NX bit. The final statement here is that the NX bit is not set, but your README.md still says it is. Not a blocker, just raising this as a minor nit!

Oh dear. :face_with_spiral_eyes: I'm sorry if I missed updating that. Where did you find it documented as still being set? In the tagged version of README.md at https://github.com/ipxe/shim-review/tree/ipxe-shim-x64-aa64-20240306?tab=readme-ov-file#do-you-have-the-nx-bit-set-in-your-shim-if-so-is-your-entire-boot-stack-nx-compatible-and-what-testing-have-you-done-to-ensure-such-compatibility I see:

No, the NX bit is not set.

While it is believed that the shim+iPXE boot stack is NX-compatible (and the iPXE binary is already built with the NX bit set), we do not intend to set the NX bit in the shim until it is set by default in the shim release.

steve-mcintyre commented 7 months ago

Apologies - that's my mistake. I didn't have the latest update to the README.md from 24c6d6c0eaa2faf49d97c1e28e15bbb2abc7ae31.

All good then! :+1:

mcb30 commented 7 months ago

For anyone wanting to track progress: this has now been submitted to Microsoft for Secure Boot signing (with submission number 13887439201546611).

raddirad commented 7 months ago

Purely out of curiosity. Does this acceptance mean that other project could use iPXE instead of Grub for things that Grub currently can't do? As mentioned earlier for example wimboot or boot via HTTPS

mcb30 commented 7 months ago

Purely out of curiosity. Does this acceptance mean that other project could use iPXE instead of Grub for things that Grub currently can't do? As mentioned earlier for example wimboot or boot via HTTPS

Once the shim is signed by Microsoft, then yes. The intention is to release the signed shim, and then to make regular releases of some standard builds of iPXE signed by the EV certificate in the shim.

raddirad commented 7 months ago

Once the shim is signed by Microsoft, then yes. The intention is to release the signed shim, and then to make regular releases of some standard builds of iPXE signed by the EV certificate in the shim.

The intention of me asking is for example create an own build of iPXE with an embedded CA, sign the kernel with this cert and boot it via HTTPS

mcb30 commented 7 months ago

The intention of me asking is for example create an own build of iPXE with an embedded CA, sign the kernel with this cert and boot it via HTTPS

Two issues with that:

The expected usage pattern is that a Linux boot will involve the following steps:

  1. UEFI firmware downloads and executes the Microsoft-signed iPXE shim (i.e. the subject of this GitHub issue) as e.g. ipxe-shimx64.efi
  2. The Microsoft-signed iPXE shim automatically downloads and execute the iPXE-signed published iPXE binary (e.g. ipxe.efi).
  3. The user's iPXE script directs iPXE to download a distro kernel (vmlinuz), shim (shimx64.efi), and initramfs (initrd.img)
  4. iPXE executes the distro kernel via the distro shim
steve-mcintyre commented 7 months ago

@mcb30 Argh, apologies - just noticed one thing I missed on my review (and clearly so did others :-( )...

You're embedding a PEM-encoded certificate in your build, which can't work. It needs to be DER-encoded.

Sorry, should have noticed this much earlier.

steve-mcintyre commented 7 months ago

@mcb30 I'm surprised you didn't see this in testing, tbh...

mcb30 commented 7 months ago

@mcb30 Argh, apologies - just noticed one thing I missed on my review (and clearly so did others :-( )...

You're embedding a PEM-encoded certificate in your build, which can't work. It needs to be DER-encoded.

Sorry, should have noticed this much earlier.

Great catch, thank you for spotting that! This slipped through testing because the test environment, of course, was using test certificates rather than the real EV certificate, and the test certificate was correctly encoded as DER.

I'll update the submission now: give me 15 minutes for everything to propagate through...

mcb30 commented 7 months ago

Updated submission (previously https://github.com/rhboot/shim-review/issues/319#issuecomment-1981384784, fixed to use DER-encoded certificate and README.md updated as noted in https://github.com/rhboot/shim-review/issues/319#issuecomment-2007658545):

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/ipxe/shim-review/tree//ipxe-shim-x64-aa64-20240319

What is the SHA256 hash of your final SHIM binary?

shimx64.efi 636d581943f1872d4eb17af39b3b7e07ed8dfc5ec5e85779550fa2c01fa3aeae shimaa64.efi 986978b520a3b159f74da9e16939aac6a986b8467f6f41929e4d94ef87863ad0

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

Continued in this issue, so as not to lose track of the fact that this submission has already been waiting over one year.

mcb30 commented 7 months ago

@steve-mcintyre Everything updated. Sorry it took so long: it's a very manual process dealing with all of this automation. :upside_down_face:

Could you please recheck and re-add the "accepted" label? :pray:

steve-mcintyre commented 7 months ago

Cool, all looks good with that change. Accepted.

mcb30 commented 7 months ago

Cool, all looks good with that change. Accepted.

Thank you. Resubmitted to Microsoft as UEFI signing submission 14306281433235827.

stappersg commented 5 months ago

What I understand from https://learn.microsoft.com/en-us/windows-hardware/drivers/dashboard/file-signing-manage#view-uefi-or-lsa-submission-details are some credentials needed to see the status.

Some one with such credentials, please update this issue with that status.

steve-mcintyre commented 4 months ago

Hey folks, did you get a signed shim here yet please?

mcb30 commented 4 months ago

Discussions with Microsoft are still ongoing. Next meeting is currently scheduled for late June.

steve-mcintyre commented 3 months ago

Hey! Did you make any progress there?

mcb30 commented 3 months ago

Yes, we had one meeting in late June and another today. The reception has been generally positive, we have clarified a lot of details about the usage policies for the iPXE shim, and we're hoping to get to a final green light within the next month or two.

stappersg commented 2 weeks ago

On Tue, Jul 16, 2024 at 02:15:04PM -0700, Michael Brown wrote:

... and we're hoping to get to a final green light within the next month or two.

Off by one, by now

mcb30 commented 2 weeks ago

Still ongoing, just very slowly. The next call with Microsoft has been deferred multiple times due to availability constraints, and we're currently looking at October 31st.