search

rhboot / shim-review

Reviews of shim
67 stars 127 forks source link

Shim 15.7 for Proxmox Bookworm-based releases #330

Closed Fabian-Gruenbichler closed 8 months ago

Fabian-Gruenbichler commented 1 year 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/Fabian-Gruenbichler/shim-review/tree/proxmox-shim-15.7-amd64-20231006

(updated based on first feedback by @aronowski) (2023-10-06: updated again to include Debian Grub SBAT entry)

What is the SHA256 hash of your final SHIM binary?

d93f0245909a4655ceb8961778f382897b2cc50b1d1e996d1ac450cf7fbcfeb7

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

N/A

aronowski commented 1 year ago

While I'm not an official reviewer, I can see a few curiosities:

*******************************************************************************
### Do you add a vendor-specific SBAT entry to the SBAT section in each binary that supports SBAT metadata ( grub2, fwupd, fwupdate, shim + all child shim binaries )?
### Please provide exact SBAT entries for all SBAT binaries you are booting or planning to boot directly through shim.
### Where your code is only slightly modified from an upstream vendor's, please also preserve their SBAT entries to simplify revocation.
*******************************************************************************

[...]

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.proxmox,3,Proxmox,grub2,2.06-8.1+pmx1,https://git.proxmox.com/?p=grub2.git

Why is your GRUB2's product-specific generation number set to 3?

The https://git.proxmox.com/?p=grub2.git URL leads to a 404 - No such project page. Therefore I cannot verify further GRUB2-related things.

Does your kernel have the latest NX requirements mentioned here enabled:

Also NX support needs to be added to bootloader and kernel.

In other words: do you have a patch that toggles the NX compatibility flag for the vmlinuz binary somewhere in the build process?

Since I can't see any inside the pve-kernel git repository, I thought that maybe you have it inside the kernel's sources directly. However, I cannot clone them:

$ git clone git://git.proxmox.com/git/mirror_ubuntu-kernels.git
Cloning into 'mirror_ubuntu-kernels'...
remote: Enumerating objects: 9432306, done.
remote: Total 9432306 (delta 0), reused 0 (delta 0), pack-reused 9432306
Receiving objects: 100% (9432306/9432306), 2.02 GiB | 1.23 MiB/s, done.
Resolving deltas: 100% (8032695/8032695), done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.

There's also an error when trying to browse the tree through your git web interface: 404 - Reading tree failed.

Fabian-Gruenbichler commented 1 year ago

thanks for the fast response!

we are still in the process of preparing our bookworm based release, so some things are still in flux.

Why is your GRUB2's product-specific generation number set to 3?

after re-reading https://github.com/rhboot/shim/blob/main/SBAT.md , it is clear to me it should be 1 - thanks for catching this!

what's the procedure here (other than updating it in our Grub 2 packaging - which should be unproblematic given that nothing has been signed so far using the production keys) - point this issue at an updated tree in our fork of shim-review? start a new one? just note the fact that this has been fixed here in a comment?

The https://git.proxmox.com/?p=grub2.git URL leads to a 404 - No such project page. Therefore I cannot verify further GRUB2-related things.

I'll see about getting it published today.

Does your kernel have the latest NX requirements mentioned https://github.com/rhboot/shim-review/issues/307 enabled:

Also NX support needs to be added to bootloader and kernel.

In other words: do you have a patch that toggles the NX compatibility flag for the vmlinuz binary somewhere in the build process?

as per https://github.com/rhboot/shim-review/issues/307 I was under the impression that submitting an NX-enabled shim while working on the rest of the stack is okay (it's linked at the top of the Issues queue ;)). if that's no longer the case it might be good to update it. if it's a requirement for getting a (new) shim signed, I'll prioritize pulling in and testing x86_64: Improvements at compressed kernel stage

our current kernels for Bullseye are not signed, I'll see about publishing our Bookworm branch (which has the signing bits enabled and packaging updated to support post-build signing of the kernel image itself) later today as well and post instructions, since the git submodules part can be a bit annoying.

