Shim 15.7 for EuroLinux 9 #327
Comments
|
I'm not an authorized reviewer, I'm just trying to help and learn.
Only a warning: here seems that the general consensus about a certificate validity is around 10 years, I see yours is set to 18 years from now. No problem for me (Secure Boot doesn't check the validity), but maybe the official reviewer will want to check that.
|
|
Thanks for the review. Regarding the certificate validity curiosity, this was done according to the reviewer guidelines.
|
|
I'm not an authorized reviewer, I'm just trying to help and learn.
|
|
Hello people! First of all I'd like to thank @keithdlopresto and @ClaudioGranatiero-10zig for reviewing my issue. Partaking in Open Source projects requires effort indeed. It's vital that a strong community work together in order to achieve great things. Without it, we wouldn't have the systems we have today. The same is applicable when it comes to reviewing shims. Robbie suggested it should all be peer review in here:
I suggest reading the whole conversation in this pull request and the proposals I provided in my comment there to help the peer review idea become a thing and make it easier for people to get into this. So for this to happen I'd like to ping all the people I've had interactions with to help me out with this:
Go ahead @Fabian-Gruenbichler, @MuthuvelKuppusamy, @amzdev0401, @joseph-zeronsoftn, @jsegitz, @mheese, @nicholasbishop, @ozun215, @parheliamm, @rehakp, @uibmz, @vden-irm, help me out with this. Thanks in advance! |
|
I will review your SHIM submission completely. The first thing I noticed, SHIM code needs to be forked from the following branch. |
|
As far as I could see back then and can see now, forking this repository is not required. Just cloning it and uploading to a new remote. Historically Red Hat, Inc. did so and everything is alright. |
|
I'm not an official reviewer, I'm just peer reviewing this as @aronowski peer reviewed mine and asked me/us to do the same. Here are my findings that check all the boxes:
$ sha256sum shimx64.efi
c1c4b58a3cd11df0fa9af88ea13591dee6525362d100ab4846e7a3a798afaa5b shimx64.efi
$ pedump --extract section:.sbat shimx64.efi
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.eurolinux,1,EuroLinux,shim,15.7,[email protected]
❯ pedump shimx64.efi
...
=== PE Header ===
signature: "PE\x00\x00"
# IMAGE_FILE_HEADER:
Machine: 34404 0x8664 x64
NumberOfSections: 10 0xa
TimeDateStamp: "1970-01-01 00:00:00"
PointerToSymbolTable: 812032 0xc6400
NumberOfSymbols: 3664 0xe50
SizeOfOptionalHeader: 240 0xf0
Characteristics: 518 0x206 EXECUTABLE_IMAGE, LINE_NUMS_STRIPPED
DEBUG_STRIPPED
# IMAGE_OPTIONAL_HEADER64:
Magic: 523 0x20b 64-bit executable
LinkerVersion: 2.35
...
SectionAlignment: 4096 0x1000
FileAlignment: 512 0x200
...
Subsystem: 10 0xa EFI_APPLICATION
DllCharacteristics: 256 0x100 NX_COMPAT
...
...
=== SECTIONS ===
NAME RVA VSZ RAW_SZ RAW_PTR nREL REL_PTR nLINE LINE_PTR FLAGS
"/4" 5000 1db34 1dc00 400 0 0 0 0 40400040 R-- IDATA
.text 23000 5e715 5e800 1e000 0 0 0 0 60300020 R-X CODE
.reloc 82000 a 200 7c800 0 0 0 0 42100040 R-- IDATA DISCARDABLE
"/14" 84000 49 200 7ca00 0 0 0 0 c0600040 RW- IDATA
"/26" 85000 47 200 7cc00 0 0 0 0 40300040 R-- IDATA
.data 86000 2d5b4 2d600 7ce00 0 0 0 0 c0600040 RW- IDATA
"/37" b4000 40f 600 aa400 0 0 0 0 40300040 R-- IDATA
.dynamic b5000 f0 200 aaa00 0 0 0 0 c0400040 RW- IDATA
.rela b6000 1b468 1b600 aac00 0 0 0 0 40400040 R-- IDATA
.sbat d2000 c0 200 c6200 0 0 0 0 40100040 R-- IDATA
❯ openssl x509 -text -noout -inform DER -in eurolinuxCA.cer
Certificate:
Data:
Version: 3 (0x2)
Serial Number:
2b:08:6d:be:fc:95:93:e8:a4:e7:d1:db:39:1a:f9:79:e8:84:85:5f
Signature Algorithm: sha256WithRSAEncryption
Issuer: C = PL, ST = Poland, L = Cracow, O = EuroLinux Sp. z o.o., CN = EuroLinux Secure Boot CA
Validity
Not Before: Feb 11 13:32:30 2022 GMT
Not After : Oct 29 13:32:30 2041 GMT
Subject: C = PL, ST = Poland, L = Cracow, O = EuroLinux Sp. z o.o., CN = EuroLinux Secure Boot CA
Subject Public Key Info:
Public Key Algorithm: rsaEncryption
RSA Public-Key: (2048 bit)
Modulus:
00:d8:5d:79:7e:fc:d0:7d:8d:08:de:4d:77:74:07:
5b:85:74:44:49:50:6d:61:b6:b3:14:b0:ff:2b:06:
7b:3b:a7:40:67:f8:50:07:60:cf:81:5f:b7:dc:da:
19:93:21:e2:52:44:40:e0:11:64:3f:3c:83:6b:bd:
dd:cd:dc:24:7f:7a:91:53:95:7c:14:57:f0:78:49:
4f:6d:2c:de:a8:cc:85:2e:93:2e:f1:6a:a1:65:53:
ad:99:46:18:e8:87:7b:b9:b0:e7:e0:c7:9c:50:78:
74:e3:3d:dd:e7:16:79:8b:69:3d:52:51:18:5a:88:
31:08:0a:64:c2:97:ff:b5:15:39:45:dc:4b:f7:29:
30:65:67:aa:4b:93:1b:d3:1a:b9:af:ad:c1:c2:ea:
74:7b:cc:1f:ae:4e:ef:99:35:9b:7f:ff:26:55:8d:
a3:93:1c:71:79:52:21:df:15:41:78:a6:d8:a2:4c:
4b:54:24:74:29:6a:56:bc:d4:d2:17:04:10:97:23:
97:eb:a3:7d:19:80:85:ff:e4:c6:da:6d:af:51:6c:
1c:ce:0c:21:4e:35:d0:e8:9d:e8:90:61:95:62:3a:
62:48:dd:cb:cc:f7:d1:7a:77:eb:f8:9c:1a:45:ef:
bf:fb:8e:c0:5e:49:e9:79:b6:c8:1e:05:25:45:4e:
e8:a7
Exponent: 65537 (0x10001)
X509v3 extensions:
X509v3 Subject Key Identifier:
F3:5C:51:DD:7A:39:F5:95:30:39:EE:51:87:3C:9C:D6:4D:06:43:EB
X509v3 Authority Key Identifier:
F3:5C:51:DD:7A:39:F5:95:30:39:EE:51:87:3C:9C:D6:4D:06:43:EB
X509v3 Key Usage:
Digital Signature, Non Repudiation, Key Encipherment, Data Encipherment, Key Agreement, Certificate Sign, CRL Sign
X509v3 Basic Constraints: critical
CA:TRUE
Netscape Comment:
EuroLinux Secure Boot CA
Signature Algorithm: sha256WithRSAEncryption
Signature Value:
26:a5:8c:fe:f8:18:70:54:19:e6:52:a2:53:d4:4c:e3:b4:5e:
5a:52:ae:c1:ec:a5:db:a9:bf:8d:ef:a0:5d:cb:43:a2:be:88:
d0:08:24:4d:2e:9d:a9:bc:dc:b6:f6:24:6d:51:c8:d9:8e:2b:
56:aa:da:32:0d:b2:d2:1d:67:f6:2d:e6:63:a0:8d:3a:6b:04:
ae:cd:a3:0a:49:e7:49:49:ea:21:4a:0c:fe:49:e5:75:57:3b:
0b:40:dd:26:10:b9:75:dd:ce:cd:3f:0d:5c:3f:64:16:2b:e5:
98:3d:7e:87:06:25:bb:08:8b:32:0c:bc:1c:83:be:7f:ff:46:
79:51:1b:d6:78:a3:df:7b:fb:f6:dc:1d:2b:af:b4:3a:91:e5:
a8:64:bf:1e:a8:af:b6:da:ae:15:ef:aa:13:66:ad:b4:10:6d:
f8:db:a8:42:e4:d9:20:47:c5:b9:0a:4e:bf:f4:60:75:e3:9d:
02:ec:0b:b5:5b:cc:f7:41:d2:d3:17:f6:12:e6:e3:a5:1b:7d:
ec:0d:82:72:69:b4:b3:19:89:46:1a:12:fc:4d:85:74:ec:0b:
e4:28:2b:53:ed:f2:0e:c9:2c:6d:5c:a1:f7:17:6f:42:e4:86:
13:4b:4c:b3:87:e3:6e:dd:15:a8:93:23:4f:f8:c0:95:89:d5:
24:92:fe:50And here are the potential problems/issues that I have found:
shim.eurolinux,1,EuroLinux,shim,15.7,[email protected]The last field is supposed to be a URL.
TBH, this bothers me in general about this process: we are asked to do that, but there are no checksums or signatures for tarballs released, so it's kind of problematic to begin with. Also in general, I love how detailed the submission is: it includes detailed explanations on how the whole boot chain includes NX compatibility, including references to patches etc. @aronowski, I hope this peer review helps you, and thanks again so much for peer reviewing my submission as well! |
|
@mheese, thanks for pointing the things out. That's the way it should be!
Edit: just to clarify: I've only changed the contents of Dockerfile, switching to a fixed version of EuroLinux 9.1, leaving everything else as it was, e.g. the build logs, which mention Oracle Linux 9.1. Since the systems from the Enterprise Linux family are 1:1 binary compatible, the final artifact is the exact same too.
The answer sure helped. I owe you a big one! |
Yes, the build reproduces fine now! Kind of plays into my point (4) ;)
As I was saying, this is probably nitpicking :) ... although technically this is even something which would need to be fixed in RHEL. It doesn't parse as a URL.
Yeah, no idea why github markdown skipped one number :)
I don't really have an issue with source RPMs personally, just trying to point out that it might be harder to proof that this is indeed coming from a 15.7 tarball. As your distro is sort of RHEL clone, and RHEL is doing it the same way, it probably isn't a problem at all. This is definitely a problem for the review process though. For example, I did not go through the trouble to see if this source RPM is truly from a clean 15.7 source tree.
Glad this was helpful! And let's hope by doing peer reviews this sends the right message to the official reviewers that we are all willing to help/participate as best as we can. |
|
Review for
|
|
Thank you for the review, Thore! Appreciate it!
The ExtKeyUsage/CodeSigning value in the root certificate is an optional value. In many cases, the absence of this extension in the root certificate does not interfere with code signing or other operations that require this certificate. Therefore, I don't think there's a need to intervene, especially as we already have a working stack.
That's right.
For context, I introduced the kernel NX-related entries once I was about to publish the initial application for a Shim review, i.e. with the commit After this application got published, and I made several reviews, then I got this tip from Julian on the required patchset. There are several things I'd like to address here: Back then (when publishing the application) I interpreted the Microsoft requirements as having only the NX bit set in a PE/COFF binary being satisfactory. Once I got a tip from Julian, I researched this a bit more and came to the conclusions that, apart from patching, a thorough research and a laboratory setting would be needed to ensure all this works correctly. I found this Microsoft's article that may shed some light on the matter. To quote them: Since No Execute (NX) compliance can't be detected statically, firmware that sets `IMAGE_DLLCHARACTERISTICS_NX_COMPAT` should follow these steps to ensure that the firmware image can operate correctly with NX protections applied. So, if I'm understanding this correctly, we could prove that NX is not properly implemented if we could achieve at least one of the forbidden entries listed via either exploiting the kernel somehow or compiling a custom one, which can trigger something listed above, e.g., via a hotkey. Am I right? Am I wrong? I don't know - I'm not a kernel developer, low level programmer or a malware analyst. Recently I shared some thoughts publicly as part of this comment and as said there, I'd like an expert like Peter to confirm, what's there already and what needs to be ported. Now, in regard to this patchset, it has not been ported as I really don't want to accidentally cause incompatibilities or even additional security issues, if it has not been approved in the RHEL kernel ours is based on. As far as I'm concerned, the one in RHEL doesn't have this yet, but got approved, so, if this assumption is right, then we can well wait for a proper port once Microsoft demands it being right there.
So, just as I said that we can wait for a proper port, this is in alignment with another post by Julian:
We already have a proven one in Shim, as well as in GRUB2. Once a well-respected entity like Red Hat, Inc. has it in their kernel, I'll be sure it's a proven one, and in the meantime I'll be happy to assist in its development and integration, if I'm allowed to.
Yes, the appropriate x509.genkey.rhel asset has been added to our application. It normally resides inside a kernel's Source RPM and is used as part of the build process when |
THS-on
added
extra review wanted
and removed
question
|
@aronowski thanks for answering the questions, now LGTM! Regarding NX kernel support on RHEL based distributions. As far as I can see the last submissions were done still before the NX deadline or submitted with some exemption (#305, #304). |
That's right. Looking back, I think that if there's a need for the proper port I mentioned in my earlier comment, I'd just consult with the people involved in it, if that's a possibility. If not, I'd stick to the implementation provided by RHEL. Now, my way of "NX enablement" was just to change one |
|
We've been waiting a long time and I'd like someone from the committee to help us out. |
|
I just ran Dockerfile by
|
|
This reviewed issue opened by @aronowski has been reviewed by many experts, including @ClaudioGranatiero-10zig, @keithdlopresto, @amzdev0401(even not comment here), @mheese, @rehakp, and @THS-on since Mar 23 of this year. All questions were answered. Let's put "accepted" tag on it. |
dennis-tseng99
added
accepted
|
@dennis-tseng99, thank you for the acceptance. Furthermore, I'd also like to thank everyone involved in helping to make this application as good as possible. |
|
What is the status of this @aronowski? Did you get a shim back or are you creating a new submission for 15.8? |
|
@THS-on, we'll create an update regarding 15.8 as we do not have a signed 15.7 binary. There are ongoing discussions, if it's better to update this application or to create a new one. |
|
Superseded by #375 |
aronowski
added
the
superseded
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/EuroLinux/shim-review/tree/eurolinux-shim-x86_64-20230517What is the SHA256 hash of your final SHIM binary?
c1c4b58a3cd11df0fa9af88ea13591dee6525362d100ab4846e7a3a798afaa5b
What is the link to your previous shim review request (if any, otherwise N/A)?
#258
The text was updated successfully, but these errors were encountered: