Skip to content
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

kernel : enable module signing #3116

Merged
merged 1 commit into from
May 23, 2023

Conversation

shjala
Copy link
Member

@shjala shjala commented Mar 23, 2023

Enable build time module signing and enforce loading only signed modules at run-time.

I will send another PR for new-kernel.

@shjala shjala requested review from eriknordmark and rvs as code owners March 23, 2023 12:15
@shjala shjala force-pushed the pkg_kernel_module_sig branch 3 times, most recently from 45ba090 to 01db481 Compare March 23, 2023 13:21
@rene
Copy link
Contributor

rene commented Mar 23, 2023

@shjala , your approach let to the kernel build system generate the keys. But I think we should allow (and simplify) the usage of custom keys. You could put pre-generated keys under pkg/kernel/certs and use them. I think it makes more easy to use customized keys. What do you think?

@christoph-zededa
Copy link
Contributor

Can you change the commit message?
I see:

Signed-off-by: Your Name <you@example.com>

@shjala shjala force-pushed the pkg_kernel_module_sig branch from 01db481 to 8a40837 Compare March 23, 2023 14:01
@shjala
Copy link
Member Author

shjala commented Mar 23, 2023

@rene I can add a cp /certs/signing_key.pem /linux/certs/, is that what you mean?

@rene
Copy link
Contributor

rene commented Mar 23, 2023

@rene I can add a cp /certs/signing_key.pem /linux/certs/, is that what you mean?

You need to generate the cert file and pass its path to CONFIG_MODULE_SIG_KEY. You can use custom location if you want.

@shjala
Copy link
Member Author

shjala commented Mar 23, 2023

You need to generate the cert file and pass its path to CONFIG_MODULE_SIG_KEY. You can use custom location if you want.

Yeah sure, but if we just copy the custom generated key to certs/signing_key.pem then it is simple and clean and the default case, so no need for extra doc:

the kernel build will automatically generate a new keypair using openssl if one does not exist in the file:
certs/signing_key.pem

@rene
Copy link
Contributor

rene commented Mar 23, 2023

You mean, copy the generated key to the final container's image? Well, that's an approach as well. But I'm wondering that some manufacturer(s) (really concerned about security) will eventually want to use his(their) own key, that's why I asked what do you think.... but for me copy the generated key it's also fine...

@shjala
Copy link
Member Author

shjala commented Mar 23, 2023

@rene The current way is good because it throws the key away after build, no chance of leaking it and signing malicious modules. But what I mean by cp .. is that you can put your custom key in eve/pkg/kernel/cert/signing_key.pem, then kernel uses that to sign the modules, will this satisfy what you are looking for?

@rene
Copy link
Contributor

rene commented Mar 23, 2023

@rene The current way is good because it throws the key away after build, no chance of leaking it and signing malicious modules. But what I mean by cp .. is that you can put your custom key in eve/pkg/kernel/cert/signing_key.pem, then kernel uses that to sign the modules, will this satisfy what you are looking for?

Got it, yes, looks good this way.

@shjala
Copy link
Member Author

shjala commented Mar 23, 2023

@rene 👍, I will add necessary changes.

@shjala shjala force-pushed the pkg_kernel_module_sig branch 7 times, most recently from 6d0e9d9 to 5023e4b Compare March 24, 2023 14:19
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tryig to understand the implications of the throwaway key, but I don't see the code which generates it and uses it, so I need some help.

My concern is around buiding the same release of EVE-OS (x.y.x-kvm-amd64) mutiple times and whether or not that would result in different bits (hence different sha256) for the kernel?

Separately, once we nailed this down for pkg/kernel we should also apply it to pkg/new-kernel.

@shjala
Copy link
Member Author

shjala commented Mar 27, 2023

@eriknordmark

I'm tryig to understand the implications of the throwaway key, but I don't see the code which generates it and uses it, so I need some help.

The kernel build process will generate it, to be exact here, then if the kernel configs for automatic signing are set, during make modules_install sign-file.c is called for each module, key is passed as argument.

My concern is around buiding the same release of EVE-OS (x.y.x-kvm-amd64) mutiple times and whether or not that would result in different bits (hence different sha256) for the kernel?

yes it will be different, will that be a problem? my understanding is that we have one official release per-version, right?

Separately, once we nailed this down for pkg/kernel we should also apply it to pkg/new-kernel.

sure, that's the plan.

@eriknordmark
Copy link
Contributor

yes it will be different, will that be a problem? my understanding is that we have one official release per-version, right?

Today we build using the github runners and publish the resulting artificates on the releases page.
But it might make sense to separately run a build from source on more controller, trusted and isolated build servers, and compare that this produces identical bits for all of the artficates.