There's also an error when trying to browse the tree through your git web interface: 404 - Reading tree failed.

this is just a mirror of the Ubuntu kernel tags we use (as base) in our build, it usually only gets tags pushed to it and gitweb doesn't handle that very nicely. the actual repository is the pve-kernel one at https://git.proxmox.com/?p=pve-kernel.git;a=summary , which references the mirror (and other things) as git submodule. but please note that the public version of that git repo only contains our Bullseye state, not the one for the upcoming Bookworm based release, as indicated above I'll work on fixing that.

aronowski commented 1 year ago

The procedure I personally follow is to update my review e.g. in the main branch, fix the README entry and GRUB2 related assets (so the final binary would have proper SBAT entry), then tag the updated version and edit the issue so it points to the updated tag.

You can use this for inspiration, but change things according to your situation - I attach assets directly in the review rather than pointing to a remote repository.

I'm waiting for the GRUB2 repository related updates. Please, ping me once they are here.

Fabian-Gruenbichler commented 1 year ago

fwupd-efi packaging tree for our upcoming bookworm releases, based on Debian bookworm packaging:

grub2 packaging tree, same (vendor SBAT already corrected to 1):

kernel packaging tree, packaging is custom:

the kernel packages consist of packaging files (custom, directly in the repository), kernel sources (based on Ubuntu Lunar's, which are in turn based on upstream 6.2.x, included via git submodule in submodules/ubuntu-kernel), zfs module sources (based on our OpenZFS packaging, which is based on Debian's, included via (nested!) git submodule(s) in submodules/zfsonlinux) and kernel patches (in patches/kernel).

to get all of the kernel build files a recursive clone can be used git clone --recursive git://git.proxmox.com/git/pve-kernel.git -b wip-secureboot. as the branch name implies, this is the current working copy for our upcoming release, it isn't yet released or finalized.

Fabian-Gruenbichler commented 1 year ago

@aronowski here you go!

updated review request to point at updated shim-review tag (all changes as discussed here in the comments are also noted in the additional information part of the review readme)

aronowski commented 1 year ago

Thank you for the update!

The GRUB2-related things I'd like to discuss:

I can now see that these GRUB2 sources come directly from Debian and you don't add anything custom on your own:

*******************************************************************************
### If shim is loading GRUB2 bootloader what exact implementation of Secureboot in GRUB2 do you have? (Either Upstream GRUB2 shim_lock verifier or Downstream RHEL/Fedora/Debian/Canonical-like implementation)
*******************************************************************************
We re-use Debian's implementation (rebuilding Grub with SBAT adapted to
differentiate the two variants).

However, it might be a good idea to leave Debian's SBAT entry as-is and append Proxmox's to the end. It's going to simplify management too as you'll always be having grub.proxmox,1,[...] in the end as you're not going to include anything custom on your own.

This would also comply with the Red Hat Bootloader Team's guidelines:

If a binary is derived from a build from another distro, it's helpful to also include the details of that build and distro in the SBAT. This will make it easier to revoke things if we find that a whole family of builds are all broken due to a shared vendor patch.

Recently there's also been this incident in Debian. Just saying.

I checked the sources for GRUB2's NX support and I cannot see its presence. It appears to me that Debian doesn't have it implemented. Not that it's required right now as we're having the conversation, but it will be sooner or later for the whole chain to be compliant with Microsoft's requirements.

There are already patches out there for Fedora/RHEL that implement it: look here for the files that contain nx in their names. Maybe they can be sort of an inspiration.

When it comes to the kernel, I can see there's this mainline implementation:

#ifdef CONFIG_EFI_DXE_MEM_ATTRIBUTES
  .word IMAGE_DLL_CHARACTERISTICS_NX_COMPAT # DllCharacteristics
#else
  .word 0       # DllCharacteristics
#endif

And I can see this line in your debian.master/config/annotations file:

CONFIG_EFI_DXE_MEM_ATTRIBUTES                   policy<{'amd64': 'y'}>

So judging by this hint, I assume the NX support is already here. But having no experience with Debian's configuration, I can't tell for sure.

What I would do, however, would be to inspect the vmlinuz binary to check the effective outcome (historically there have been issues that there was no NX support despite people attaching appropriate patches).

In other reviews I recommend tools for static analysis of PE binaries such as NTCore's CFF Explorer or zed-0xff's pedump to act as judges. You can use them yourself or link the artifact for me to inspect on my own (it could well be a .deb package).

Off-topic: What's going on with the git server? I can browse files through the web interface, but can't clone the GRUB2 and fwupd repositories. I keep on getting the message: warning: remote HEAD refers to nonexistent ref, unable to checkout.

Cloning these repositories would come in handy - it would be faster to browse them and I could have the whole history with me offline - useful when reviewing during a travel with no stable internet connection.

Fabian-Gruenbichler commented 1 year ago

However, it might be a good idea to leave Debian's SBAT entry as-is and append Proxmox's to the end. It's going to simplify management too as you'll always be having grub.proxmox,1,[...] in the end as you're not going to include anything custom on your own.

that seems sensible - at least for the time being, since the amount of delta might change at some point, we are not as conservative as Debian w.r.t. backporting features or fixes, although for Grub it will definitely fall more into the latter category (e.g., we likely would have backported https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=987008 if we were already shipping our own, custom Grub packages).

the same rationale would apply to fwupd-efi as well (or even more so, since the need for deviation is even smaller for that compared to Grub).

Recently there's also been https://github.com/rhboot/shim/pull/550. Just saying.

yes, we are aware of that (this issue was actually what triggered our mishap of grub.proxmox SBAT generation being 3 in the original submission, with a misguided rationale of "we never shipped a broken generation 3 signed Grub, as opposed to Debian" ;))

I checked the sources for GRUB2's NX support and I cannot see its presence. It appears to me that Debian doesn't have it implemented. Not that it's required right now as we're having the conversation, but it will be sooner or later for the whole chain to be compliant with Microsoft's requirements.

There are already patches out there for Fedora/RHEL that implement it: look here for the files that contain nx in their names. Maybe they can be sort of an inspiration.

we will definitely stick to Debian's lead here, and are closely following developments there (I had some fruitful exchanges with the Debian EFI team regarding their implementation, and there is considerable overlap between that team and Debian Grub maintainership as well, and I am maintaining a few unrelated packages in Debian itself as well).

Off-topic: What's going on with the git server? I can browse files through the web interface, but can't clone the GRUB2 and fwupd repositories. I keep on getting the message: warning: remote HEAD refers to nonexistent ref, unable to checkout.

the default HEAD is likely pointing to a non-existent master. git.proxmox.com is our public, read-only mirror, I'll see about getting the HEAD pointing correctly there as well.

Cloning these repositories would come in handy - it would be faster to browse them and I could have the whole history with me offline - useful when reviewing during a travel with no stable internet connection.

you can clone them, just append -b proxmox/bookworm to your clone command (or -b wip-secureboot for the pve-kernel repo, as indicated in my previous comment) or switch to that branch after the failed initial clone (the clone still happened, just checking out afterwards fails). sorry for that, most of our own git repositories do in fact use the default HEAD, but for projects which are mainly backports or slightly modified backports I prefer using an explicit branch name to avoid confusing names (since master could mean upstream master, Debian master if that exists or our trunk branch).

aronowski commented 1 year ago

So there will indeed be some changes rather than using the stock Debian releases. That's OK.

As for the SBAT entry, I'll let the official committee decide on this specific scenario. Doesn't bother me personally but I may be missing something.

Regarding GRUB2 NX support - while I'm not a developer, I'll see what can be done to speed up the process.

Regarding this one off-topic thing I mentioned and others that may be to come, I don't want to cause unnecessary noise in the review and may instead send an email soon.

To sum it up, the review looks good to me now. Good job!

Fabian-Gruenbichler commented 1 year ago

FWIW, the HEADs of our grub2, fwupd-efi, efi-boot-shim repos should now be properly pointing to their respective branches :)

