[24.05] backport fcgiwrap instances fix for local privilege escalation issue #331465
Conversation
pacien
force-pushed
the
release-24.05-backport-fcgiwrap-instances
branch
from
14842c8
to
1c1ce6d
Compare
| warnings = flatten (flip mapAttrsToList cfgs (inst: cfg: | ||
| optional (cfg.user == "root") '' | ||
| `services.cgit.${inst}` is configured to run as root. | ||
| This has security implications: <TODO: advisory link>. |
There was a problem hiding this comment.
I'm not sure whether it's usual to post an advisory link in evaluation warnings, but it seems useful to me.
Nevertheless, here's a draft for the advisory:
https://gist.github.com/pacien/82119f8e1a6569c5b12bf0f75ecc3b9a
I'd appreciate a review for it as well.
There was a problem hiding this comment.
Thanks for your work on this. I think linking to the advisory is great; we should certainly post it on Discourse and arguably https://github.com/NixOS/nixpkgs/security/advisories (as‐yet unused).
I personally feel that a warning is not sufficient and that insecure configurations should be a hard error, because otherwise it’s likely they won’t be surfaced through automatic update pipelines etc.; however I realize there is a compatibility cost here.
There was a problem hiding this comment.
I personally feel that a warning is not sufficient and that insecure configurations should be a hard error, because otherwise it’s likely they won’t be surfaced through automatic update pipelines etc.; however I realize there is a compatibility cost here.
I think the biggest issue with that, especially on the stable channel, is that for people using the auto-upgrade and not really monitoring their machine, it would block other compatible security upgrades. Like for example the recent curl CVE.
There was a problem hiding this comment.
There’s lots of circumstances where that would bite you. Packages do break unexpectedly. You need monitoring of failures to have security; Nix warnings, on the other hand, are rarely security‐relevant, and are trickier to automatically detect and monitor.
There was a problem hiding this comment.
There’s lots of circumstances where that would bite you. Packages do break unexpectedly. You need monitoring of failures to have security; Nix warnings, on the other hand, are rarely security‐relevant, and are trickier to automatically detect and monitor.
I agree, you definitely should monitor your machines. But I think lots of people still don't.
But it's true that warnings would be harder to detect, hmm…
There was a problem hiding this comment.
I think we could just add a services.fcgiwrap.allowLocalPrivilegeEscalationUntilNextStableRelease option or something if we want an escape hatch.
I agree that the systemd service failure option is not ideal.
The GitHub advisory tab lets us assign CVEs, as I understand it, so it would actually be quite nice to use. But only repository admins can post stuff; we’d probably need to organize procedures and someone to be responsible for it.
There was a problem hiding this comment.
Should we use the Github advisory then?
Would it be possible to reserve a number and URL to put it in the warnings and errors?
There was a problem hiding this comment.
GHA are not really adequate in our situation with the current way the permissions are handled. Creation and publication needs to be handled by repo admins and cannot be delegated :/ (also not sure if tying more things with the GH ecosystem is a good play here).
In this specific case at this point the cat is out of the bag so I suggest to go ahead and publish the advisory on https://discourse.nixos.org/c/announcements/security/56 when this is ready to be merged to get the final link so it can be added to the messages.
There was a problem hiding this comment.
I think Discourse will be okay, but there’s indeed a bit of a race condition there. Maybe we can post something set to private and then let it go public once we merge? I’m not sure who has that kind of access on Discourse other than the moderation team.
There was a problem hiding this comment.
I have added a commit which causes an evaluation failure, with a new option
services.fcgiwrap.allowGlobalInstanceLocalPrivilegeEscalation as an escape
hatch.
I'm not sure of the consensus of whether to have this or not, so feel free
to include or not this latest commit.
The error should be clear enough nevertheless.
Regarding the Discourse advisory: making a private post and then making it
public might break notifications.
I'll make the post and update the link in the PR whenever someone will tell me
to.
pacien
requested review from
mweinelt,
bendlas,
nh2,
LeSuisse,
minijackson,
sternenseemann and
emilazy
| warnings = [ | ||
| '' | ||
| The fcgiwrap module is configured with a global shared instance. | ||
| This has security implications: <TODO: advisory link>. |
There was a problem hiding this comment.
Same as #331465 (comment)
There was a problem hiding this comment.
PR looks clean and sensible to me. Would approve.
About the advisory: https://gist.github.com/pacien/82119f8e1a6569c5b12bf0f75ecc3b9a
Cache files owned by root may however need to be manually cleared.
What's the impact of having root-owned cache files left over? Does anything break?
Would it make sense to have a pre-start script to either clear cache files of the wrong user, or chown them?
Overall thanks for taking this on!
This backports the options `services.fcgiwrap.instances.*`, allowing to configure isolated instances of fcgiwrap, as an alternative to the global shared one. This prepares the deprecation of the latter. Backport of: commit efc7aeb nixos/fcgiwrap: require explicit owner for UNIX sockets commit 4f2da6c nixos/fcgiwrap: add option migration instruction errors (partial: move to instances) commit 51b246a nixos/fcgiwrap: do not run as root by default commit 81f7201 nixos/fcgiwrap: add unix socket owner, private by default commit 289c158 nixos/fcgiwrap: limit prefork type to positives commit 3955eaf nixos/fcgiwrap: improve readability of CLI args commit 022289f nixos/fcgiwrap: group options logically, fix doc commit 41419ca nixos/fcgiwrap: refactor for multiple instances
This deprecates the use of the global shared instance of fcgiwrap, due to its security issues (running as root by default, actually insecure control socket, allowing local remote escalation privileges, with no fix due to the multiple consumers). A warning is added to encourage users to migrate to properly isolated instances (`services.fcgiwrap.instances.*`).
This makes the CGI part of smokeping run as the unprivileged "smokeping" user like the rest of the service (instead of root). This also sets proper permissions for the fcgiwrap control socket. Backport of: commit 4f2da6c nixos/fcgiwrap: add option migration instruction errors (partial: move to instances) commit c5dc3e2 nixos/fcgiwrap: adapt consumer modules and tests commit 8101ae4 nixos/fcgiwrap: adapt consumer modules and tests commit bf2ad6f nixos/fcgiwrap: adapt consumer modules and tests
This adds options to set the users and groups as which cgit instances run, allowing the use of an unprivileged user instead of root. "root" is kept as the default user to avoid breaking existing setups, but a warning is shown in that case to alert the user. Backport of: commit 4f2da6c nixos/fcgiwrap: add option migration instruction errors (partial: move to instances) commit 3d10deb nixos/cgit: fix GIT_PROJECT_ROOT ownership commit 2d8626b nixos/cgit: configurable user instead of root commit c5dc3e2 nixos/fcgiwrap: adapt consumer modules and tests commit 8101ae4 nixos/fcgiwrap: adapt consumer modules and tests commit bf2ad6f nixos/fcgiwrap: adapt consumer modules and tests
pacien
force-pushed
the
release-24.05-backport-fcgiwrap-instances
branch
from
1c1ce6d
to
31cdff5
Compare
|
Quoting Herwig Hochleitner (2024-08-02 10:37:42)
What's the impact of having root-owned cache files left over? Does anything break?
Would it make sense to have a pre-start script to either clear cache files of the wrong user, or chown them?
The previously cached files would never be cleaned.
I just added a `chown` in the existing `preStart` script,
so that gets taken care of automatically.
Corresponding obsolete mention removed from the advisory also.
|
Do you think that's something that should go to unstable? I could help with forward-porting that ... |
|
Quoting Herwig Hochleitner (2024-08-02 12:10:50)
> I just added a `chown` in the existing `preStart` script, so that gets taken
> care of automatically.
Do you think that's something that should go to unstable? I could help with
forward-porting that ...
I think that, ideally, the smokeping module on master should instead be updated
to put the cache files in the proper FHS directory managed by systemd with
`CacheDirectory` (something like `/var/cache/smokeping`). But that could
require more work and is out of the scope of the current PR, and should
probably be discussed with the maintainers of that particular module.
|
|
All good points. I'm halfway through putting up a PR to main. I did go the route of managing the current directory structure with |
This adds a security assertion when using the global instance of fcgiwrap, which is vulnerable to a local privilege escalation. This is in addition to the current evaluation warning, and is more in line with being loud with security issues, similarly to with vulnerable packages. The evaluation failure can nevertheless be bypassed by setting: `services.fcgiwrap.allowGlobalInstanceLocalPrivilegeEscalation = true`.
There was a problem hiding this comment.
This PR looks good to me in its current state; sorry for leaving this hanging for so long. The advisory should maybe be updated to mention services.fcgiwrap.allowGlobalInstanceLocalPrivilegeEscalation. Tagging @LeSuisse for a final review.
There is an awkward timing thing here: we’d ideally post the advisory publicly at the point where this is merged and users on 24.05 can actually follow the mitigation, but that would mean that we couldn’t include the advisory link in the corresponding channel bump. Users can subscribe to the security announcements category on Discourse and get emailed alerts, so timing does matter. I think notifications will work fine because every post in that category needs to be approved anyway. I think that making the post private and including the URL here, then later making it public when the PR hits the channels, is probably the best approach. cc @mweinelt who has the perms to approve such posts
|
Another option would be to make the advisory public early, but clarify that users on 24.05 will need to wait for the PR to hit the channels and maybe link one of the trackers for that. That way we don’t need to do any special coordination of the timing. |
|
@emilazy: I have updated the advisory accordingly and submitted it on the |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
There was a slight communication mishap and it’s been published already 😅 That should be fine though since it already points to the PR trackers, so if you edit in the link we can finally get this merged. Thank you for all your work on this issue 💜 |
|
Link added! |
Description of changes
Backport of #318599.
Both
services.fcgiwrap.*and the replacement optionsservices.fcgiwrap.instances.*will co-exist on stable, with warnings when the former are used to notify users of the security issues, giving them time to migrate.The consumer modules have been internally migrated too. For the cgit module, the user and group options were backported while keeping the previous implied default and showing a warning, allowing users to fix the situation gradually.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.