From 73674467616109806c8501f1357b699fadc9b342 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Thu, 20 Aug 2020 13:11:05 +0200 Subject: [PATCH 1/6] test: rename assert_matches_exactly -> assert_full_match More precise, needed in a later commit. --- test/scenarios/lib.py | 2 +- test/scenarios/withnetns.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/scenarios/lib.py b/test/scenarios/lib.py index 2989fec..78c28a8 100644 --- a/test/scenarios/lib.py +++ b/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}'") diff --git a/test/scenarios/withnetns.py b/test/scenarios/withnetns.py index 7534a14..6b5df81 100644 --- a/test/scenarios/withnetns.py +++ b/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" ) From 343e026030751f97bd8a364dbf3d88515178171f Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Thu, 20 Aug 2020 13:11:06 +0200 Subject: [PATCH 2/6] rename dbus.nix -> security.nix This file has a broader scope than just configuring dbus. --- modules/modules.nix | 2 +- modules/{dbus.nix => security.nix} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename modules/{dbus.nix => security.nix} (100%) diff --git a/modules/modules.nix b/modules/modules.nix index 3625c1e..1b6e253 100644 --- a/modules/modules.nix +++ b/modules/modules.nix @@ -16,7 +16,7 @@ ./lightning-loop.nix ./secrets/secrets.nix ./netns-isolation.nix - ./dbus.nix + ./security.nix ./backups.nix ]; diff --git a/modules/dbus.nix b/modules/security.nix similarity index 100% rename from modules/dbus.nix rename to modules/security.nix From 96ea2e671ca303d25b74a6e92848de3c929a7906 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Thu, 20 Aug 2020 13:11:07 +0200 Subject: [PATCH 3/6] security: simplify and fix dbus configuration Previously, due to the dependency on a helper service, this dbus config was initially inactive after system boot, allowing for unrestricted use of the problematic dbus call. This also broke the accompanying VM test on faster systems. Remove 'allow' policy for root because it's a no-op: 1. It's overridden by the 'mandatory' deny policy. 2. Root can use all dbus calls anyways, regardless of policy settings. Also, add some comments. --- modules/security.nix | 50 ++++++++++---------------------------------- 1 file changed, 11 insertions(+), 39 deletions(-) diff --git a/modules/security.nix b/modules/security.nix index 000b0ff..89efc66 100644 --- a/modules/security.nix +++ b/modules/security.nix @@ -1,55 +1,27 @@ { config, lib, pkgs, ... }: -with lib; +{ + # Only show the current user's processes in /proc. + # Users with group 'proc' can still access all processes. + security.hideProcessInformation = true; -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" '' + # This mitigates a systemd security issue leaking (sub)process + # command lines. + # Only allow root 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 = [ (pkgs.writeTextDir "etc/dbus-1/system.d/dbus.conf" '' - - - - - - ''; -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}"; - }; - }; - }; + '') ]; } From 588a0b240515f7c104914d5b20e3fc5fc68e2a69 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Thu, 20 Aug 2020 13:11:08 +0200 Subject: [PATCH 4/6] security: enable full systemd-status for group 'proc' Previously, systemd-status was broken for all users except root. Use a 'default' deny policy, which is overridden for group 'proc'. Add operator to group 'proc'. Also, remove redundant XML boilerplate. --- modules/presets/secure-node.nix | 1 + modules/security.nix | 36 +++++++++++++++++++-------------- test/scenarios/lib.py | 4 +++- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/modules/presets/secure-node.nix b/modules/presets/secure-node.nix index 374dd52..7933487 100644 --- a/modules/presets/secure-node.nix +++ b/modules/presets/secure-node.nix @@ -227,6 +227,7 @@ in { isNormalUser = true; extraGroups = [ "systemd-journal" + "proc" # Enable full /proc access and systemd-status cfg.bitcoind.group ] ++ (optionals cfg.clightning.enable [ "clightning" ]) diff --git a/modules/security.nix b/modules/security.nix index 89efc66..f4d2c78 100644 --- a/modules/security.nix +++ b/modules/security.nix @@ -7,21 +7,27 @@ # This mitigates a systemd security issue leaking (sub)process # command lines. - # Only allow root to retrieve systemd unit information like + # 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 = [ (pkgs.writeTextDir "etc/dbus-1/system.d/dbus.conf" '' - - - - - - - - - '') ]; + services.dbus.packages = lib.mkAfter [ # Apply at the end to override the default policy + (pkgs.writeTextDir "etc/dbus-1/system.d/dbus.conf" '' + + + + + + + + + '') + ]; } diff --git a/test/scenarios/lib.py b/test/scenarios/lib.py index 78c28a8..2ebda30 100644 --- a/test/scenarios/lib.py +++ b/test/scenarios/lib.py @@ -103,11 +103,13 @@ 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 + # `systemctl status` run by unprivileged users shouldn't leak cgroup info assert_matches( "sudo -u electrs systemctl status clightning 2>&1 >/dev/null", "Failed to dump process list for 'clightning.service', ignoring: Access denied", ) + # The 'operator' with group 'proc' has full access + assert_full_match("sudo -u operator systemctl status clightning 2>&1 >/dev/null", "") machine.succeed("grep -Fq hidepid=2 /proc/mounts") ### Additional tests From a36789b4685cad40725055f8e0a396fec7e1a03c Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Thu, 20 Aug 2020 13:11:09 +0200 Subject: [PATCH 5/6] test: move security tests to separate function --- test/scenarios/lib.py | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/test/scenarios/lib.py b/test/scenarios/lib.py index 2ebda30..bffd4bb 100644 --- a/test/scenarios/lib.py +++ b/test/scenarios/lib.py @@ -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,15 +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") - # `systemctl status` run by unprivileged users shouldn't leak cgroup info - assert_matches( - "sudo -u electrs systemctl status clightning 2>&1 >/dev/null", - "Failed to dump process list for 'clightning.service', ignoring: Access denied", - ) - # The 'operator' with group 'proc' has full access - assert_full_match("sudo -u operator systemctl status clightning 2>&1 >/dev/null", "") - machine.succeed("grep -Fq hidepid=2 /proc/mounts") - ### Additional tests # Current time in µs @@ -157,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", "") From 44de5064cd9f8ae625997955820146b38afedf90 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Thu, 20 Aug 2020 13:11:10 +0200 Subject: [PATCH 6/6] security: don't restrict process info by default for module users --- modules/presets/secure-node.nix | 3 +- modules/security.nix | 64 ++++++++++++++++++--------------- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/modules/presets/secure-node.nix b/modules/presets/secure-node.nix index 7933487..133f649 100644 --- a/modules/presets/secure-node.nix +++ b/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 = { diff --git a/modules/security.nix b/modules/security.nix index f4d2c78..cd5ad4e 100644 --- a/modules/security.nix +++ b/modules/security.nix @@ -1,33 +1,39 @@ -{ config, lib, pkgs, ... }: +{ config, lib, pkgs, options, ... }: { - # Only show the current user's processes in /proc. - # Users with group 'proc' can still access all processes. - security.hideProcessInformation = true; + options = { + nix-bitcoin.security.hideProcessInformation = options.security.hideProcessInformation; + }; - # 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" '' - - - - - - - - - '') - ]; + 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" '' + + + + + + + + + '') + ]; + }; }