THS-on commented 11 months ago

Review of proxmox-shim-15.7-amd64-20230405

Hashes

#25 [22/24] RUN sha256sum /efi-boot-shim/shim*.efi /shim-review/$(basename /shim/shim*.efi)
#25 0.499 d93f0245909a4655ceb8961778f382897b2cc50b1d1e996d1ac450cf7fbcfeb7  /efi-boot-shim/shimx64.efi
#25 0.521 d93f0245909a4655ceb8961778f382897b2cc50b1d1e996d1ac450cf7fbcfeb7  /shim-review/shimx64.efi
#25 DONE 0.5s

SBAT

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.proxmox,1,Proxmox,shim,15.7,https://git.proxmox.com/?p=efi-boot-shim.git

Notes and Questions

Besides this question, LGTM! I'll send out some emails for contact verification.

Edit: small note that the PGP key for the Proxmox Security Team cannot be found on https://keyserver.ubuntu.com/, but can be found here https://enterprise.proxmox.com/debian/security-report.gpg.pub

Fabian-Gruenbichler commented 11 months ago

here's the verification part for my mail address:

shirt difference skates dishcloths éclair flower paradoxically
bandwagons fencer sprinter
Fabian-Gruenbichler commented 11 months ago

Because it is based on Debian, can you keep their SBAT entry? This makes revocation in their downstream patches easier

that seems sensible! is it okay to just include that in our first signed Grub package version, since it would not affect the shim hash/acceptance directly?

Edit: small note that the PGP key for the Proxmox Security Team cannot be found on https://keyserver.ubuntu.com/, but can be found here https://enterprise.proxmox.com/debian/security-report.gpg.pub

and also here: https://pve.proxmox.com/wiki/Security_Reporting :) if desired, we can also upload it to a few keyservers of course.

THS-on commented 11 months ago

that seems sensible! is it okay to just include that in our first signed Grub package version, since it would not affect the shim hash/acceptance directly?

Because the SBAT entries are part of the review, can you update it to include the new entry? Then just create a new tag where this is the only difference. After this it looks good to go from my side.

Fabian-Gruenbichler commented 11 months ago

done!

we will closely follow Debian's development for all involved packages - at the moment the last CVE fixes for Grub (CVE-2023-4693 and CVE-2023-4692) are still not available for Bookworm, but we will ensure they are folded in before releasing our first signed packages, which will likely entail another SBAT bump for the upstream component.

THS-on commented 11 months ago

Tag proxmox-shim-15.7-amd64-20231006 looks now ready from my side.

I would like for at least one other reviewer to take a look at it before marking it as accepted (and the completing the contact verification). @aronowski you already had a look at it, can you take another closer look?

ThomasLamprecht commented 11 months ago

As requested, for the Proxmox Security Team I got the following verification words in the mail: outputs wholesales vitamin's golf's alphanumeric connection genres retention treadled pictured

ClaudioGranatiero-10zig commented 11 months ago

I'm not an authorized reviewer, I'm just trying to help and learn.

shimx64.efi: file format pei-x86-64

Contents of section .sbat: d0000 73626174 2c312c53 42415420 56657273 sbat,1,SBAT Vers d0010 696f6e2c 73626174 2c312c68 74747073 ion,sbat,1,https d0020 3a2f2f67 69746875 622e636f 6d2f7268 ://github.com/rh d0030 626f6f74 2f736869 6d2f626c 6f622f6d boot/shim/blob/m d0040 61696e2f 53424154 2e6d640a 7368696d ain/SBAT.md.shim d0050 2c332c55 45464920 7368696d 2c736869 ,3,UEFI shim,shi d0060 6d2c312c 68747470 733a2f2f 67697468 m,1,https://gith d0070 75622e63 6f6d2f72 68626f6f 742f7368 ub.com/rhboot/sh d0080 696d0a73 68696d2e 70726f78 6d6f782c im.shim.proxmox, d0090 312c5072 6f786d6f 782c7368 696d2c31 1,Proxmox,shim,1 d00a0 352e372c 68747470 733a2f2f 6769742e 5.7,https://git. d00b0 70726f78 6d6f782e 636f6d2f 3f703d65 proxmox.com/?p=e d00c0 66692d62 6f6f742d 7368696d 2e676974 fi-boot-shim.git d00d0 0a .

