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: disable module signing #107625

Merged
merged 1 commit into from Dec 30, 2020

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Dec 26, 2020

Motivation for this change

https://r13y.com

Closes #107524

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Without this, the kernel would generate a random one for us which obviously
isn't reproducible.

`nix-build -A linux --check` succeeds now!
(Tested at different times with different kernel)
@Atemu
Copy link
Member Author

Atemu commented Dec 26, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Dec 26, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@Izorkin
Copy link
Contributor

Izorkin commented Dec 26, 2020

This PR breaks work the lockdown module:

boot.kernelParams = [ "lockdown=confidentiality" ];

@Atemu
Copy link
Member Author

Atemu commented Dec 26, 2020

Intentionally so. Without enforcing signatures or otherwise preventing root from loading modules, lockdown is pretty pointless AFAICT.

@ajs124
Copy link
Member

ajs124 commented Dec 26, 2020

I thought lockdown does enforce signatures? This says that, at least.

@raboof
Copy link
Member

raboof commented Dec 27, 2020

I thought lockdown does enforce signatures?

Correct, it does. The lockdown feature depends on module signatures being enabled, because without it you could circumvent the lockdown protections (by loading a custom module that performs the protected actions).

Copy link
Member

@raboof raboof 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 in favor of this change.

The lockdown feature is a feature that restricts root from performing certain actions. On a 'regular' NixOS machine that does not make much sense, since root could just build and reboot into a kernel that does allow the desired operations.

To make effective use of the lockdown feature, you'd build a hardened image with just the things you need and make sure root cannot reboot into different kernels, using for example EFI Secure Boot (or other mechanisms for other deployment scenario's). Doing that correctly requires some reading and configuration work - if you're going that far anyway, it would be a good idea to customize the kernel build configuration and only include the features you want to be available for that particular use case (either baking them into the kernel directly and not using modules at all, or using module signatures).

I think it's reasonable to have module signatures and lockdown disabled for 'regular' NixOS installations - I think the advantage of having reproducible kernel images outweighs the disadvantage of having to explicitly enable the feature when you need it. We should probably mention it in the release notes - ideally we could point to some documentation on how to effectively use lockdown, though the latter is probably beyond the scope of this PR.

@raboof
Copy link
Member

raboof commented Dec 27, 2020

Another data point: while Debian and Arch I think by default do include module signatures, OpenWRT does not (https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/config-4.14;h=d54ede9efda0a3ffd84e9a0c49dc410a01737d82;hb=refs/tags/v19.07.5#l2736).

@Atemu
Copy link
Member Author

Atemu commented Dec 27, 2020

OpenWRT doesn't disable module signatures but simply doesn't set the option. Pretty sure it defaults to yes.

@raboof
Copy link
Member

raboof commented Dec 27, 2020

OpenWRT doesn't disable module signatures but simply doesn't set the option.

Good point

Pretty sure it defaults to yes.

I don't think so: I don't see a default in https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/init/Kconfig?h=v5.4.85#n2016, and https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt suggests in that case the default is 'n'. If you enable SECURITY_LOCKDOWN_LSM then https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/security/lockdown/Kconfig?h=v5.4.85#n4 automatically also enables MODULE_SIG, though.

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

I'd love to see some people tinkering with this feature for their own locked down systems (building their own kernels with their own keys), but it probably doesn't make much sense to default to a nonreproducible key in nixpkgs. 👍

@kevincox
Copy link
Contributor

I see that we have other reviewers and some of them have committer rights. As I don't have expertise in this area I'm going to leave it to them.

@kevincox kevincox removed their request for review December 30, 2020 13:12
@7c6f434c
Copy link
Member

Yeah, given that most systems presumably fetch a kernel from Hydra and so use a random but same key…

@7c6f434c 7c6f434c merged commit a95d8f1 into NixOS:staging Dec 30, 2020
@Atemu Atemu deleted the r13y/kernel-disable-module-signing branch December 30, 2020 18:16
@r-burns
Copy link
Contributor

r-burns commented Jan 11, 2021

Does this require kernel 5.4? I think this has broken linux-config for older kernels.
https://www.phoronix.com/scan.php?page=news_item&px=Linux-5.4-Adds-Lockdown

@Atemu
Copy link
Member Author

Atemu commented Jan 11, 2021

It does indeed. I don't know why you'd want to use such an ancient kernel but we shouldn't break that. Fix above.

NeQuissimus pushed a commit that referenced this pull request Jan 11, 2021
kernel: disable module signing
(cherry picked from commit a95d8f1)
eadwu added a commit to eadwu/flakes that referenced this pull request Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants