-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
shim-15.8 for WAXAR #362
Comments
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." |
@aronowski Hi, many thanks for any tips! |
@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. |
@aronowski, I understand and I thank you for the work you do in your spare time! |
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 There's no ephemeral key - is the strategy sufficient, as it's described right now? It would be preferable to mention the usage of The CA certificate 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? |
Verification emails sent. |
I have received your email. Here is the decrypted text: |
@aronowski Hi, thank you very much for the first hints!
|
@aronowski Hi, should we now switch to 15.8 ? |
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. |
@aronowski Hi, |
@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? |
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).
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.
4096 byte? ;-) |
@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... ;-))) |
@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? |
@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. |
@aronowski Hi, please only continue with review if you have slept enough. i have made all adjustments for version 15.8 ;-)) |
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 #368 (comment).
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 Please don't be surprised, I have forgotten something:
Now all the documentation is complete... I think :-) |
@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. |
@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.
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) |
By just looking at the provided kernel config and setup description this contradicts the statement made in the README.
@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 |
@THS-on , @aronowski Hi, thank you for questions.
|
@Jurij-Ivastsuk Simply deleting old kernel modules from a system is not enough for revocation. You need to have technical measures in place to block them from loading. There's a few issues here where it's not clear from your submission that you understand how things are meant to work. |
@THS-on , @aronowski
I can't say that I know everything, but unfortunately I can't do anything with this statement of yours. I would be very happy to learn something new from you. If you think you know something more in this context, please share your knowledge with everyone here who is in a technically similar situation and would like to improve their solutions. |
Hello dear reviewer @aronowski @THS-on @steve-mcintyre, to answer once again your question about the prevention of loading unwanted modules: we use our own certificate to check the signatures of the modules to be loaded. Our kernel can also be delivered with new keys, if we need it, to prevent old modules from being loaded. |
I'll raise the issue during the next call.
I myself was thinking about writing a guide, how to start the shim-related venue from scratch and implement appropriate measures for reasonable security, but taking into account each company's distinct situation, but there's no way I'll be able to account for so many different cases and limitations. Especially doing this in a free time after job hours and considering my own knowledge, experience and limitations. I could think of something similar to the venue from this thread about smaller companies (one referred there as a fictional MyCompany) and their products deriving from the larger ones, but in the context of shim implementation and the whole bootloader chain (referred as Using downstream implementations from Canonical in your application). Still, as said earlier, I can see too many distinct cases to make it reasonable. |
@Jurij-Ivastsuk regarding the kernel module signing:
|
@THS-on Hello, thank you very much for your comments. In our current kernel-mudule signing-setup we currently use two different certificates: 1. our own CA certificate and 2. kernel specific certificate for each compilation. |
Can you update your documentation to reflect that? And also describe how your procedure with those keys are in detail?
This refers to your third point from this comment #362 (comment) |
Hello @THS-on In order to implement the realization of the “completely secure boot” principle, we have developed an approach with double signing of certain boot components using HSM. We combine the signing of the kernel and modules once with our CA certificate (for kernel) and additionally with unique private key + CA certificate (for modules). This approach for signing the modules is more secure compared to signing only with a unique private key. Additional use of HSM gives us an even higher protection for security because both private keys are only in HSM and never appear as physical files. We have adapted and updated our setup documentation in line with your comments. Please see here: (https://github.com/Jurij-Ivastsuk/WAXAR-shim-review/blob/waxar-shim-x86_64-aarch64-20240804/Setup_for_module_signing.txt) |
Thanks for the update. Some questions:
|
The kernel only allows and validates a single signature at the end of the module. So, it's not valid to say that modules are signed with both a unique key and your CA certificate's key. Furthermore, it's more secure to sign modules only with the ephemeral key generated during the kernel build. That way, only modules built for that particular kernel build can be loaded. It ensures that you can't take a vulnerable module from an older kernel build and load it with a current kernel. In that scheme, modules only need to be signed with the ephemeral key during the kernel build. There's no need to keep the ephemeral key or re-sign the modules later. If you did sign the modules with your CA key, then any module ever signed by you would be valid for loading in the current kernel. The only place this causes an issue is with out of tree modules like the nvidia drivers. In that case, you obviously need to sign them outside of the kernel build. This is what Following the standard process ( |
Hello @dbnicholson, |
Hello @THS-on,
|
Hi @Jurij-Ivastsuk, I'm trying to understand why this is a blocker. Quoting from that comment:
In this comment it sounds like you're doing exactly what I suggested. An ephemeral key is created during the kernel build and the private key is thrown away after the build. Unless you're following a different procedure, this is what's stored in The in tree modules are already signed with the ephemeral key and will be validated against it with the public certificate stored in |
Hi @dbnicholson, Because some reviewers (#362 (comment)) have pointed out that you have to create a procedure to prevent old modules from being loaded, we have proposed an idea with double signing. Although, if the ephemeral key is already in use, this measure is completely unnecessary. The only point the reviewer @THS-on pointed out was the use of HSM when implementing automatic signing during kernel build. Because the argument that the signature key is physically accessible for a certain time during the kernel build, I think his argument is perfectly fine. After all, you want to have the highest possible security so that you can prevent other perhaps unwanted modules from being loaded. We have implemented HSM for the creation of ephemeral keys (https://github.com/Jurij-Ivastsuk/WAXAR-shim-review/blob/waxar-shim-x86_64-aarch64-20240804/Setup_for_module_signing.txt). So, now we hope that all security requirements are fulfilled by us and we would be happy that the review process can be decided positively for us at this point. |
@Jurij-Ivastsuk Thanks very much for that explanation. So, the purpose of the HSM within the kernel build is to generate the key and do the signing rather than doing it all with a key generated on disk from the OS random source? That is certainly more secure. However, this part seems to undo all of that:
If you sign the modules with your CA certificate, then you lose the ability to prevent old modules from being loaded since all modules from every kernel build will be valid. Furthermore, the linux module signature verification process only validates a single signature. It does not support having multiple signatures on a module. Looking at the validation process, it simply copies a single signature of bytes from the end of the module for verification. So, it's only validating the last signature, which is your CA signature. The ephemeral key signature isn't being used at all. I feel like you should just drop this step. You just said "Although, if the ephemeral key is already in use, this measure is completely unnecessary." The only thing this step is going to do is decrease security. |
Hi @dbnicholson, |
Hi @THS-on, @aronowski and @dbnicholson, according to our last discussions with @dbnicholson, the area concerning the double signing of the modules has been completely removed from our signing setup. Please have a look at the updated version of our setup here: |
The updated setup description seems reasonable at a first glance, but nevertheless I'll need to review the application again, considering how many changes there have been in the meantime. Should be easier, considering that some reviewer questions have been answered here. Also, pinging others for further help - if you noticed some missing details or curiosities that need clarification from the applicants' side, please point them out. |
Review as per The tag referred in the opening post is still
This question is not about the review application URL, please correct it.
OK: the "626" patch seems to be no longer used, although it still resides in the application's repository and the Hint: since I noticed some other errors, so that the application will have to be corrected, you can remove that folder, so it doesn't cause further confusion in case of another re-review.
OK: all the issues mitigated by those upstream patches are mitigated and explained as per #368 (the same kernel from the accepted application is reused).
This is a bigger thing, that got it's own description and there have been some discussions to help with it. It seems OK and thoroughly described, but nevertheless, I wanted to try it out for myself:
Is this an issue regarding SoftHSM or is there something else going on? Decomposing this command into two steps, i.e. dumping the object into a file first, and then using this file directly with As far as I can see, the custom I'll need to take another look, once these are clarified.
I can see this entry being from January 15, 2024, so in the meantime it should have been updated to
In case of reusing the same modules as the upstream release, i.e.:
Please, provide the list of them here.
If the setup just reuses upstream mechanisms, please describe them here.
Please see #362 (comment), otherwise things might get confusing especially for those, who work with different family of distros. |
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/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
The text was updated successfully, but these errors were encountered: