Skip to content
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

libvirtd: polkit integration, security fixes #87576

Merged
merged 2 commits into from May 13, 2020

Conversation

@offlinehacker
Copy link
Contributor

offlinehacker commented May 11, 2020

Motivation for this change

Currently if libvirtd module is enabled anyone can access libvirtd socket (not only read only), since sockets has wide read and write permissions. This is by design, since in most distros polkit is enabled and polkit does auth/authz checks. In nixos (polkit) auth was disabled and it was thought security was enforced by unix_sock_rw_perms, but it was not, since sockets were created by systemd with wide open permissions.

Anyway, enabling polkit authorization fixes all that and enables additional rules to be created. A default rule is that anyone with access to libvirtd group has access to libvirt, which is the same as in other distros.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Contributor

bjornfor left a comment

Nit: 2nd commit should have "nixos/libvirtd" prefix.

@offlinehacker offlinehacker force-pushed the xtruder:pkgs/libvirtd/polkit branch from 11551b6 to 9530d2b May 11, 2020
@offlinehacker
Copy link
Contributor Author

offlinehacker commented May 11, 2020

fixed, thanks

@offlinehacker offlinehacker changed the title Pkgs/libvirtd/polkit libvirtd: polkit integration, security fixes May 12, 2020
@offlinehacker
Copy link
Contributor Author

offlinehacker commented May 12, 2020

@GrahamcOfBorg build libvirt

@ajs124
ajs124 approved these changes May 12, 2020
@offlinehacker
Copy link
Contributor Author

offlinehacker commented May 13, 2020

I will merge this as it presents a security issue, where it is very easy to compromise nixos workstation running libvirtd. I am also using libvirt for some time with this configuration without any issues.

@offlinehacker offlinehacker force-pushed the xtruder:pkgs/libvirtd/polkit branch from 9530d2b to 9d7fa9d May 13, 2020
@offlinehacker offlinehacker force-pushed the xtruder:pkgs/libvirtd/polkit branch from 9d7fa9d to 056ab3d May 13, 2020
@offlinehacker offlinehacker merged commit 9a29fe5 into NixOS:master May 13, 2020
1 check was pending
1 check was pending
grahamcofborg-eval Checking original out paths
Details
@conferno
Copy link
Contributor

conferno commented May 13, 2020

Should services.libvirtd.enable = true also set services.polkit.enable = true ?

@conferno
Copy link
Contributor

conferno commented May 13, 2020

Or

if config.services.polkit.enable then <new code> else <old code>

?

Now

services.libvirtd.enable = true 
services.polkit.enable = false

is not verboten, but will it work?

@offlinehacker
Copy link
Contributor Author

offlinehacker commented May 13, 2020

Good catch I will create new pull request that explicitly enables polkit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.