Also, as we do a patch (to the pkg/foo source somewhere) and build the x.y.1 patch release it would make sense to be able to do a binary comparison with the artifacts produced for x.y.0 to make sure the right components changed.

Both of those would be problematic with the random module signing key.

I wonder about the threat model and whether a fixed module signing key (stored in github) would be as effective as a random one. Let's discuss offline or on the EVE call?

@shjala
Copy link
Member Author

shjala commented Mar 27, 2023

Let's discuss offline or on the EVE call?

sure.

@shjala shjala marked this pull request as draft March 28, 2023 07:06
@shjala
Copy link
Member Author

shjala commented Mar 28, 2023

Both of those would be problematic with the random module signing key.

There are two artifacts that are affected, one is kernel itself (public key is inserted in the kernel key ring during build) and second one are the modules. Signature is appended at the end of each module file, and stripping the module will remove it so still possible to do a byte by byte binary comparison of individual modules.

For kernel I think pinpointing the embedded key offset and size and skipping over that part for binary comparison is possible.

@shjala shjala force-pushed the pkg_kernel_module_sig branch from 5023e4b to 0a210a6 Compare March 29, 2023 10:49
@shjala shjala force-pushed the pkg_kernel_module_sig branch from d2e2a5a to b57fde0 Compare March 29, 2023 14:11
@shjala shjala marked this pull request as ready for review March 29, 2023 15:53
@shjala shjala requested a review from eriknordmark March 30, 2023 09:16
@eriknordmark
Copy link
Contributor

Both of those would be problematic with the random module signing key.

There are two artifacts that are affected, one is kernel itself (public key is inserted in the kernel key ring during build) and second one are the modules. Signature is appended at the end of each module file, and stripping the module will remove it so still possible to do a byte by byte binary comparison of individual modules.

For kernel I think pinpointing the embedded key offset and size and skipping over that part for binary comparison is possible.

Given that two months ago or so I had to compare two different EVE images/builds on short order, I'd like to see the tools to do such comparison with the above ignores as part of this PR, otherwise we will not be able to do such emergency comparisons.

@shjala
Copy link
Member Author

shjala commented Apr 11, 2023

I did binary diff between two consecutive (identical) builds of the kernel from master:

shah@dev:~/eve/pkg/kernel$ docker build --no-cache --file Dockerfile --output out1 .
shah@dev:~/eve/pkg/kernel$ docker build --no-cache --file Dockerfile --output out2 .

this is the result on unziped kernel images :

shah@dev:~/eve/pkg/kernel$ diff out1/kernel out2/kernel
Binary files out1/kernel and out2/kernel differ

visual diff also shows lot of differences :
image

there is two much difference and hard to pinpoint what is what (to skip over).

@eriknordmark how did you performed the image comparison before?

@shjala
Copy link
Member Author

shjala commented Apr 11, 2023

I also did a binary diff on two builds from master and two builds from shjala:pkg_kernel_module_sig, both showed no difference at function level, but differ in byte comparison.

IMHO, it makes sense to build the release versions in a secure environment and upload them to Github.

@shjala
Copy link
Member Author

shjala commented Apr 11, 2023

The last option to enforce some policy on module loading is using the LoadPin LSM, to for example enforce loading of kernel modules only form RO rootfs, but EVE is not using something like dm-verity and root can remount the rootfs as RW.

@shjala shjala force-pushed the pkg_kernel_module_sig branch 5 times, most recently from 233fadc to 955afda Compare May 16, 2023 13:17
@shjala
Copy link
Member Author

shjala commented May 16, 2023

@eriknordmark added the diff script.

@shjala shjala force-pushed the pkg_kernel_module_sig branch 2 times, most recently from 07425be to 8da98ff Compare May 16, 2023 15:44
@shjala
Copy link
Member Author

shjala commented May 17, 2023

I will fix the complains once the script is reviewed.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document the example usage and output from the rootfs-diff script (in the BUILD.md I assume)?

@shjala shjala force-pushed the pkg_kernel_module_sig branch from 8da98ff to dcc9d5e Compare May 19, 2023 08:38
@shjala shjala requested a review from eriknordmark May 19, 2023 08:40
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some yetus issues (but ignore any ones about "ro" being a typo if you have those).

Enable build time module signing and enforce loading only signed
modules at run-time.

Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
@shjala shjala force-pushed the pkg_kernel_module_sig branch from dcc9d5e to a2bd471 Compare May 22, 2023 11:42
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eriknordmark eriknordmark merged commit b990512 into lf-edge:master May 23, 2023
@shjala shjala deleted the pkg_kernel_module_sig branch June 8, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants