Merge #225: Fix process info restriction

44de5064cd security: don't restrict process info by default for module users (Erik Arvstedt)
a36789b468 test: move security tests to separate function (Erik Arvstedt)
588a0b2405 security: enable full systemd-status for group 'proc' (Erik Arvstedt)
96ea2e671c security: simplify and fix dbus configuration (Erik Arvstedt)
343e026030 rename dbus.nix -> security.nix (Erik Arvstedt)
7367446761 test: rename assert_matches_exactly -> assert_full_match (Erik Arvstedt)

Pull request description:

ACKs for top commit:
  nixbitcoin:
    ACK 44de5064cd

Tree-SHA512: f782cfdc81b5d6b3da968d0221bd54420791a9f5cd89cde9e62d6d04882d921b5efe9046d975133587b5c2d711c47133b3a5a2351940899a90a28bf16218a7ad

modules/dbus.nix

@@ -1,55 +0,0 @@
-{ config, lib, pkgs, ... }:
-
-with lib;
-
-let
-  inherit (config) nix-bitcoin-services;
-  dataDir = "/var/lib/dbus-hardening";
-  # Mitigates a security issue that allows unprivileged users to read
-  # other unprivileged user's processes' credentials from CGroup using
-  # `systemctl status`.
-  dbus-hardening = pkgs.writeText "dbus.conf" ''
-    <?xml version="1.0" encoding="UTF-8"?> <!-- -*- XML -*- -->
-
-    <!DOCTYPE busconfig PUBLIC
-     "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN"
-     "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
-
-    <busconfig>
-      <policy user="root">
-        <allow send_destination="org.freedesktop.systemd1"
-          send_interface="org.freedesktop.systemd1.Manager"
-          send_member="GetUnitProcesses"/>
-      </policy>
-
-      <policy context="mandatory">
-        <deny send_destination="org.freedesktop.systemd1"
-          send_interface="org.freedesktop.systemd1.Manager"
-          send_member="GetUnitProcesses"/>
-      </policy>
-    </busconfig>
-  '';
-in {
-  config = {
-    systemd.tmpfiles.rules = [
-      "d '${dataDir}/etc/dbus-1/system.d' 0770 messagebus messagebus - -"
-    ];
-
-    services.dbus.packages = [ "${dataDir}" ];
-
-    systemd.services.hardeneddbus = {
-      description = "Install hardeneddbus";
-      wantedBy = [ "multi-user.target" ];
-      script = ''
-        cp ${dbus-hardening} ${dataDir}/etc/dbus-1/system.d/dbus.conf
-        chmod 640 ${dataDir}/etc/dbus-1/system.d/dbus.conf
-      '';
-      serviceConfig = nix-bitcoin-services.defaultHardening // {
-        PrivateNetwork = "true";
-        Type = "oneshot";
-        User = "messagebus";
-        ReadWritePaths = "${dataDir}";
-      };
-    };
-  };
-}

modules/modules.nix

@@ -16,7 +16,7 @@
     ./lightning-loop.nix
     ./secrets/secrets.nix
     ./netns-isolation.nix
-    ./dbus.nix
+    ./security.nix
     ./backups.nix
   ];
 

modules/presets/secure-node.nix

@@ -42,8 +42,7 @@ in {
 
     networking.firewall.enable = true;
 
-    # hideProcessInformation even if hardened kernel profile is disabled
-    security.hideProcessInformation = true;
+    nix-bitcoin.security.hideProcessInformation = true;
 
     # Tor
     services.tor = {
@@ -227,6 +226,7 @@ in {
       isNormalUser = true;
       extraGroups = [
           "systemd-journal"
+          "proc" # Enable full /proc access and systemd-status
           cfg.bitcoind.group
         ]
         ++ (optionals cfg.clightning.enable [ "clightning" ])

modules/security.nix

@@ -0,0 +1,39 @@
+{ config, lib, pkgs, options, ... }:
+
+{
+  options = {
+    nix-bitcoin.security.hideProcessInformation = options.security.hideProcessInformation;
+  };
+
+  config = lib.mkIf config.nix-bitcoin.security.hideProcessInformation {
+    # Only show the current user's processes in /proc.
+    # Users with group 'proc' can still access all processes.
+    security.hideProcessInformation = true;
+
+    # This mitigates a systemd security issue leaking (sub)process
+    # command lines.
+    # Only allow users with group 'proc' to retrieve systemd unit information like
+    # cgroup paths (i.e. (sub)process command lines) via D-Bus.
+    # This D-Bus call is used by `systemctl status`.
+    services.dbus.packages = lib.mkAfter [ # Apply at the end to override the default policy
+      (pkgs.writeTextDir "etc/dbus-1/system.d/dbus.conf" ''
+        <busconfig>
+          <policy context="default">
+            <deny
+              send_destination="org.freedesktop.systemd1"
+              send_interface="org.freedesktop.systemd1.Manager"
+              send_member="GetUnitProcesses"
+            />
+          </policy>
+          <policy group="proc">
+            <allow
+              send_destination="org.freedesktop.systemd1"
+              send_interface="org.freedesktop.systemd1.Manager"
+              send_member="GetUnitProcesses"
+            />
+          </policy>
+        </busconfig>
+      '')
+    ];
+  };
+}

test/scenarios/lib.py

@@ -9,7 +9,7 @@ def assert_matches(cmd, regexp):
         raise Exception(f"Pattern '{regexp}' not found in '{out}'")
 
 
-def assert_matches_exactly(cmd, regexp):
+def assert_full_match(cmd, regexp):
     out = succeed(cmd)
     if not re.fullmatch(regexp, out):
         raise Exception(f"Pattern '{regexp}' doesn't match '{out}'")
@@ -38,9 +38,7 @@ if "is_interactive" in vars():
 # The argument extra_tests is a dictionary from strings to functions. The string
 # determines at which point of run_tests the corresponding function is executed.
 def run_tests(extra_tests):
-    assert_running("setup-secrets")
-    # Unused secrets should be inaccessible
-    succeed('[[ $(stat -c "%U:%G %a" /secrets/dummy) = "root:root 440" ]]')
+    test_security()
 
     assert_running("bitcoind")
     machine.wait_until_succeeds("bitcoin-cli getnetworkinfo")
@@ -103,13 +101,6 @@ def run_tests(extra_tests):
     machine.wait_until_succeeds(log_has_string("bitcoind-import-banlist", "Importing node banlist"))
     assert_no_failure("bitcoind-import-banlist")
 
-    # test that `systemctl status` can't leak credentials
-    assert_matches(
-        "sudo -u electrs systemctl status clightning 2>&1 >/dev/null",
-        "Failed to dump process list for 'clightning.service', ignoring: Access denied",
-    )
-    machine.succeed("grep -Fq hidepid=2 /proc/mounts")
-
     ### Additional tests
 
     # Current time in µs
@@ -155,3 +146,21 @@ def run_tests(extra_tests):
 
     ### Check that all extra_tests have been run
     assert len(extra_tests) == 0
+
+
+def test_security():
+    assert_running("setup-secrets")
+    # Unused secrets should be inaccessible
+    succeed('[[ $(stat -c "%U:%G %a" /secrets/dummy) = "root:root 440" ]]')
+
+    # Access to '/proc' should be restricted
+    machine.succeed("grep -Fq hidepid=2 /proc/mounts")
+
+    machine.wait_for_unit("bitcoind")
+    # `systemctl status` run by unprivileged users shouldn't leak cgroup info
+    assert_matches(
+        "sudo -u electrs systemctl status bitcoind 2>&1 >/dev/null",
+        "Failed to dump process list for 'bitcoind.service', ignoring: Access denied",
+    )
+    # The 'operator' with group 'proc' has full access
+    assert_full_match("sudo -u operator systemctl status bitcoind 2>&1 >/dev/null", "")

test/scenarios/withnetns.py

@@ -84,7 +84,7 @@ def prestop():
     machine.fail("netns-exec nb-electrs ip a")
 
     # test that netns-exec drops capabilities
-    assert_matches_exactly(
+    assert_full_match(
         "su operator -c 'netns-exec nb-bitcoind capsh --print | grep Current '", "Current: =\n"
     )