nixos/wpa_supplicant: harden and run as unprivileged user#427528
nixos/wpa_supplicant: harden and run as unprivileged user#427528rnhmjoj merged 4 commits intoNixOS:masterfrom
Conversation
2d70906 to
9619d68
Compare
|
Could someone from @NixOS/freedesktop help with testing this doesn't break NetworkManager? |
|
@LeSuisse, since you're the only one to have touched connman recently, can you help testing? |
| # set up imperative config file | ||
| "+${pkgs.coreutils}/bin/touch /etc/wpa_supplicant/imperative.conf" | ||
| "+${pkgs.coreutils}/bin/chmod 664 /etc/wpa_supplicant/imperative.conf" | ||
| "+${pkgs.coreutils}/bin/chown -R wpa_supplicant:wpa_supplicant /etc/wpa_supplicant" | ||
| ] | ||
| ++ lib.optionals cfg.userControlled [ | ||
| # set up client sockets directory | ||
| "+${pkgs.coreutils}/bin/mkdir /run/wpa_supplicant/client" | ||
| "+${pkgs.coreutils}/bin/chown wpa_supplicant:wpa_supplicant /run/wpa_supplicant/client" | ||
| "+${pkgs.coreutils}/bin/chmod g=u /run/wpa_supplicant/client" |
There was a problem hiding this comment.
Shouldn't this be done via tmpfiles instead?
There was a problem hiding this comment.
I always see tmpfiles being recommended, but it's not a substitute for a privileged prestart script. Even if I add an After=systemd-tmpfiles-setup.service, it's not guaranteed that the files actually exist when the service is started, they may have been deleted in the meantime.
There was a problem hiding this comment.
Why is that not guaranteed? What should delete those files? If the user deletes them then that is their fault.
There was a problem hiding this comment.
What should delete those files?
I don't know, but why would I increase the chance of failure when I can just set up everything needed right before starting the service?
| RootDirectory = "/run/wpa_supplicant"; | ||
| RootDirectoryStartOnly = true; |
There was a problem hiding this comment.
Would it make sense to instead to rely on systemd.services.wpa_supplicant.confinement?
I remember that it was controversial when I wrote the Akkoma NixOS package.
There was a problem hiding this comment.
I'm not familiar with this option: what's the difference?
There was a problem hiding this comment.
It creates a bind mount with only the store paths required to run the service.
There was a problem hiding this comment.
store paths as in /nix/store paths?
There was a problem hiding this comment.
My last touch with confinement was in postfix-tlspol where it was removed again because it is not easy to debug.
There was a problem hiding this comment.
store paths as in /nix/store paths?
Exactly. It does so by producing a systemd override configuration from the closure information with BindReadOnlyPath directives for all required store paths: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/security/systemd-confinement.nix#L218
I hacked together a similar thing for a systemd user service via Home Manager: ausweisapp.nix.txt
Instead of relying on RootDirectory like your proposal or the confinement module, it sets up an empty tmpfs with TemporaryFileSystem = "/:ro,nodev,noexec,nosuid", which I felt was cleaner.
There was a problem hiding this comment.
It seems a bit extreme. What's the benefit of locking down /nix/store? The only thing I can think of is preventing wpa_supplicant from stealing some world-readable "secret" that went into the store.
There was a problem hiding this comment.
It seems a bit extreme. What's the benefit of locking down /nix/store? The only thing I can think of is preventing wpa_supplicant from stealing some world-readable "secret" that went into the store.
I believe the idea is not to hide confidential information but to reduce the set of executable code to the bare minimum required to operate the service.
Compared to other hardening features it may have a less favourable cost/benefit ratio, especially if run-time dependencies vary with configuration.
043ec85 to
e1fde84
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5708 |
|
Since apparently no one is using nor maintaining them: For testing I modified the Detailsdiff --git a/nixos/modules/virtualisation/qemu-vm.nix b/nixos/modules/virtualisation/qemu-vm.nix
index 0b0b2136ea03..77b63b23b8de 100644
--- a/nixos/modules/virtualisation/qemu-vm.nix
+++ b/nixos/modules/virtualisation/qemu-vm.nix
@@ -1435,10 +1435,6 @@ in
VertRefresh 50-160
'';
- # Wireless won't work in the VM.
- networking.wireless.enable = mkVMOverride false;
- services.connman.enable = mkVMOverride false;
-
# Speed up booting by not waiting for ARP.
networking.dhcpcd.extraConfig = "noarp";
diff --git a/nixos/tests/wpa_supplicant.nix b/nixos/tests/wpa_supplicant.nix
index 2dbd5b5d18f5..d1bf62da480b 100644
--- a/nixos/tests/wpa_supplicant.nix
+++ b/nixos/tests/wpa_supplicant.nix
@@ -11,7 +11,7 @@ let
];
};
- naughtyPassphrase = ''!,./;'[]\-=<>?:"{}|_+@$%^&*()`~ # ceci n'est pas un commentaire'';
+ naughtyPassphrase = "reproducibility";
runConnectionTest =
name: extraConfig:
@@ -76,18 +76,100 @@ let
};
# wireless client
- networking.wireless = lib.mkMerge [
- {
- # the override is needed because the wifi is
- # disabled with mkVMOverride in qemu-vm.nix.
- enable = lib.mkOverride 0 true;
- userControlled = true;
- interfaces = [ "wlan1" ];
- fallbackToWPA2 = lib.mkDefault true;
- secretsFile = "/var/lib/secrets/wpa";
- }
- extraConfig
+ networking = {
+ useDHCP = false;
+ useNetworkd = false;
+ firewall.allowedUDPPorts = [ 547 ];
+ interfaces = lib.mkOverride 0 {
+ wlan0.ipv4.addresses = [
+ {
+ address = "192.168.0.1";
+ prefixLength = 24;
+ }
+ ];
+ wlan0-1.ipv4.addresses = [
+ {
+ address = "192.168.1.1";
+ prefixLength = 24;
+ }
+ ];
+ wlan0-2.ipv4.addresses = [
+ {
+ address = "192.168.2.1";
+ prefixLength = 24;
+ }
+ ];
+ };
+ };
+
+ services.kea.dhcp4.enable = true;
+ services.kea.dhcp4.settings = {
+ interfaces-config = {
+ interfaces = [
+ "wlan0"
+ "wlan0-1"
+ "wlan0-2"
+ ];
+ dhcp-socket-type = "raw";
+ service-sockets-require-all = true;
+ service-sockets-max-retries = 5;
+ service-sockets-retry-wait-time = 2500;
+ };
+ subnet4 =
+ map
+ (n: {
+ id = n + 1;
+ subnet = "192.168.${toString n}.0/24";
+ pools = [ { pool = "192.168.${toString n}.3 - 192.168.${toString n}.254"; } ];
+ option-data = [
+ {
+ data = "192.168.${toString n}.1";
+ name = "routers";
+ }
+ {
+ data = "192.168.${toString n}.1";
+ name = "domain-name-servers";
+ }
+ ];
+ })
+ [
+ 0
+ 1
+ 2
+ ];
+ };
+
+ services.connman.enable = true;
+ services.connman.networkInterfaceBlacklist = [ "wlan0-1" "wlan0-2" "wlan0" ];
+ environment.systemPackages = [ pkgs.connman-gtk ];
+
+ networking.networkmanager.enable = true;
+ networking.networkmanager.unmanaged = [ "wlan0-1" "wlan0-2" "wlan0" ];
+
+ services.xserver.enable = true;
+ services.xserver.desktopManager.plasma6.enable = true;
+ services.xserver.displayManager.autoLogin.enable = true;
+ services.xserver.displayManager.autoLogin.user = "vm";
+
+ virtualisation.memorySize = 2048;
+ services.spice-vdagentd.enable = true;
+ virtualisation.qemu.options = [
+ "-vga qxl"
+ "-spice port=5924,disable-ticketing=on"
+ "-device virtio-serial -chardev spicevmc,id=vdagent,debug=0,name=vdagent"
+ "-device virtserialport,chardev=vdagent,name=com.redhat.spice.0"
];
+
+ users.users.vm = {
+ password = "vm";
+ isNormalUser = true;
+ extraGroups = [
+ "networkmanager"
+ "wheel"
+ ];
+ };
+ security.sudo.enable = true;
+
};
testScript = ''
It's a bit tricky to run an 802.11 simulator, access point, DHCP server and wireless client all in the same host, but it worked. |
419b2cd to
7ec99e0
Compare
7ec99e0 to
24b6601
Compare
|
By the way, these formatting changes are becoming annoying: they turned an almost theoretical problem (arguing about code style) into a very real one (constant merge conflicts). |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5787 |
NixOS/nixpkgs#427528 This adds the wpa-supplicant hardening upstream and also leans on the networkmanager for managing the wireless (if using networkmanager) Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
NixOS/nixpkgs#427528 This adds the wpa-supplicant hardening upstream and also leans on the networkmanager for managing the wireless (if using networkmanager) Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
NixOS/nixpkgs#427528 This adds the wpa-supplicant hardening upstream and also leans on the networkmanager for managing the wireless (if using networkmanager) Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
NixOS/nixpkgs#427528 This adds the wpa-supplicant hardening upstream and also leans on the networkmanager for managing the wireless (if using networkmanager) Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
NixOS/nixpkgs#427528 This adds the wpa-supplicant hardening upstream and also leans on the networkmanager for managing the wireless (if using networkmanager) Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
NixOS/nixpkgs#427528 This adds the wpa-supplicant hardening upstream and also leans on the networkmanager for managing the wireless (if using networkmanager) Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
NixOS/nixpkgs#427528 This adds the wpa-supplicant hardening upstream and also leans on the networkmanager for managing the wireless (if using networkmanager) Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
|
Duplicating my message from Matrix: I think that this needs to be reverted. I believe it hasn't made it into a stable release yet. The breakage it causes for non-trivial use-cases is extensive and hard to recover from. This kind of massive change to a core component of the system (wifi!) should be feature-flagged, and not just rolled out like that without a way to turn it off. If this makes it into a stable release as-is it is very likely that some people with enterprise networking setups (even something as simple as eduroam) end up offline after upgrading, until they roll back. For more complicated setups (such as TPM-based authentication) I haven't even been able to get this to work yet, as it requires complicated configuration of a plethora of additional accesses (to devices, mutable files, dynamic libraries and so on). Of course the general idea is laudable, but something this core needs to be done more carefully. |
|
Agreed, @rnhmjoj could you revert or offer an option for opt-out? While it seems to work well enough for common use cases that it took weeks for many to notice - thank you for testing properly! - opting-out should still be made easy for a while IMO. And unless I am mistaken, there wasn't any effort to discuss the patch to wpa_supplicant with upstream before merging? (I also think merging such a big change yourself, seemingly without any approvals is less than ideal to be honest) |
|
I'm working on adding an option to disable the hardening sandbox, but still keeping wpa_supplicant running as its own user. I think this will allow you to keep your setup as is, without having to remove the wpa_supplicant patches. |
|
Please take a look at PR #484382, can you tell me if that solves your problem? |
|
I think this is a good change on making the default NixOS desktop installation more secure and Maybe it is also worth to take another look at the release notes and expand them with those specific usecases to put more attention to them. |
I think most of the users who reported a problem didn't have a declarative configuration, in which case an option to manually disable the hardening is still required. Otherwise, it could probably be done if someone provides some examples.
Yeah, I'm also working on improving the manual, which was very outdated and incomplete. See #483534 |
|
@rnhmjoj Sorry, could you please respond to my request of at least adding a feature-flag around this? Doing so has been a thing for significant changes to core parts of NixOS for quite a while now and included unanimously welcomed changes (such as systemd-in-initrd & nixos-rebuild-ng for example) and has no discernible drawbacks IMO. As it stands:
You seem to deliberately want to break user setups in order to force feedback:
I do think this stance is unacceptable for a committer and kindly ask you to reconsider. |
|
See #484382 |
|
This is a feature-flag for one part of the PR, but judging by the diff it does not affect the permission changes around certificates (the eduroam issue). Nor does it update the release notes. |
Yes, I understood that and I mentioned it in the release notes. It's not something unheard of... you may need to perform some manual step when updating between NixOS stable versions, or if you're on the unstable NixOS channel.
#484382 addresses this. Breaking the enterprise networks setup is of course unacceptable and that's why I'm adding an option to revert the hardening; but I don't think that mandating proper ownerships of some files is that unreasonable. Still, I could throw in a
This is not true, see https://github.com/NixOS/nixpkgs/blob/master/nixos/doc/manual/release-notes/rl-2605.section.md?plain=1#L103. I will nontheless improve the documentations and instructions (see #483534) once |
It doesn't, though. Even with all those changes applied as overrides, having reverted the The toggle needs to completely undo the original changes. |
From what google shows me, that could be a red herring and was already an issue before. https://forum.manjaro.org/t/troubles-with-networkmanager/38420/5 This should be fixed with the sleep fix already done. If you're current issue is different, please share that.
What you are asking for, is running a service that talks to potentially untrusted input with complete root permissions, so every exploit would be fatal. I understand that migrating of that can be painful but we need to start somewhere and try to move in the right direction and for that we need feedback. I think having an option which opens things up for the next release(s) is great but we then also need to receive actionable feedback where we can improve things and make the secure default work properly.
No, it isn't. I think the best example is ssh: when there permissions are to wide, it just refuses to use the files. |
Sorry, you're way behind on the discussion of that particular error. Read the related threads.
What I am asking for is the status quo. As has been said before, it's great that you want to move in a more secure direction (although of course I am wary of all the bugs in systemd in this regard), but you can't do this by breaking workflows completely and without providing a way to fix it. Right now, this requires |
My apologies, I stand corrected regarding the release notes. Not sure how I missed this in my original reading. Still, the rest of the argument still stands: it should be possible to opt these changes completely. Deliberately breaking User setups to force feedback is not okay |
|
This PR breaks using the |
Similarly to #479042 (comment), please don't do this: it worked mostly by accident and has never been tested. When using NetworkManager you should configure networks declaratively with |
As we did for ages and upstream still recommends, as illustrated by the fact that we need an in-tree patch to even support running unprivileged. A patch that afaik hasn't even been proposed to upstream.
Yes, and that "somewhere" is a feature-flag, not a forced breakage of user setup.s |
|
Reminder that #484382 is still waiting for your feedback. |
|
I have NetworkManager handling ethernet, and don't have wireless networks configured since I don't use any, but I also have the read-only etc overlay enabled, which breaks the new service definition attempting to create |
I guess you can safely disable wpa_supplicant, then. Use
Don't do this? Many services expect /etc to be writable, including NetworkManager, wpa_supplicant, resolvconf, etc.
This is actually not needed if wpa_supplicant is controlled from dbus. The condition could probably be refined to deal with this case. |


One more step towards zero services running as root.
Inspired by #305722
Things done
nixosTests.wpa_supplicantnixosTests.networking.networkmanagernixosTests.connmannixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.