Not sure why GH thinks @raboof's commit I based off of isn't in staging already but it's the same one that was merged yesterday.

/marvin opt-in
/status needs_reviewer

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.

Oi, gimme that label back @ofborg.

Looks like the commit was rebased, not merged; rebased.

I'm not sure I'm in favor of this. I get the reproducibility aspect, but now if someone turns on signature enforcement in their config (e.g. with linuxManualConfig) I'd say it's a reasonable assumption that this would lead to a kernel for which you cannot build modules later and load them. As in, you can only load modules from that kernel build.

As is, this assumption would not be true anymore, in fact anyone could build modules and load them.

Maybe this could be conditional on the relevant signature enforcement options? IIRC the kernel build already uses the module system to do that kind of config matching magic in other places.

If your security relies on the kernel build generating a ephemeral key during build, you should enforce its ephermerality rather than assuming it IMO. Perhaps even build your config around CONFIG_MODULE=n.
Also, if you do something custom like that, you're on your own. I can't make sure people's manual configs don't break as an upstream contributor. I can only be expected to make sure Nixpkgs doesn't break.
Might be worth including in the release notes though.

Almost nothing in pkgs uses the module system, linuxManualConfig is passed an attrset of arguments, i.e. features. Most of the kernel config only depends on version and platform with only very few options decided by features' attrs. I could add something there but I think that's meant for big upstream features like Xen or grsecurity.

Note: This does not improve or worsen kernel security since we don't enforce signatures

I don't think this is strictly speaking true, in theory: we enable SECURITY_LOCKDOWN_LSM by default, and users could be doing "echo 'confidentiality' > /sys/kernel/security/lockdown". Without this PR, this would protect against loading malicious kernel modules. With this PR, it would no longer protect against it, since the attacker could use the private key from the repo to sign their malicious modules.

That said, I think it is also somewhat unlikely people are actually using SECURITY_LOCKDOWN_LSM. Specifically, I think people are unlikely to use SECURITY_LOCKDOWN_LSM on 'regular' NixOS machines: since this feature is intended to protect you even when the attacker has gained root, I think this is a feature you would use on machines where you don't build new kernels on the machine itself, but you'd generate images with something like nixos-generators. In that context you'd probably want to edit your kernel config to only ship the modules you want to be available (and possibly use SECURITY_LOCKDOWN_LSM to protect against loading others, with a private key that doesn't leave the machine that builds images), or indeed just CONFIG_MODULE=n them.

Given this, I think it would be better to disable MODULE_SIG and SECURITY_LOCKDOWN_LSM by default, instead of including a "publicly-known private key" in the repo. I suspect there will be few users actually using the SECURITY_LOCKDOWN_LSM features, and for those who do want it, we can document how to lock down things even better when using Nix. Including a "publicly-known private key" would be surprising for those users, and because of that a dangerous default.

Marvin pinged me, but I don't think this is really ready for review and/or merging, since the discussion around the PR and its consequences is not done yet.

I like the fact that the key explicitly says to not trust it, but should we even introduce potential security pitfalls (as described in earlier comments to this PR) as a sacrifice for reproducibility?

@Atemu does this configuration even boot with signature enforcement? Can an out-of-tree built-by-Nix module (e.g. openrazer drivers) be loaded? Will these modules be signed too? I think this PR needs lots of documentation before it's ready to merge, and before that, I can't say that I would merge this if I had rights to merge. I'll set "awaiting changes" so Marvin wouldn't bother us until the discussion is complete.

/status awaiting_changes

By default, there is no security issue here. The kernels built by hydra would be just as insecure as before.
The only potential security issue here exists for people who manually customise and build their own kernels with signature enforcement and rely on the ephermerality of the generated key.
As I said though, it is questionable whether we as upstream should have to worry about breaking such custom configs.

Out of tree module loading should not have changed. I didn't check but logic dictates that, previously, you should not have been able to load OOT modules with signature enforcement because the key required by the kernel was generated during its build process and deleted by the nix-daemon afterwards.
It would theoretically be possible now actually because the key is known and could be passed to the OOT modules' derivations but since enabling signature enforcement wouldn't provide a security benefit anymore, I don't count that.

I'll check if disabling module signatures completely is possible as that is indeed a cleaner solution, builds are running...

It works!

I had to disable SECURITY_LOCKDOWN_LSM though, did that do anything on our kernels?

Yes it did lock a few things down but, without enforced signatures, you could get access to those anyways because you could insert an arbitrary module so I don't think we'd lose anything.

#107625

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

I think this is supposed to be closed now, according to the new PR mentioned above, therefore closing.

I think it would've gotten closed when staging made it into master.