-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
nixos/fcgiwrap: refactor to fix permissions #318599
Conversation
This allows configuring and starting independent instances of the fgciwrap service, each with their own settings and running user, instead of having to share a global one. I could not use `mkRenamedOptionModule` on the previous options because the aliases conflict with `attrsOf submodule` now defined at `services.fcgiwrap`. This makes this change not backward compatible.
This also fixes the gitolite-fcgiwrap test by running git through fcgiwrap as the proper user.
Since we're already introducing some backward-incompatible change in the previous commit, let's make the options more tidy, also preparing for the introduction of more options. This also fixes the documentation of the user and group options which are applying to the service's running user, not the socket.
This adds a few options to properly set the ownership and permissions on UNIX local sockets, set to private by default. Previously, the created UNIX local sockets could be used by any local user. This was especially problematic when fcgiwrap is running as root (the default).
Use a dynamic user instead unless one is specified.
This allows running cgit instances using dedicated users instead of root. This is now set to "cgit" by default.
The GIT_PROJECT_ROOT directory is now created at runtime instead of being assembled at build time. This fixes ownership issues which prevented those repositories to be read by users other than root. This also avoids creating symlinks in the nix store pointing to the outside.
0ea83ba
to
3d10deb
Compare
(Rebased to fix merge conflict). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks sensible to me and I'm also find with the changes.
Not quite sure how to handle it on stable :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks quite good, and quite useful. Thanks! I'm interested in running cgit
as non-root.
Moving to merge this, flagging for backport to 24.05 so we can discuss how we want to handle it. |
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
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 |
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 |
That should have never been a thing. Thanks for cleaning that up.
I agree that the scope of this PR goes far beyond removing the obvious footgun and can't be backported in full.
But that was also an open discussion question.
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.
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 think that is undesirable. |
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 so? |
Hello,
Thank you for the feedback and I can understand the frustration. To be clear, I merged this because:
|
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?
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. |
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 |
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 |
@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. |
Hello, I believe some clarifications are needed. Purposes of these changes, security aspectThe fcgiwrap module had the following flaws:
While already problematic when taken separately, both of these flaws combined Hence the security aspect of this pull request with the matching tag, and open (I also reached out privately to a member of the security team prior to posting On the API breakage / refactorThe fcgiwrap options are consumed by several other modules within nixpkgs (such Those modules were able to share the same instance "thanks to" the flaws listed As such, changing the default user and/or fixing the control socket ownership Taking into account those constraints, it was not possible to continue sharing (It is to be noted that the sourchut module was already avoiding this issues by Breaking API changes to support isolated instances were therefore introduced Migration handlingAn implicit mapping from the previous options to a default instance was first
In any case, some manual intervention by system administrators is still As per the usual guidelines, both the breaking changes and migration Complementing those further, I propose adding migration instructions on errors |
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 |
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. |
At least it was my intention and yes I would have hopped to have something for stable ready. |
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? |
With the constraint of keeping the fcgiwrap module options untouched (keeping
This approach still relies on the shared fcgiwrap daemon to run as root, and 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, I'm not familiar with how advisory are handled. In particular, who should write |
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 have the same conclusion but maybe we could at least add an assertion throwing a message when |
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. |
My opinions on how this should be done, as an industrial user (meaning running servers that people depend on):
But I think it would have been even better to:
Doing that seems to have no drawbacks. |
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 |
description = "User permissions for the socket."; | ||
description = '' | ||
User to be set as owner of the UNIX socket. | ||
Defaults to the process running user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
While fixing the options, I copied the mistake from the previous documentation…
Here are two possible solutions (pick one):
#324923 goes with
This is a bit trickier for the consumer modules that we have in nixpkgs |
It is unrelated to this change but note this already happen from time to time when packages get tagged with 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. |
@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
Can I configure 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: |
24.05 backport PR: #331465 |
* Treewide: re-run depotfmt * //third_party/nixpkgs:html5validator: build with Python 3.11, dependency openstackdocstheme doesn't support 3.12 * //users/sterni/machines/ingeborg: adapt to poorly handled fcgiwrap module API change: NixOS/nixpkgs#318599 * //tvix/*-go: regenerate protobuf files * //third_party/nixpkgs:treefmt: Remove patch for merged pull request * //users/flokli/ipu6-softisp: rebase, drop upstreamed kernel patches Change-Id: Ie4e0df007c287e8cd6207683a9a25838aa5bd39a Reviewed-on: https://cl.tvl.fyi/c/depot/+/11971 Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: flokli <flokli@flokli.de> Reviewed-by: aspen <root@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: Ilan Joselevich <personal@ilanjoselevich.com>
* Treewide: re-run depotfmt * //third_party/nixpkgs:html5validator: build with Python 3.11, dependency openstackdocstheme doesn't support 3.12 * //users/sterni/machines/ingeborg: adapt to poorly handled fcgiwrap module API change: NixOS/nixpkgs#318599 * //tvix/*-go: regenerate protobuf files * //third_party/nixpkgs:treefmt: Remove patch for merged pull request * //users/flokli/ipu6-softisp: rebase, drop upstreamed kernel patches Change-Id: Ie4e0df007c287e8cd6207683a9a25838aa5bd39a Reviewed-on: https://cl.tvl.fyi/c/depot/+/11971 Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: flokli <flokli@flokli.de> Reviewed-by: aspen <root@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: Ilan Joselevich <personal@ilanjoselevich.com>
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Description of changes
This PR fixes mulitple issues with the fcgiwrap module:
solving the interference issue between its consumer modules.
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
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.