shim-15.8 for WAXAR - Githubissues

Jurij-Ivastsuk commented 7 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 (we use no extra patches)
[x] any extra patches to grub via your own git tree or as files (no extra patches, we use Canonical upstream for GRUB)
[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/Jurij-Ivastsuk/WAXAR-shim-review/tree/waxar-shim-x86_64-aarch64-20240601

What is the SHA256 hash of your final SHIM binary?

SHA256 (shimx64.efi): 1894c7e467991c117162e1e40dd61ed85a5f790a68216fdbee93b2619b70df67 SHA256 (shimaa64.efi): 5c7238473401d791c7f376efee90cf1e9fe23e2bf1814f4795474d57920bdfbf

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

N/A

aronowski commented 7 months ago

Hello.

I'll start reviewing the application soon, but the first requirement is that all the checkboxes are checked and render well.

Please, fix the formatting and check the missing one - it's supposed to mean "if we have any extra patches, then they have been included."

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, many thanks for any tips! I literally understood point 6 as "if you have extra patches for GRUB..." Since we use the GRUB software upstream from Canonical, and don't make any changes there, with patches or otherwise, I figured I shouldn't mark this point as done.

aronowski commented 7 months ago

@Jurij-Ivastsuk, thank you!

I agree that some part in here may not be clear, but I'll try my best to address them. Some documents I've been working on improving during my free time, but haven't requested to incorporate them so far.

Reviewing also may not be easy especially when dealing with environments I'm not that familiar with or analyzing patches for correctness (took me about 8 hours to spot one major error, but I did it). If there's no activity e.g. by the end of next week, ping me.

Jurij-Ivastsuk commented 7 months ago

@aronowski, I understand and I thank you for the work you do in your spare time!

aronowski commented 7 months ago

Hello.

I did scratch the surface of the application. There are some curiosities and things that need to be fixed. I address them as some loose thoughts below.

In the meantime, I'm sending contact verification emails soon. Please, post the random words, that I'm emailing you, in the comments.

The NX compatibility bit should be enabled only if the whole bootchain is NX compatible - that's the requirement Microsoft has, as I'm aware right now. Therefore, the patch 530 shall not be used.

Furthermore, the validation function patch does its job in regard to older requirements, which have changed - there's no need to use it today.

Please, update the binaries and the building process as described - I'll then try to rebuild it and check if everything is correct.

The 626.patch PR hasn't been merged and debugging the code in my head is out of the question. It's just a one-line change, but no idea about the consequences and possible corner-cases, especially since I'm not a low-level programmer.

I'm not saying that it's unable to be approved as-is - just that I'll need some help with it and wondering, how to get some.

Linux required upstream commits - are they applied for the 5.15.1 Debian kernel?

The commit eadb2f47a3ced5c64b23b90fd2a3463f63726066 as far as I know got introduced in Linux v5.19. Is it backported in Debian?

There's no ephemeral key - is the strategy sufficient, as it's described right now?

It would be preferable to mention the usage of CONFIG_MODVERSIONS=y and CONFIG_MODULE_SIG_FORCE=y. Furthermore, is a new key being used to sign each new kernel build?

The CA certificate waxar.ceris valid for almost 100 years and it's only 2048 bit.

I don't know if Microsoft would be willing to approve this - any chances of generating one with a shorter lifespan and/or using 4096 bit public modulus?

aronowski commented 7 months ago

Verification emails sent.

Jurij-Ivastsuk commented 7 months ago

I have received your email. Here is the decrypted text: summerhouses ants regionally announces unmasking fusing addling bantered sprouted chiropractic

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, thank you very much for the first hints! Now I have changed the following:

patches 530 and 531 (NX compatibility) have been removed
docker file adapted accordingly
CA certificate waxar.cer is now valid for 50 years and its size is now 4096 byte
for kernel building we use CONFIG_MODVERSIONS=y and CONFIG_MODULE_SIG_FORCE=y
the new certificate is already used for signing kernel build as well as for signing all modules Regarding commit eadb2f47a3ced5c64b23b90fd2a3463f63726066 (CVE-2022-21499) in Debian, I have found the following information: bullseye 5.10.197-1 fixed bullseye (security) 5.10.205-2 fixed bullseye 5.10.120-1 fixed bookworm 6.1.66-1 fixed Exactly for kernel 5.15.1 no Information could be found... As for 626.patch, I understand that you cannot be responsible for all matters. Maybe you can ping someone who does low-level programming and who can properly assess our PR request. Actually I explained the problem and the solution pretty well. Without this patch we can't use shim like this because it keeps giving us errors (screenshot and explanation in PR 626).

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, should we now switch to 15.8 ?

aronowski commented 7 months ago

Updating should extend the lifecycle of the review if something happened and if reviews from other people from the committee got delayed. I'd do so, especially since I just scratched the surface and will perform a further review, after the latest fixes have been applied.

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, the second contact (Lukas Kienbaum) have received your email (shim-words x2). Here is the decrypted text: hauled multiplication Collins reemerge exhilarates fathers Vijayawada stubbiest fête refile

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, did I understand you correctly, we are waiting until the last reviews from the committee are closed, then you will continue with your review with us? I still don't understand if we will stay with version 15.7 or will we logically switch to 15.8. In this case, if I understand correctly, a new review "shim-15.8 for WAXAR" will be requested. Is that the case?

aronowski commented 7 months ago

@Jurij-Ivastsuk

Please, update all the required things to reflect shim 15.8. I'll then perform further review and ask other reviewers for help (you can ping them as well).

we are waiting until the last reviews from the committee are closed, then you will continue with your review with us?

I'll try my best when there's optimal environment for me - for instance, the comment from January 22 I posted in the middle of the night in my timezone, as I had some free time and no other things on my mind.

Still, there are other things that can make some applications being reviewed ASAP and some that wait for, let's say, a month or even more. The main thing is that I'm just more familiar with certain distros and their environments.

Some things I gathered as part of the improvements I'd like to introduce to the project, so they are blatantly visible, e.g. here.

CA certificate [...] size is now 4096 byte

4096 byte? ;-)

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, thank you very much for the tips and explanations! I really like your suggestions for improvements. I have to apologize for the incorrect representation of the certificate size: of course it's bits and not bytes. So the CA certificate size is now 4096 bits... ;-))) I'll let you know when I've made all the adjustments for 15.8.

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, I have a question, can you please explain to me why the CA certificate must be 4096 bit in size and not 2048 bit?

aronowski commented 7 months ago

@Jurij-Ivastsuk, it's not like it must be this or that, but instead a good practice to follow the recommendations from NIST.SP.800-57pt1r5 - table no. 2 on page 54 and table no. 4 on page 59.

I'll review all the updates soon, once I get more free time and some well-deserved sleep.

Jurij-Ivastsuk commented 7 months ago

@aronowski Hi, please only continue with review if you have slept enough. i have made all adjustments for version 15.8 ;-))

aronowski commented 6 months ago

@Jurij-Ivastsuk,

Exactly for kernel 5.15.1 no Information could be found...

Apologies, that's the Ubuntu Jammy kernel, rather than Debian's as I thought earlier.

So I think our best bet is to wait for an official answer from Canonical kernel maintainers. Please see https://github.com/rhboot/shim-review/issues/368#issuecomment-1943557439.

please only continue with review if you have slept enough

Let's say 6 hours is enough for now. :-P

I was checking things out and got stuck with the kernel-related thing. Let's hope it gets resolved soon.

aronowski commented 6 months ago

Build reproduces, SHA256 sums match. The binaries' characteristics are fine, no NX support, as the whole chain is not NX-compatible.

I was worried about the 626.patch as it's not about the #define used to check, whether an ASCII character is printable, but what happens next (after that one if got changed), if the new check is not satisfied, i.e. to what second_stage points to? A random address rather than the one, where there's a non-printable character?
(In the file load-options.c the line no. 8 contains the declaration CHAR16 *second_stage;, which I suppose may remain not initialized even after parse_load_options returns EFI_SUCCESS.)