shimx64.efi: file format pei-x86-64

Contents of section .sbatlevel: 84000 00000000 08000000 22000000 73626174 ........"...sbat 84010 2c312c32 30323230 35323430 300a6772 ,1,2022052400.gr 84020 75622c32 0a007362 61742c31 2c323032 ub,2..sbat,1,202 84030 32313131 3530300a 7368696d 2c320a67 2111500.shim,2.g 84040 7275622c 330a00 rub,3.. 

- Certificate seems ok:

Certificate: Data: Version: 3 (0x2) Serial Number: 4d:5a:3c:bd:e3:65:b7:4a:b3:b7:5e:09:f8:7d:a6:84:40:76:48:d5 Signature Algorithm: sha256WithRSAEncryption Issuer: C = AT, ST = Vienna, L = Vienna, O = Proxmox Server Solutions GmbH, CN = Secure Boot CA, emailAddress = office@proxmox.com Validity Not Before: Mar 6 13:51:34 2023 GMT Not After : Mar 3 13:51:34 2033 GMT Subject: C = AT, ST = Vienna, L = Vienna, O = Proxmox Server Solutions GmbH, CN = Secure Boot CA, emailAddress = office@proxmox.com Subject Public Key Info: Public Key Algorithm: rsaEncryption Public-Key: (4096 bit) Modulus: 00:9e:7d:98:ff:20:44:ba:ac:03:a3:f9:dd:e8:f2: d3:03:24:d2:a2:e5:20:2b:43:90:f5:ec:26:88:b8: 41:11:e3:94:f1:2b:c5:7b:f9:ce:c6:78:5a:a4:86: d9:b4:3c:11:6f:79:14:07:fc:10:e1:7a:81:ef:86: d7:b9:de:62:dd:70:0e:69:6f:1f:32:c1:7e:a4:c9: 8f:91:01:46:05:63:75:63:a3:5f:70:24:4b:f7:a5: 3b:2c:c7:76:ae:2b:28:eb:5e:51:2a:50:94:c3:f8: b9:bd:9c:01:47:72:ff:ca:e9:a2:e4:db:76:eb:6d: 36:ff:26:ca:60:0e:f3:df:e4:6d:70:84:98:a1:62: 45:2f:9a:bf:e8:72:90:51:72:5c:7f:ca:4d:16:b6: 42:7b:cb:55:a7:4c:12:49:ad:03:eb:e6:fb:24:21: 61:60:93:0f:c8:5d:8a:c6:b5:a3:77:4a:33:46:76: 97:c2:f2:cd:f2:0e:b1:55:dc:c0:71:c1:ec:08:6d: 47:8b:5b:2d:1f:53:cf:6a:7f:9b:d1:b3:ff:fb:e3: f3:67:41:68:28:61:ea:4c:48:31:9f:24:00:23:f6: f1:d0:39:eb:54:76:3f:a6:5c:8f:32:d2:27:44:ea: e5:ee:2d:89:6f:52:34:4c:ee:4d:82:93:46:f1:5e: 42:84:21:e2:eb:03:75:21:77:89:f5:c3:ba:29:92: 86:9b:fc:61:a7:f8:7e:7b:72:21:a5:1f:9e:6e:87: e6:db:cd:af:d6:fd:47:0e:99:2f:6d:34:2d:5e:55: ba:04:35:12:97:c4:17:4f:9c:dc:b4:1f:ba:bc:13: ee:cb:c8:4a:05:d2:63:ad:85:7b:f4:e2:1c:50:0d: 39:55:cc:e6:3d:ff:0e:bd:a4:b4:ac:9c:7b:b0:54: 18:de:1e:c2:95:8f:bc:ff:7d:8a:8f:ab:75:de:1b: 4d:17:ae:0e:1f:23:d2:78:6c:49:d6:0a:7e:fb:8e: 9a:f4:29:80:bf:12:9b:39:e8:e7:8f:e1:76:9b:8e: e9:cb:fe:b2:50:ab:d4:0a:43:7b:19:7c:80:03:cf: c0:26:51:b0:7e:da:3d:cf:76:83:e6:0f:cc:39:b7: 93:be:d1:aa:40:97:3c:0d:7d:28:a4:0b:60:ab:cb: ed:af:87:de:b9:ea:80:ef:35:09:d1:85:3c:73:b3: 23:95:2b:b8:a2:cb:78:31:80:59:1d:1b:a3:42:6a: 0c:c1:0b:f3:fa:0f:87:d2:2f:1a:8f:25:15:c4:eb: a2:cd:0d:12:87:fa:03:ba:7c:48:69:b1:33:63:68: 75:3a:3d:b2:90:d5:b3:ce:31:0c:f9:69:e3:25:f8: bb:c0:3d Exponent: 65537 (0x10001) X509v3 extensions: X509v3 Basic Constraints: critical CA:TRUE X509v3 Key Usage: critical Digital Signature, Certificate Sign, CRL Sign X509v3 Extended Key Usage: Code Signing X509v3 Subject Key Identifier: B5:F1:B1:1E:48:2D:B5:86:93:29:CB:97:5A:A3:52:05:F8:72:CD:7A X509v3 Authority Key Identifier: B5:F1:B1:1E:48:2D:B5:86:93:29:CB:97:5A:A3:52:05:F8:72:CD:7A X509v3 Subject Alternative Name: email:office@proxmox.com X509v3 Issuer Alternative Name: email:office@proxmox.com Signature Algorithm: sha256WithRSAEncryption Signature Value: 05:06:59:3b:7b:f7:68:9b:56:d1:3d:c8:a8:2a:f5:8b:66:c9: b2:86:5a:30:c7:74:c2:a5:e4:07:27:c4:b0:fa:d3:3e:dc:12: 07:14:06:91:17:69:46:d3:ce:27:39:af:3e:df:62:15:0c:db: d2:d9:90:5f:0d:20:0d:9a:af:3a:c5:d3:0f:89:d7:36:59:28: 6d:95:bd:35:a6:76:0c:a7:34:d2:90:fe:24:d1:6e:18:12:67: de:21:11:63:b0:ba:69:44:5b:83:ba:1f:05:0d:f9:56:05:c7: cc:1f:8e:fc:96:73:52:ba:71:d0:9c:ef:42:1e:0b:93:ce:8d: 84:e5:5c:a3:e8:0e:af:32:b0:f3:0b:87:19:bb:76:c9:3b:0c: 35:9c:47:4b:22:31:b6:2a:18:b2:ca:e5:c2:41:95:91:3e:f5: 0d:cd:8f:fa:b8:9b:7a:1d:8c:ad:48:2e:b0:c6:2a:a5:96:26: 0c:68:a4:4d:b5:7b:dd:37:12:3f:93:37:0f:93:76:e5:f2:80: 7a:b0:56:5a:04:5d:48:b8:72:59:8c:33:00:9f:84:40:48:f5: 5b:b2:d3:e4:12:2f:1b:eb:1c:a6:f0:62:95:9d:e7:d4:41:40: 4b:14:b9:b8:bc:b1:3f:0e:87:b9:e3:82:61:1d:fc:f7:fd:b8: 3e:d4:d5:17:7d:23:ed:4c:90:d3:99:5f:2a:32:bd:a2:50:97: d3:59:f5:1a:72:1b:e6:5b:79:0a:05:70:bd:82:78:2c:b4:8c: 94:10:4a:29:48:e6:4e:11:b6:20:eb:1a:7e:91:1b:19:51:2f: f1:53:bc:65:81:91:01:8d:c2:fa:1e:17:00:fa:8b:bc:d8:6a: 29:df:d5:9e:df:d4:38:7c:dc:cc:4a:6a:bc:69:03:05:fa:d4: af:ab:1e:9b:c0:be:33:7f:1d:de:1d:b2:58:f9:0b:ea:75:f3: c1:67:19:aa:a2:16:51:40:81:57:05:ff:f1:7e:c9:91:46:62: ed:6c:4c:84:09:e5:fd:02:dc:b8:85:2d:e4:18:8b:74:8f:4d: fd:0b:c2:4e:fc:78:78:28:eb:1d:5c:20:e3:37:fc:2b:78:99: f9:47:07:19:eb:7c:e2:7d:61:d1:00:9f:63:24:96:90:d3:e5: d8:9f:fb:df:b3:a4:dd:66:c8:24:27:e0:7a:16:3a:0a:16:21: a7:12:ba:fb:85:73:31:d3:41:fd:88:5f:86:44:10:4e:52:67: 01:37:d5:fe:c4:d1:ca:a3:81:79:b0:6f:3d:5a:f3:55:0c:64: 7c:ef:af:bd:6b:0d:e5:a2:a1:18:04:dc:f7:b5:86:dc:e9:3a: 6c:39:62:0b:e3:b6:7f:24

- Alignment is ok:

bjdump -x shimx64.efi | grep -E 'SectionAlignment|DllCharacteristics' SectionAlignment 00001000 DllCharacteristics 00000100

- NX_COMPAT is enabled:

bjdump -x shimx64.efi | grep -B 1 NX_COMPAT DllCharacteristics 00000100 NX_COMPAT

- Section Code is not Writable (OK):

pedump --sections shimx64.efi

=== SECTIONS ===

NAME RVA VSZ RAW_SZ RAW_PTR nREL REL_PTR nLINE LINE_PTR FLAGS "/4" 5000 1d6ec 1d800 400 0 0 0 0 40000040 R-- IDATA .text 23000 5d9aa 5da00 1dc00 0 0 0 0 60000020 R-X CODE .reloc 81000 a 200 7b600 0 0 0 0 42000040 R-- IDATA DISCARDABLE "/14" 83000 6b 200 7b800 0 0 0 0 c0000040 RW- IDATA "/26" 84000 47 200 7ba00 0 0 0 0 40000040 R-- IDATA .data 85000 2cdf4 2ce00 7bc00 0 0 0 0 c0000040 RW- IDATA "/37" b2000 682 800 a8a00 0 0 0 0 40000040 R-- IDATA .dynamic b3000 100 200 a9200 0 0 0 0 c0000040 RW- IDATA .rela b4000 1b468 1b600 a9400 0 0 0 0 40000040 R-- IDATA .sbat d0000 d1 200 c4a00 0 0 0 0 40000040 R-- IDATA



