Shim 15.8 for Canonical

kukrimate commented 10 months ago

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

[x] completed README.md file with the necessary information
[x] shim.efi to be signed
[x] public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
[x] binaries, for which hashes are added to vendor_db ( if you use vendor_db and have hashes allow-listed )
[x] any extra patches to shim via your own git tree or as files
[x] any extra patches to grub via your own git tree or as files
[x] build logs
[x] a Dockerfile to reproduce the build of the provided shim EFI binaries

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

https://github.com/canonical/shim-review/blob/ubuntu-shim-amd64+arm64-20240202

What is the SHA256 hash of your final SHIM binary?

$ sha256sum shim*.efi
21d895284c1783b4e3d82584bc4aca197204f385f0da2192e2222e501ed9cc1b  shimaa64.efi
39037872a0bb357a0f40c5e3ce6a1757b5b54c78c31f8d5da01169c2ca94b3a7  shimx64.efi

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

https://github.com/rhboot/shim-review/issues/298

jsetje commented 9 months ago

The build reproduces for me. The vendor dbx patch seems reasonable.

The generation number all look OK, but we should probably agree on when the vendor specific ones are bumped. Incrementing them for every release could provide us with additional options in the future. (This is not a blocking comment.)

I didn't spot any issues with the rest of the answers.

kukrimate commented 9 months ago

@jsetje

Thanks for the review.

Note for reviewers:

I have been added to the list of security contacts in this release, we also have two other already verified. I'll likely be handling posting shims here for the foreseeable future, so maybe we should the verification dance. (My GPG key fingerprint is in the README and available on keyserver.ubuntu.com)

aronowski commented 9 months ago

I have been added to the list of security contacts in this release, we also have two other already verified. I'll likely be handling posting shims here for the foreseeable future, so maybe we should the verification dance.

Alright! I'll send a verification email soon.

If I may put my two cents in, I'd kindly nitpick on this answer:

*******************************************************************************
### If your boot chain of trust includes a Linux kernel:
[...]                              
### Is upstream commit [eadb2f47a3ced5c64b23b90fd2a3463f63726066 "lockdown: also lock down previous kgdb use"](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eadb2f47a3ced5c64b23b90fd2a3463f63726066) applie
d?
*******************************************************************************
All Ubuntu kernels in all currently supported series have the above
applied.

While the CVE-2022-21499 fix implementation is (supposedly) realized by porting the commit eadb2f47a3ced5c64b23b90fd2a3463f63726066, please say "ported" or something similar rather than "applied", as it suggests the upstream code is there 1:1, while the implementation differs a bit, what may cause confusion for other reviewers - for instance, grepping the sources for LOCKDOWN_DBG_WRITE_KERNEL prints nothing, while LOCKDOWN_KGDB is used instead (I've just checked with Ubuntu Jammy kernel sources).

Furthermore, the upstream changes in kernel/debug/kdb/kdb_main.c appear to be missing, so an explanation on how this accomplishes mitigating the security issue would really come in handy. Especially for those who are not C programmers or don't follow the kernel development closely.

aronowski commented 9 months ago

Verification email sent to mate.kukri@canonical.com.

kukrimate commented 9 months ago

Verification words:

lofts pompoms larboard sol conceptualizes tackled blogs undergone apocryphal arrangement

kukrimate commented 9 months ago

Also re kernel patches, wording can be changed next time. Point is lockdown bypass fixes are applied and tested by the kernel team, and I assume change are made as appropriate.

I don't maintain kernel code, so can't provide details off the top of my head. But if you are concerned about specific upstream changes missing I can forward that to the kernel team.

aronowski commented 9 months ago

Point is lockdown bypass fixes are applied and tested by the kernel team, and I assume change are made as appropriate.

Alright! Thanks for sharing.

I wanted to double-check that everything is correct in case something changed in the meantime, as well as because I'm not a Canonical employee and don't know, what processes are being done behind the scenes, where I can't see.

Furthermore, I could base the Canonical implementation as the source of truth for any downstream vendors that reuse this kernel. Hence why I wanted to make sure the ported patch mitigates CVE-2022-21499.

But if you concern about specific upstream changes missing I can forward that to the kernel team.

It's more about requesting an explanation on how the code mitigates that CVE or even better a guide, how to test the final kernel artifact out in a laboratory setting to ensure there are no security issues. Not everyone is a programmer and a guide would let others replicate the setting too and confirm that things are alright.

xnox commented 9 months ago

Valid questions which in theory should have already had publically traceable documentation (unless i need to chase people up with a yard stick). Can you please request review from me on this PR to comment with details?

aronowski commented 9 months ago

@xnox, sure thing - assigned you.

Some help will definitely come in handy as reviewing on my own would take a lot of time for checking the document for correctness, as well as learning new things - while I trust that Canonical Ltd. has the skills to maintain and test out all their software stack, I don't want to be held accountable for missing some crucial details, if there happens to be an error somewhere.

That's especially true regarding technologies I don't work with - for instance I have some experience with UKIs but loading them through GRUB2 only, and this software stack is different.

julian-klode commented 9 months ago

@aronowski Well @xnox is commenting as part of our kernel team, not as a separate entity, I don't think that was clear.

aronowski commented 9 months ago

The assignment is being done from organizational standpoint, at least that was my intention. I suppose next time I should only leave a request in a comment, like I did some time ago here.

If that's not correct, we can always unpin an account from this GitHub issue.

jsetje commented 9 months ago

@aronowski can you confirm that you're happy with the answers? If so, I'd like to approve this.

aronowski commented 9 months ago

@jsetje, to a certain degree, yes. If it turns out I missed something and Microsoft will sign a shim for a chain that has security issues, I won't be held accountable. ;-)

Notice: I haven't reviewed the complete application yet, as this one will be tough. I just spotted a curiosity as part of reviewing #362. Since we've been discussing that the following reviews are needed:

Minimum of 3 for a new submitter, at least one official/accredited

Minimum of 2 for previously-accepted submitters, at least one official/accredited

I'll unpin myself and let another person (may not be in the committee) review this one.

jsetje commented 9 months ago

@aronowski in the interest of moving this along I'm going to count you as a reviewer here.

kukrimate commented 9 months ago

Signed shims received, closing.

rhboot / shim-review