nixos/fcgiwrap: refactor to fix permissions

[pacien]

Description of changes

This PR fixes mulitple issues with the fcgiwrap module:

• It's now possible to run multiple instances of fcgiwrap,
  solving the interference issue between its consumer modules.
• The service and the spawned scripts no longer run as root by default.
• The UNIX socket is no longer exposed to everyone by default.

With the previous defaults in the module,
any local user could execute scripts as root through the socket when any of
services.{fcgiwrap,cgit,smokeping}.enable = true was set.
The smokeping and cgit services were indeed partially and fully running as
root. This has also been fixed in this PR.

This changes the exposed options quite a bit, making this change backward
incompatible. The security benefits might however make it worth backporting.

See indidivual commit messages and modified release notes for details.

I tested these changes by running the NixOS tests of some consumers of this
module: nix build .#nixosTests.{smokeping,zoneminder,gitolite-fcgiwrap,cgit}

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[pacien]

(Rebased to fix merge conflict).

[LeSuisse]

The approach looks sensible to me and I'm also find with the changes.

Not quite sure how to handle it on stable :/

[minijackson]

Changes looks quite good, and quite useful. Thanks! I'm interested in running cgit as non-root.

[LeSuisse]

Moving to merge this, flagging for backport to 24.05 so we can discuss how we want to handle it.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/can-we-please-stop-breaking-stuff-willy-nilly/48496/1

[bendlas]

On a way forward: I do see value in making fcgiwrap multi-instance, so I wouldn't like to have to outright revert all of this.

How about we introduce the multi-instance options as maybe services.fcgiwraps.<name> and deprecate services.fcgiwrap, forwarding it to services.fcgiwraps.default?

[mweinelt]

I'm interested in running cgit as non-root.

That should have never been a thing. Thanks for cleaning that up.

• removing options is a lot more than just "fixing permissions"
• making a service multi-instance is a lot more than just "fixing permissions"

I agree that the scope of this PR goes far beyond removing the obvious footgun and can't be backported in full.

flagging for backport to 24.05 so we can discuss how we want to handle it.

But that was also an open discussion question.

keep the old options, and have an instance called default, to which the previous options map

I think doing breaking changes to module interfaces is reasonable on unstable, and ideally we'd have escape hatches for functionality that was covered by removed options.

think about our users. we have production deployments, depending on this. nobody cares if you think that this is better and we can just update our config. this is not NPM and many of us moved into nixos, exactly to escape the never-ending churn.

No, you don't get to make this "about our users", if your production deployments run on NixOS Unstable. You will have to deal with breaking changes that we usually announce in release notes every once in a while. Of course that'll hit you faster on NixOS Unstable.

How about we introduce the multi-instance options as maybe services.fcgiwraps. and deprecate services.fcgiwrap, forwarding it to services.fcgiwraps.default?

I think that is undesirable.

[bendlas]

No, you don't get to make this "about our users", if your production deployments run on NixOS Unstable. You will have to deal with breaking changes that we usually announce in release notes every once in a while. Of course that'll hit you faster on NixOS Unstable.

I do get to, I did, and I think the whole community should do more of that.

But speaking of that: I do see a release note update, but where are the upgrade instructions in that?

How about we introduce the multi-instance options as maybe services.fcgiwraps. and deprecate services.fcgiwrap, forwarding it to services.fcgiwraps.default?

I think that is undesirable.

How so?

[LeSuisse]

Hello,

Breaking options on a stable release is absolutely unacceptable. I'd usually not blame the author in this, but since @pacien is also a Member, you should also be held to a higher standard.

Thank you for the feedback and I can understand the frustration.

To be clear, I merged this because:

• I did not see a way to fix the footgun/LPE introduced by the existing module in a backward compatible way
• the bundle with the support of multi-instances made sense and something desirable, especially since users are going to need to check/adjust their config for the first bullet point anyway
• I did not have the intention to merge it as is in stable but it is easier to have a discussion on what we do and how we can do it if we have something concrete to discuss on

[bendlas]

• I did not see a way to fix the footgun/LPE introduced by the existing module in a backward compatible way

That's what I don't understand: Are you putting "tightening the socket permissions" on the same level of incompatibility as "changing nixos options in an incompatible way"? If so, is there a way to convince you that those aren't even in the same ballpark of breakage?

Or is there another security enhancement that I'm failing to see?

• the bundle with the support of multi-instances made sense and something desirable, especially since users are going to need to check/adjust their config for the first bullet point anyway

Most wouldn't, because the only thing accessing the fcgiwrap socket will be nginx in many cases.

Besides that, I do support the multi-instance thing. And I'm still wondering what @mweinelt has against pulling up a new hierarchy and mapping the old one to that.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/can-we-please-stop-breaking-stuff-willy-nilly/48496/20

[LeSuisse]

It has been an hectic week and I'm too tired to work efficiently on this tonight. I will get back to it tomorrow to see how we improve the migration phase/improve the warnings and provide more context around the whole thing.

Since I saw you were looking for my username on Discourse, it's @tgerbet (and I'm @tgerbet:matrix.org in the Matrix rooms if you want to reach me)

[bendlas]

@LeSuisse thanks for the olive branch and for staying level-headed.

I'd like ask your forgiveness (as well as other participants in this PR) for me escalating this immediately and putting your nose to the grindstone to that degree.

In a sense, it was just unlucky for this particular PR to be the one where I've finally had enough, and me getting that "loud" wasn't exactly deserved for what happened here.

I'm 100% willing to help sort this out in a productive and constructive way (including just letting it be), if you still want my help after this.

[pacien]

Hello,

I believe some clarifications are needed.
Here's a brief summary bringing more context to this pull request.

Purposes of these changes, security aspect

The fcgiwrap module had the following flaws:

1. The fcgiwrap daemon and all scripts it spawned were executed as a single
   user (happening to be root by default).

2. Its control socket was not protected, allowing any user to make the fcgiwrap
   daemon execute any program. The user/group options were also not doing as
   described: they were setting the forking user instead of restricting who had
   access to the control socket.

While already problematic when taken separately, both of these flaws combined
allowed any local user to execute any program as root on the machine.

Hence the security aspect of this pull request with the matching tag, and open
discussions on finding a possibly partial, non-disruptive, mitigation that
could be backported to the stable channel.

(I also reached out privately to a member of the security team prior to posting
this pull request, to confirm whether it was acceptable to post this in
public.)

On the API breakage / refactor

The fcgiwrap options are consumed by several other modules within nixpkgs (such
as cgit, smokeping, etc).

Those modules were able to share the same instance "thanks to" the flaws listed
above (1. running as root allowing to ignore file ownership, and 2. everyone
being able to use the control socket).

As such, changing the default user and/or fixing the control socket ownership
and permissions would have broken those consumer modules.

Taking into account those constraints, it was not possible to continue sharing
a single global fcgiwrap instance without the issues above.

(It is to be noted that the sourchut module was already avoiding this issues by
definig its own internal instance, avoiding the fcgiwrap module altogether.)

Breaking API changes to support isolated instances were therefore introduced
to the fcgiwrap module so that other consumer modules would remain usable.

Migration handling

An implicit mapping from the previous options to a default instance was first
considered, using mkRenamedOptionModule. This however had the following
issues:

• The defaults of the old and the new corresponding user option do not match,
  causing breakage as old consumers would expect root instead of a dynamic
  user. Setting root as the default for the new options would have simply moved
  the footgun.

• The previous user/group options were not doing as described, making it
  unclear to which option they should have implicitly mapped to.

• mkRenamedOptionModule was incompatible with the use of an attrset option
  for the different instances due to the namespace overlap.

In any case, some manual intervention by system administrators is still
necessary no matter what: file ownership and permissions would have to be
corrected instead of relying on root being able to access everything and
hiding the issue.

As per the usual guidelines, both the breaking changes and migration
instructions were already added in the release notes in this pull request.

Complementing those further, I propose adding migration instructions on errors
in this follow-up pull request: #324923.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/can-we-please-stop-breaking-stuff-willy-nilly/48496/30

[emilazy]

While already problematic when taken separately, both of these flaws combined allowed any local user to execute any program as root on the machine.

Thank you for your work on fixing this vulnerability!

Are there any plans to issue a security advisory of some kind for users that outlines the risk they’ve been exposed to, perhaps after this is fixed on stable? I would be willing to help out if people think it’s desirable and nothing has been drafted yet.

[LeSuisse]

Are there any plans to issue a security advisory of some kind for users that outlines the risk they’ve been exposed to, perhaps after this is fixed on stable? I would be willing to help out if people think it’s desirable and nothing has been drafted yet.

At least it was my intention and yes I would have hopped to have something for stable ready.
That's said given the current situation we can probably issue something and update it once we have an agreement for 24.05.

[emilazy]

Good to hear it’s in the pipeline, feel free to ping me on Matrix if I can be of any help :)

Is there a mitigation users on stable can apply to their configurations to lock down the control socket, or will it require changes on stable before anything can be done?

[pacien]

With the constraint of keeping the fcgiwrap module options untouched (keeping
the single shared instance), one can partially mitigate the issues on stable as
follows:

1. Have each consumer wrap their SCRIPT_FILENAME to drop privileges, to
   avoid their CGI programs running as root.

2. Create a group of users explicitly allowed to use the fcgiwrap socket, and
   bypass the fcgiwrap module to add the matching restriction on the socket.

This approach still relies on the shared fcgiwrap daemon to run as root, and
trusts all users of the group to only run programs dropping their privileges.

Here's an (untested) example for cgit served by nginx:

{ config, lib, pkgs, ... }:

{
  users = {
    groups.cgit = { };
    users.cgit = {
      isSystemUser = true;
      group = "cgit";
    };
    groups.fcgiwrap.members = [ "nginx" ];
  };

  systemd.sockets.fcgiwrap.socketConfig = {
    SocketGroup = "fcgiwrap";
    SocketMode = "0660";
  };

  security.wrappers."cgit.cgi" = {
    source = "${pkgs.cgit}/cgit/cgit.cgi";
    setuid = true;
    setgid = true;
    owner = "cgit";
    group = "cgit";
  };

  services.nginx.virtualHosts."domain.tld".locations."/".fastcgiParams = {
    SCRIPT_FILENAME = lib.mkForce "${config.security.wrapperDir}/cgit.cgi";
  };
}

This mitigation method would need to be applied by the users themselves,
because altering the users and groups in the consumer modules within nixpkgs
would still cause breakage (for example git repositories needing to be
chowned/chmodded). I don't think there's any way we can backport anything
without breaking any existing setup.

I'm not familiar with how advisory are handled. In particular, who should write
and publish them, and where?

[LeSuisse]

I'm not familiar with how advisory are handled. In particular, who should write and publish them, and where?

We recently started to publish them in the following Discourse sub category: https://discourse.nixos.org/c/announcements/security/56

I was starting to craft something up but it's all your work so if you want to do it please help yourself (and thanks for your work on this ❤️ ). Do not hesitate to post it in the #security-discuss Matrix room if you want some reviews before publishing it.

I don't think there's any way we can backport anything without breaking any existing setup.

I have the same conclusion but maybe we could at least add an assertion throwing a message when services.fcgiwrap.user and services.fcgiwrap.group are null linking to the advisory. Yes, it would break things but that would make people aware of the situation. If they still want to go with it they can set services.fcgiwrap.user = "root"; and services.fcgiwrap.group = "root"; explicitly.

[emilazy]

I’m a bit worried about starting another unproductive discussion, but if there’s no way to protect people backwards‐compatibly and the manual mitigation is fairly complex, maybe it might make sense to backport this to stable after all. Silently leaving people with a known‐insecure configuration seems very bad, but making people add involved manual mitigations to their configuration, when they’ll have to change again upon upgrading to the next stable release, seems silly. As an analogy, Rust’s backwards‐compatibility guarantee is explicitly waived for soundness issues. Of course, we’d need to make sure we have good error messages in that case.

[nh2]

My opinions on how this should be done, as an industrial user (meaning running servers that people depend on):

• The writeup nixos/fcgiwrap: refactor to fix permissions #318599 (comment) by @pacien is a great and exactly what's needed to make good decisions.
• The security tag is correct here. This was a big Local Privilege Escalation bug.
• This should not be backported to stable if it requires manual user interaction:
  • Lots of people run stable with system.autoUpgrade to get 0-manual-involvement security updates (great value!). They rely on servers staying online, and getting automatic fixes for critical security bugs, such as the recent OpenSSH Remote Code Execution vulnerability.
  • Thus stable changes should never break autoUpgrade.
    Introducing e.g. a throw error during evaluation (e.g. telling the user that they should change their NixOS options) would stop automatic updates, preventing systems from automatically getting much more important fixes, like the OpenSSH fix.
  • Making a change stable that just turns the services in question off would be wrong. Lots of servers are set up such that Local Privilege Escalation does not matter / is a very small threat. The services staying online is important. Stable should backport straightforward fixes; when re-archtitecturing is needed to fix wrong security architecture (as happens here), that should be in unstable and be part of the next release.
• What this PR does (multiple independent fcgiwrap instances) is the correct technical solution.
• This PR is acceptable for nixos-unstable.

But I think it would have been even better to:

• Do the fix as services.fcgiwraps, keep services.fcgiwrap unchanged.
  (As suggested by @bendlas.)
  • This allows trivial backporting of this change to stable without breakage.
  • Also reduces damage and allows users to switch back if we find that the fix is not quite correct.
  • Lets stable users choose whether fixing a Local Privilege Escalation is something they need to fix now or with the next release.
• Immediately wrap an eval warning around services.fcgiwrap, land that in unstable and stable.
  This will make all users that interactively upgrade NixOS notice and upgrade fast, while not breaking autoUpgrade.
• Make a security announcement (as proposed above, here).
• On unstable, add a warning that services.fcgiwrap is insecure and unmaintained, and will be removed soon.
  Remove the old module shortly before the release of NixOS 24.11, or remove it for NixOS 25.05 (removing it later would allow users to upgrade NixOS independently from switching to the new module architecture as a minor added convenience).

Doing that seems to have no drawbacks.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/can-we-please-stop-breaking-stuff-willy-nilly/48496/51

[sternenseemann]

This is not true (also in the case of true). If unset, it defaults to the empty string in the systemd configuration which results in the socket being owned by root.

[pacien]

Good catch!
While fixing the options, I copied the mistake from the previous documentation…
Here are two possible solutions (pick one):

• nixos/fcgiwrap: fix doc for socket owner defaults #326298
• nixos/fcgiwrap: require explicit owner for UNIX sockets #326315

[pacien]

• Do the fix as services.fcgiwraps, keep services.fcgiwrap unchanged.
  (As suggested by @bendlas.)

#324923 goes with services.fcgiwrap.instances.
That should allow both to co-exist on stable after back-porting the
multi-instance module.

• This allows trivial backporting of this change to stable without breakage.
• Also reduces damage and allows users to switch back if we find that the fix is not quite correct.
• Lets stable users choose whether fixing a Local Privilege Escalation is something they need to fix now or with the next release.
• Immediately wrap an eval warning around services.fcgiwrap, land that in unstable and stable.

This is a bit trickier for the consumer modules that we have in nixpkgs
for stable; My current plan is to migrate them to the multi-instance module
while keeping insecure defaults in the consumers not to break them, adding
some option to override those.

[LeSuisse]

* This should _not_ be backported to stable if it requires manual user interaction:
  
  * Lots of people run stable with [`system.autoUpgrade`](https://search.nixos.org/options?channel=24.05&show=system.autoUpgrade.enable&from=0&size=50&sort=relevance&type=packages&query=autoUpgrade) to get 0-manual-involvement security updates (great value!). They rely on servers staying online, and getting automatic fixes for critical security bugs, such as the recent [OpenSSH Remote Code Execution vulnerability](https://discourse.nixos.org/t/security-advisory-openssh-cve-2024-6387-regresshion-update-your-servers-asap/48220/21).
  * Thus **stable changes should never break `autoUpgrade`**.
    Introducing e.g. a `throw` error during evaluation (e.g. telling the user that they should change their NixOS options) would stop automatic updates, preventing systems from automatically getting much more important fixes, like the OpenSSH fix.
  * Making a change stable that just turns the services in question _off_ would be wrong. Lots of servers are set up such that Local Privilege Escalation does not matter / is a very small threat. The services staying online is important. Stable should backport straightforward fixes; when re-archtitecturing is needed to fix wrong security architecture (as happens here), that should be in `unstable` and be part of the next release.

It is unrelated to this change but note this already happen from time to time when packages get tagged with knownVulnerabilities due to the lack of a better option.

Assuming the threat model of everyone by concluding that a LPE to root does not matter / is a very small threat is also probably not something that should be done at a distro level. We do not have access to the context of all possible deployments that could allow us to say that with certitude.

I'm also a bit puzzled by the fact the described scenario assumes people will not notice a failure in the auto-upgrade mechanism but they will notice an advisory.

[nh2]

Assuming the threat model of everyone by concluding that a LPE to is a very small threat [..] not something that should be done at a distro level

@LeSuisse I didn't say "everybody", I said "lots of servers are set up such that LPE is a very small threat". Most servers I've encountered in industry in the last 10 years were not "multi UNIX user systems" where multiple humans SSH in. So LPE is usually a danger of risk escalation from one already-compromised service to root. But of course true multi-user systems still exist, e.g. universities often have them.

My point is that a distro should not assume that users prefer their services being turned off over continuing running with a bug that may or may not be a real risk for them. The users should decide that.

(Of course the distro should automatically deploy any straightforward fixes; here we're talking only about the situation where you have to pick between turning the service off or keeping it running bugged.)

I also want to point out that turning the service off explicitly is the only thing NixOS could do for a backwards-incompatible change. Because just inserting throw would make evaluation fail, but that keeps existing services running (and thus brings no security fix but turns off other updates).

I'm also a bit puzzled by the fact the described scenario assumes people will not notice a failure in the auto-upgrade mechanism but they will notice an advisory.

Can I configure system.autoUpgrade easily to notify me about failures?

For the advisories, all I need to do is to click the bell icon on https://discourse.nixos.org/c/announcements/security/56, very easy and convenient:

[pacien]

24.05 backport PR: #331465

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/security-advisory-local-privilege-escalation-in-the-fcgiwrap-nixos-module-also-affecting-the-cgit-smokeping-and-zoneminder-modules/51419/1