--------------------------------------------------------------------------
THS-on commented 11 months ago

Phrases are correct, therefore the contact verification is complete.

aronowski commented 11 months ago

@THS-on,

@aronowski you already had a look at it, can you take another closer look?

You know it!

A thorough review will be made the next Tuesday at the earliest. I'm kind of all tied up at the moment and commenting on this for transparency reasons.

aronowski commented 11 months ago

done!

we will closely follow Debian's development for all involved packages - at the moment the last CVE fixes for Grub (CVE-2023-4693 and CVE-2023-4692) are still not available for Bookworm, but we will ensure they are folded in before releasing our first signed packages, which will likely entail another SBAT bump for the upstream component.

Basically what we've been discussing back then in April - seems A-OK to me!

No "thorough review" as I said back on Saturday, though - I had too much on my mind and didn't check, that it was only the SBAT-related thing that changed.

@THS-on,

I would like for at least one other reviewer to take a look at it before marking it as accepted

Done!
Please, label this application as accepted and we can celebrate!

THS-on commented 11 months ago

Marked as accepted. Thanks you @aronowski and @ClaudioGranatiero-10zig for helping with the review.

Fabian-Gruenbichler commented 11 months ago

thanks for the reviews (and acceptance)! :)

Fabian-Gruenbichler commented 8 months ago

got a signed shim from MS and using it in production already! closing accordingly.