Since I'm not a C programmer, I don't feel competent to review the code and I think our best bet is to ask someone else to do such review. As far as I can see, a similar thing got reviewed by @Metabolix - maybe we could kindly ask them to help with the review as issued here?

In other words: if the reasoning above it wrong, don't make fun of me and instead let me know what's the correct one - I'll learn something new.

The kernel-related thing still applies - we can base on Canonical Ltd. being our higher authority and their implementation being our source of truth here. Furthermore, if you can, please post the kernel config file used for compiling the kernel - I might be missing out on something that others will notice, e.g. an option like this one.

Note: there seems to be a mismatch between the git tag used for the earlier (January 15) entry and the current one - I had to use git checkout origin/waxar-shim-x86_64-aarch64-20240115 to get the current revision from February 9.
It may be simply a matter of a missing tag like the commit 4fae3157bebbf700779e3f68c7a46de3170e231a not having its tag waxar-shim-x86_64-aarch64-20240209 pushed with git push --tags. If there are no expected changes to the application, please push such tag and update the original post appropriately.

Metabolix commented 6 months ago

As for 626.patch, second_stage is always initialised before calling parse_load_options, and there are already multiple code paths which leave it unmodified in that function, so the patch is safe to use. (But do note that I'm not a shim maintainer. If you manage to reach any of those, maybe they could actually merge one of the competing pull requests for this load-options issue.)

Jurij-Ivastsuk commented 6 months ago

@aronowski Hi, thank you for your continued efforts! I have set the correct tag (waxar-shim-x86_64-aarch64-20240209) and I hope everything is OK now. I am also attaching the config file we used for the kernel compilation. It is quite a long time ago. Maybe soon we will build a new kernel. The review for our patch seems to be OK? config_WRK_5_15.zip

Jurij-Ivastsuk commented 6 months ago

@aronowski Hi, after several tests I have found that my certificate (waxar.cer) is not working as expected. Therefore in my review files I have to replace this certificate with DER encoded certificate. Of course, this requires adjustments in the Dokerfile and also in the SHA256 hashes. At this point I would like to improve the documentation for Shim-BUILDING, if possible (https://github.com/rhboot/shim/blob/main/BUILDING). There it says (line 3 and 4): cp $MY_DER_ENCODED_CERT pub.cer make VENDOR_CERT_FILE=pub.cer I would make a small change here: cp $MY_DER_ENCODED_CERT pub.der make VENDOR_CERT_FILE=pub.der Because I literally understood this information so: I first generated DER certificate and then I converted DER format to CER format. And that was my mistake. You should definitely use DER formatted certificate for Vendor certificate. I'll let you know when I've adjusted everything.

Jurij-Ivastsuk commented 6 months ago

@aronowski Hi, I have just made all the adjustments. This commit is tagged as: waxar-shim-x86_64-aarch64-20240221 I hope that everything is OK now. If not, please let me know.

aronowski commented 6 months ago

At this point I would like to improve the documentation for Shim-BUILDING, if possible

Yes! Please introduce the contributions that may make the lives of vendors and reviewers easier. I appreciate it.

Hi, after several tests I have found that my certificate (waxar.cer) is not working as expected.

I have a feeling that a contribution for things like these is that apart from a static analysis of a certificate or a shim binary, or improving the proposed review bot, is to write a guide/walkthrough on how to set up a virtualized operating system with a vendor's bootloader chain, so the final result is us witnessing, what effectively happens, after everything has loaded.

I did something like this already, but for Fedora, as that's the system family I have most experience with.

aronowski commented 6 months ago

I've checked the updated certificate, as well as the new shim binaries. Everything seems alright!

Regarding the kernel, I suppose the Ubuntu source package, with all the patches from it, is used, therefore the appropriate CVE fixes have been ported properly. I'm still curious about module signing, though.

Ephemeral key is not used (OK for now):

*******************************************************************************
### Do you use an ephemeral key for signing kernel modules?
### If not, please describe how you ensure that one kernel build does not load modules built for another kernel.
*******************************************************************************
We do not use an ephemeral key. We use a WAXAR HSM backed key for signing kernel modules.

And the kernel appears to be built with CONFIG_MODVERSIONS=y (OK for now).

Please, describe the strategy that enforces that one kernel build does not load modules built for another kernel.

Certain solution proposals have been described in this comment, but you may have an even better solution in your infrastructure - showcase the cool stuff!

Furthermore, I found the following entries as well:

# CONFIG_MODULE_SIG_ALL is not set
# CONFIG_MODULE_SIG_FORCE is not set

The first one is understandable as I suppose the infrastructure is implemented in a way that signing may be being done on a different machine than the kernel building part, but I'm worried about the second one. What happens, apart from the kernel being marked as tainted, if we attempt to load an unsigned module?

Hint: see this document for the kernel used in this application.

Notes:

I'll kindly ask @THS-on for help here.

Also, I recommend committing the kernel config file as part of the shim-review application as well - it will be easier to track.

Jurij-Ivastsuk commented 6 months ago

@aronowski Hi, thank you very much for the information, it is really very helpful and very accurate, especially the comment. As for ephemeral key, there are many reasons why we have not used it including: building and signing are done on different machines. Also, I have done some tests and here are the screenshots: I have tried to start the unsigned and signed with the other certificate modules (they are started from the initrd file). Neither unsigned nor differently signed modules can be started this way. Because the startup process is still under the control of Shim. Loading_KernelInitrd and then comes: BusyBox Only the signed modules with the correct certificate can be started. As you suggested, I have committed the config file in our review. This commit is now tagged with waxar-shim-x86_64-aarch64-20240226.

Jurij-Ivastsuk commented 6 months ago

@aronowski Hi, since we are trying to have the shim signed with our vendor certificate at Microsoft for the first time, I have a question regarding the certificates. Maybe you can help me. For the signing procedure Microsoft requires an Extended Validation (EV) code signing certificate. Does this mean that this certificate must be integrated as a vendor certificate in the shim or is it sufficient if a self-generated certificate can be in the shim (as in our case) and an Extended Validation (EV) code signing certificate is only required for the signing procedure? I believe that if the review process is completed for you, then it would be too late to replace the certificate that we currently have integrated (self-generated) with an EV certificate, wouldn't it?

aronowski commented 6 months ago

@Jurij-Ivastsuk,

Hi, since we are trying to have the shim signed with our vendor certificate at Microsoft for the first time, I have a question regarding the certificates. Maybe you can help me. For the signing procedure Microsoft requires an Extended Validation (EV) code signing certificate. Does this mean that this certificate must be integrated as a vendor certificate in the shim or is it sufficient if a self-generated certificate can be in the shim (as in our case) and an Extended Validation (EV) code signing certificate is only required for the signing procedure?

People have been receiving signed shims with a self-generated CA certificates embedded in their binaries, so it should be fine.
I can't, however, speak on behalf of Microsoft in regard to signing the .cab file to be sent to Microsoft, which contains the shim to be signed.

As you suggested, I have committed the config file in our review. This commit is now tagged with waxar-shim-x86_64-aarch64-20240226.

No need to have it zipped, but alright!

Also, I have done some tests [...] I have tried to start the unsigned and signed with the other certificate modules (they are started from the initrd file). Neither unsigned nor differently signed modules can be started this way. Because the startup process is still under the control of Shim. Only the signed modules with the correct certificate can be started.

I'll need to take a deep dive into this environment as I have not enough experience there. Therefore, asking as someone who also wants to learn something new:

Is it possible to mount a malicious filesystem from that shell and boot from it? See https://wiki.gentoo.org/wiki/Custom_Initramfs#Init for more information. I'd kindly do some labs on this, but they would take some time. So I think it's better to ping an expert here and ask for help.

Jurij-Ivastsuk commented 6 months ago

@aronowski Hi, thank you very much for your efforts!

Jurij-Ivastsuk commented 5 months ago

@aronowski Hello, kindly I would like to ask what is the situation? Has there been any progress?

aronowski commented 5 months ago

No progress on my end - hence why I myself asked that question from a newcomer point of view, as well as suggested pinging an expert.

The current implementation seems to me a bit suspicious security-wise, so I don't want to make any statements like "it's secure, period", when I have my doubts. A healthy dose of vigilance may prevent an end-user from having their system compromised.

Considering that creating the laboratory I mentioned and studying a subject I've never worked with before may take a lot of time, the best way to help is by reviewing other applications, so I have a bit less work to do and can spend the time on the lab.

Alternatively, I can help with rearchitecting the company's build system, so the CONFIG_MODULE_SIG_ALL and CONFIG_MODULE_SIG_FORCE options can be set and the building and signing maybe being done on a single machine. Although I don't know if that will be possible due to things like NDAs and me not being an employee there.
(Note: that's just the beginning of things to come - the mandatory part, but not everything)

Jurij-Ivastsuk commented 5 months ago

@aronowski Hi, I think we can solve this problem differently. Please correct me if I am wrong. I could change the config file so that the two parameters (CONFIG_MODULE_SIG_ALL and CONFIG_MODULE_SIG_FORCE) are set to "Yes", recompile the kernel and all modules and then re-sign the kernel and all modules with our certificate. What do you think, will such a procedure solve the problem? Then you don't need to do any more research on these parameters.

aronowski commented 5 months ago

On 2024.04.02 09:51:18, Jurij wrote:

@aronowski Hi, I think we can solve this problem differently. Please correct me if I am wrong. I could change the config file so that the two parameters (CONFIG_MODULE_SIG_ALL and CONFIG_MODULE_SIG_FORCE) are set to "Yes", recompile the kernel and all modules and then re-sign the kernel and all modules with our certificate. What do you think, will such a procedure solve the problem? Then you don't need to do any more research on these parameters.

That might work! Ping me once that's done and I'll re-review the application.

-- Reply to this email directly or view it on GitHub: https://github.com/rhboot/shim-review/issues/362#issuecomment-2032566600 You are receiving this because you were mentioned.

Message ID: @.***>

Jurij-Ivastsuk commented 4 months ago

@aronowski Hello, we have done everything as announced in the previous message. How can we move forward?

aronowski commented 4 months ago

We've had a chat about this application during our last call. No need to do anything right now, I believe. It will be reviewed in the near future, just that patience is required.

THS-on commented 4 months ago

A few comments before looking at the at rest of the submission:

Can you include a description of your current kernel module signing setup in the submission itself? As you are not using ephemeral key signing and it's easier to read then the comments on this issue. Similarly as @aronowski suggested adding your kernel config and sources makes it easier to accept for us.
What kind of lockdown patches for the kernel are you using? Please add this to the submission
Regarding https://github.com/rhboot/shim/pull/626: I would like at least see a comment from a shim developer that this is fine. On first sight it looks ok, but any custom patch makes it harder for us to accept. Because this is a new vendor submission it might make sense to submit a vanilla shim first.
Regarding GRUB: I assume you then take the upstream binary and add your own signatures to it? Ideally rebuild it with your own SBAT entry added, as this allows easier revocation.

Jurij-Ivastsuk commented 4 months ago

@aronowski , @THS-on Thank you for the further review! Please see the current commit tagged as: waxar-shim-x86_64-aarch64-20240506

The description of our current kernel modul signing setup is described in the file Setup_for_module_signing.txt. Kernel sources you can find in archive linux-source-515.tar.gz. The config file .config_WRK_5_15_SIG_Yes and the config file .config in the surce are identical.
Unfortunately, the file with compressed kernel sources linux-source-515.tar.gz could not be pushed because the file size (193.46 MB) exceeds the GitHub limit of 100 MB. The error message: remote: error: File linux-source-515.tar.gz is 193.46 MB; this exceeds GitHub's file size limit of 100.00 MB
We are currently using the sources of linux kernel 5.15, the lockdown patches have been merged into the mainline Linux since kernel 5.4. For this reason, we have not applied any lockdown patches in our kernel version.
What concerns the patch #626 (Improving the robustness of value retention for the variable second_stage #626), I think the review by @Metabolix is exactly what you want to see. Please read the post from Feb 15 by @Metabolix.
What concerns GRUB: We use the latest official grubx64.efi file from Canonical, replace the SBAT section with a new one where our line is included and then sign this file additionally with our certificate. The SBAT section can be replaced with: objcopy --set-section-alignment '.sbat=512' --remove-section .sbat --add-section .sbat=ubuntu_waxar.csv --adjust-section-vma .sbat+10000000 grubx64.efi Therefore, we do not need to recompile the original grubx64.efi file from Canonical.

aronowski commented 4 months ago

Please see the current commit tagged as: waxar-shim-x86_64-aarch64-20240506

There's something wrong - I can't see any reference to such name.

Unfortunately, the file with compressed kernel sources linux-source-515.tar.gz could not be pushed because the file size (193.46 MB) exceeds the GitHub limit of 100 MB. The error message: remote: error: File linux-source-515.tar.gz is 193.46 MB; this exceeds GitHub's file size limit of 100.00 MB

Please, don't push such blobs directly if there's no need to showcase something strictly related to them, like their metadata (Source RPMs come to mind). I'd instead recommend creating a separate repository and pushing only the contents of that archive.

Jurij-Ivastsuk commented 4 months ago

@aronowski , @THS-on Hello, this is very strange, I can see this commit under this link: https://github.com/Jurij-Ivastsuk/WAXAR-shim-review/tree/waxar-shim-x86_64-aarch64-20240506

Please try again and let me know if it doesn't work for you. Actually, everyone can see this repository (at least public read access should work). As you suggested, I created a new repository and uploaded the Debian-Linux 5.15 kernel sources there. You can access them here: https://github.com/Jurij-Ivastsuk/Linux_kernel_source-5.15

I hope you have enough information. If there is anything else missing, please let me know.

Jurij-Ivastsuk commented 4 months ago

@aronowski , @THS-on Hi, yesterday together with @Metabolix friendly help we found the way how to make shim compilation without the patch I suggested earlier (Improving the robustness of value retention for the variable second_stage #626). For more information see the last posts under #626. I would suggest, as soon as I get information from you what else should be changed or adapted, I make the new compilation of Shim without applying patch #626, adapt all other areas like Docker-File, SHA256 sums accordingly, make any adjustments you may suggest and create a new commit that should contain all changes. Please let me know when you are ready.

Jurij-Ivastsuk commented 3 months ago

@aronowski , @THS-on Hello, I wanted to kindly ask if there is any progress in our case ?

aronowski commented 3 months ago

@Jurij-Ivastsuk,

Hello, I wanted to kindly ask if there is any progress in our case ?

I suppose it will be more helpful if I start raising both positive feedback, as well as potential issues immediately, rather than doing one big review. The latter is tough for several reasons, one of them being the lack of having a comfortable office with air conditioner - doing an analysis in over 30 degrees Celsius does not help at all.

The curiosity I noticed:

Next, we set the parameter: CONFIG_MODULE_SIG_KEY="certs/signing_key.pem" (see config file).
The string provides the path to a file containing both a private key and its corresponding X.509 certificate in PEM form.
For security reasons, these files are not included in current Sources (linux-source-515.tar.gz).

According to https://www.kernel.org/doc/html/v5.15/admin-guide/module-signing.html:

File name or PKCS#11 URI of module signing key (CONFIG_MODULE_SIG_KEY)

Setting this option to something other than its default of certs/signing_key.pem will disable the autogeneration of signing keys and allow the kernel modules to be signed with a key of your choosing.

Was this intentional? Is it possible to check the compiled modules through static analysis, if they were signed with the appropriate signing key?

Jurij-Ivastsuk commented 3 months ago

@aronowski Hello, and thank you for your questions. I can well understand that it is indeed difficult to work at temperatures above 30 degrees Celsius. I hope that the temperature conditions will be a little more bearable for you in the next few days.

To your questions, during the compilation with CONFIG_MODULE_SIG_ALL=y we noticed that if there are no pub- and priv-keys in the certs directory, they are not created automatically as the documentation says, no idea why. But if we generate both keys beforehand and then insert them into the certs directory, then all modules are signed. That is the reason for this procedure.

To check whether modules are actually signed we have 2 possibilities (as an example on the ext4.ko module):

We see a line in the building log: SIGN debian/linux-image/lib/modules/5.15.1-devimg-waxar/kernel/fs/ext4/ext4.ko
With the help of modinfo: modinfo ext4.ko filename: /usr/src/Test_Modules_Signature/ext4.ko softdep: pre: crypto-crc32c license: GPL description: Fourth Extended Filesystem author: Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others alias: fs-ext4 alias: ext3 alias: fs-ext3 alias: ext2 alias: fs-ext2 depends: mbcache,jbd2,crc16 retpoline: Y intree: Y name: ext4 vermagic: 5.15.1-devimg-waxar SMP mod_unload modversions sig_id: PKCS#7 signer: Build time autogenerated kernel key sig_key: 30:00:4F:89:A6:9D:0E:78:07:FB:35:C5:A8:07:17:E7:8E:C4:39:C1 sig_hashalgo: sha256 signature: 04:16:59:88:3C:30:C2:19:12:CB:74:DE:AF:B8:25:49:4A:EE:F0:6A: .....

I hope I have answered all your questions. If you have any further questions, please do not hesitate to contact me.

aronowski commented 3 months ago

@Jurij-Ivastsuk,

To your questions, during the compilation with CONFIG_MODULE_SIG_ALL=y we noticed that if there are no pub- and priv-keys in the certs directory, they are not created automatically as the documentation says, no idea why. But if we generate both keys beforehand and then insert them into the certs directory, then all modules are signed. That is the reason for this procedure.

As far as I can see the process does seem to work like the kernel documentation says. The modinfo listing says:

[...]
signer: Build time autogenerated kernel key
[...]

This matches the process described in the "Generating signing keys" section of the v5.15 kernel documentation. In particular the x509.genkey file should be generated by your certs/Makefile.

For extra safety I'd also check your kernel's keyring on a booted system. Please see, if anything is printed by running:

# grep "Build time autogenerated kernel key" /proc/keys

on the current compilation of your system. If the same happens, then the usage of an ephemeral key is actually present here, most likely accidentally, as your shim application says otherwise (No worries - things like that may happen and that's why the peer-review process is here to help).

What I'd also recommend is to edit the kernel's config file, change the current line:

CONFIG_MODULE_SIG_KEY="certs/signing_key.pem"

to something like

CONFIG_MODULE_SIG_KEY="certs/waxar_signing_key.pem"

and try again by renaming the file with your private key and its corresponding certificate to waxar_signing_key.pem. Once the kernel's compiled, see if modinfo prints something different, as intended in the application.

Jurij-Ivastsuk commented 3 months ago

@aronowski Hi, we have checked your suggestions: # grep "Build time autogenerated kernel key" /proc/keys there is no output, the string does not exist

After new compilation we have checked the same module (modinfo ext4.ko) [...] signer: Build time autogenerated kernel key [...] No changes in this line.

Jurij-Ivastsuk commented 3 months ago

@aronowski Hello, I have now created a commit with the current Shim version 15.8. I have compiled this Shim variant without using the improvement I suggested earlier (Improving the robustness of value retention for the variable second_stage #626). The main change: in this variant I use the parameter DISABLE_REMOVABLE_LOAD_OPTIONS for compiling (see https://github.com/rhboot/shim/blob/main/BUILDING). With this parameter I can solve my problem without using the patch #626.

Changes in this commit:

current link to the current tag: waxar-shim-x86_64-aarch64-20240531
current SHA256 hashes for both files shimx64.efi and shimaa64.efi
Dockerfile now reflects the changes in the building process
build.log file now reflects the changes in the building process
both files shimx64.efi and shimaa64.efi are now up to date

You can access this commit under the following link: https://github.com/Jurij-Ivastsuk/WAXAR-shim-review/tree/waxar-shim-x86_64-aarch64-20240531

Jurij-Ivastsuk commented 3 months ago

@aronowski Please don't be surprised, I have forgotten something:

Inserted changes and adjustments to the README.md file to reflect the latest changes in relation to recompiling without patches

Now all the documentation is complete... I think :-) You can access this commit under the following link: https://github.com/Jurij-Ivastsuk/WAXAR-shim-review/tree/waxar-shim-x86_64-aarch64-20240601

aronowski commented 3 months ago

@Jurij-Ivastsuk, I'll take a look at the updated shim no sooner than this Sunday. Please let me know everything I need to know by then.

Jurij-Ivastsuk commented 3 months ago

@aronowski Hello, I'm sure you are responsible for many application reviews and each application has its own special features. Therefore, in order to simplify things and time savings for you with our application, so that you have a better overview, I am writing a short summary of what we have done together so far.

We have checked email contacts they are OK
We have created a new certificate with 4096 bit and a validity period of 50 years (instead of 2048 bit, 100 years) and implemented it for the compilation of the current Shim version (15.8) and signing of our boot components. You have checked this certificate and it was OK
I had quite a long discussion with @Metabolix about the usefulness of the PR 626 (https://github.com/rhboot/shim/pull/626). In the end, although the solution we proposed in PR 626 turned out to be viable, we found a solution that solves our problem and does not change the code (using the parameter DISABLE_REMOVABLE_LOAD_OPTIONS=y). So I think using this parameter will make the review process easier for you, because there are no more code changes to discuss.
According to your recommendation, we have changed the config file for kernel compilation and activated the parameters CONFIG_MODULE_SIG_ALL and CONFIG_MODULE_SIG_FORCE. So that the modules are automatically signed with a certificate during kernel compilation. This increases the security that the kernel cannot load any foreign modules. We can also prove that all modules are signed accordingly after kernel compilation.

All changes and adjustments are now included in the last commit (https://github.com/Jurij-Ivastsuk/WAXAR-shim-review/tree/waxar-shim-x86_64-aarch64-20240601) Please let me know if something is missing.

THS-on commented 3 months ago

By just looking at the provided kernel config and setup description this contradicts the statement made in the README. If you have the private key in a file, it is not secured by an HSM. Please clarify the following:

How are your signing keys stored for the kernel modules?
Why are you not able to use ephemeral key signing?
How do you ensure that kernel modules from older kernels cannot be loaded on newer ones?

@aronowski if I see it correctly the cert is only newly generated if the x509.genkey changes. See https://github.com/torvalds/linux/blob/83a7eefedc9b56fe7bfeff13b6c7356688ffa670/certs/Makefile#L52-L53

rhboot / shim-review

shim-15.8 for WAXAR #362

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

What is the SHA256 hash of your final SHIM binary?

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