From 0a5542c766cc14b3a6c841d0f47ab098605776e2 Mon Sep 17 00:00:00 2001 From: Julien Moutinho Date: Sat, 17 Feb 2024 15:48:10 +0100 Subject: [PATCH 1/7] nixos/systemd-confinement: support ProtectSystem=/DynamicUser= See https://discourse.nixos.org/t/hardening-systemd-services/17147/14 --- .../manual/release-notes/rl-2405.section.md | 2 + .../modules/security/systemd-confinement.nix | 35 ++-- nixos/tests/systemd-confinement.nix | 166 +++++++++++++++++- 3 files changed, 181 insertions(+), 22 deletions(-) diff --git a/nixos/doc/manual/release-notes/rl-2405.section.md b/nixos/doc/manual/release-notes/rl-2405.section.md index a43e8f26cabe..a756751ea2a0 100644 --- a/nixos/doc/manual/release-notes/rl-2405.section.md +++ b/nixos/doc/manual/release-notes/rl-2405.section.md @@ -713,6 +713,8 @@ The pre-existing [services.ankisyncd](#opt-services.ankisyncd.enable) has been m - `documentation.man.mandoc` now by default uses `MANPATH` to set the directories where mandoc will search for manual pages. This enables mandoc to find manual pages in Nix profiles. To set the manual search paths via the `mandoc.conf` configuration file like before, use `documentation.man.mandoc.settings.manpath` instead. +- The `systemd-confinement` module extension is now compatible with `DynamicUser=true` and thus `ProtectSystem=strict` too. + - `grafana-loki` package was updated to 3.0.0 which includes [breaking changes](https://github.com/grafana/loki/releases/tag/v3.0.0). - `programs.fish.package` now allows you to override the package used in the `fish` module. diff --git a/nixos/modules/security/systemd-confinement.nix b/nixos/modules/security/systemd-confinement.nix index 0304749b8d10..ed33c41c79ae 100644 --- a/nixos/modules/security/systemd-confinement.nix +++ b/nixos/modules/security/systemd-confinement.nix @@ -79,13 +79,20 @@ in { description = '' The value `full-apivfs` (the default) sets up private {file}`/dev`, {file}`/proc`, - {file}`/sys` and {file}`/tmp` file systems in a separate user - name space. + {file}`/sys`, {file}`/tmp` and {file}`/var/tmp` file systems + in a separate user name space. If this is set to `chroot-only`, only the file system name space is set up along with the call to {manpage}`chroot(2)`. + In all cases, unless `serviceConfig.PrivateTmp=true` is set, + both {file}`/tmp` and {file}`/var/tmp` paths are added to `InaccessiblePaths=`. + This is to overcome options like `DynamicUser=true` + implying `PrivateTmp=true` without letting it being turned off. + Beware however that giving processes the `CAP_SYS_ADMIN` and `@mount` privileges + can let them undo the effects of `InaccessiblePaths=`. + ::: {.note} This doesn't cover network namespaces and is solely for file system level isolation. @@ -98,8 +105,11 @@ in { wantsAPIVFS = lib.mkDefault (config.confinement.mode == "full-apivfs"); in lib.mkIf config.confinement.enable { serviceConfig = { - RootDirectory = "/var/empty"; - TemporaryFileSystem = "/"; + RuntimeDirectory = [ "confinement/${mkPathSafeName name}" ]; + RootDirectory = lib.mkDefault "/run/confinement/${mkPathSafeName name}"; + InaccessiblePaths = [ + "-+/run/confinement/${mkPathSafeName name}" + ]; PrivateMounts = lib.mkDefault true; # https://github.com/NixOS/nixpkgs/issues/14645 is a future attempt @@ -148,16 +158,6 @@ in { + " Please either define a separate service or find a way to run" + " commands other than ExecStart within the chroot."; } - { assertion = !cfg.serviceConfig.DynamicUser or false; - message = "${whatOpt "DynamicUser"}. Please create a dedicated user via" - + " the 'users.users' option instead as this combination is" - + " currently not supported."; - } - { assertion = cfg.serviceConfig ? ProtectSystem -> cfg.serviceConfig.ProtectSystem == false; - message = "${whatOpt "ProtectSystem"}. ProtectSystem is not compatible" - + " with service confinement as it fails to remount /usr within" - + " our chroot. Please disable the option."; - } ]) config.systemd.services); config.systemd.packages = lib.concatLists (lib.mapAttrsToList (name: cfg: let @@ -183,6 +183,13 @@ in { echo "BindReadOnlyPaths=$realprog:/bin/sh" >> "$serviceFile" ''} + # If DynamicUser= is enabled, PrivateTmp=true is implied (and cannot be turned off). + # so disable them unless PrivateTmp=true is explicitely set. + ${lib.optionalString (!cfg.serviceConfig.PrivateTmp) '' + echo "InaccessiblePaths=-+/tmp" >> "$serviceFile" + echo "InaccessiblePaths=-+/var/tmp" >> "$serviceFile" + ''} + while read storePath; do if [ -L "$storePath" ]; then # Currently, systemd can't cope with symlinks in Bind(ReadOnly)Paths, diff --git a/nixos/tests/systemd-confinement.nix b/nixos/tests/systemd-confinement.nix index bde5b770ea50..a4a0bf57f1cf 100644 --- a/nixos/tests/systemd-confinement.nix +++ b/nixos/tests/systemd-confinement.nix @@ -4,7 +4,7 @@ import ./make-test-python.nix { nodes.machine = { pkgs, lib, ... }: let testServer = pkgs.writeScript "testserver.sh" '' #!${pkgs.runtimeShell} - export PATH=${lib.escapeShellArg "${pkgs.coreutils}/bin"} + export PATH=${lib.makeBinPath [ pkgs.coreutils pkgs.findutils ]} ${lib.escapeShellArg pkgs.runtimeShell} 2>&1 echo "exit-status:$?" ''; @@ -48,8 +48,14 @@ import ./make-test-python.nix { { config.confinement.mode = "chroot-only"; testScript = '' with subtest("chroot-only confinement"): - paths = machine.succeed('chroot-exec ls -1 / | paste -sd,').strip() - assert_eq(paths, "bin,nix,run") + # chroot-exec starts a socket-activated service, + # but, upon starting, a systemd system service + # calls setup_namespace() which calls base_filesystem_create() + # which creates some usual top level directories. + # In chroot-only mode, without additional BindPaths= or the like, + # they must be empty and thus removable by rmdir. + paths = machine.succeed('chroot-exec rmdir /dev /etc /proc /root /sys /usr /var "&&" ls -Am /').strip() + assert_eq(paths, "bin, nix, run") uid = machine.succeed('chroot-exec id -u').strip() assert_eq(uid, "0") machine.succeed("chroot-exec chown 65534 /bin") @@ -57,7 +63,7 @@ import ./make-test-python.nix { } { testScript = '' with subtest("full confinement with APIVFS"): - machine.fail("chroot-exec ls -l /etc") + machine.succeed('chroot-exec rmdir /etc') machine.fail("chroot-exec chown 65534 /bin") assert_eq(machine.succeed('chroot-exec id -u').strip(), "0") machine.succeed("chroot-exec chown 0 /bin") @@ -80,6 +86,146 @@ import ./make-test-python.nix { machine.fail("chroot-exec touch /bin/test") ''; } + { config.confinement.mode = "full-apivfs"; + config.serviceConfig.DynamicUser = true; + testScript = '' + with subtest("check if DynamicUser is working in full-apivfs mode"): + machine.succeed("chroot-exec ls -l /dev") + paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + """ + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/host + /run/host/.os-release-stage + /run/host/.os-release-stage/os-release + /run/host/os-release + /run/systemd + /run/systemd/incoming + /sys + /tmp + /usr + /var + /var/tmp + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied""" + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") + # DynamicUser=true implies ProtectSystem=strict + machine.fail("chroot-exec touch /etc/test") + ''; + } + { config.confinement.mode = "full-apivfs"; + config.serviceConfig.DynamicUser = true; + config.serviceConfig.PrivateTmp = false; + testScript = '' + with subtest("check if DynamicUser and PrivateTmp=false are working in full-apivfs mode"): + machine.succeed("chroot-exec ls -l /dev") + paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + """ + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/host + /run/host/.os-release-stage + /run/host/.os-release-stage/os-release + /run/host/os-release + /run/systemd + /run/systemd/incoming + /sys + /usr + /var + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied""" + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") + # DynamicUser=true implies ProtectSystem=strict + machine.fail("chroot-exec touch /etc/test") + ''; + } + { config.confinement.mode = "chroot-only"; + config.serviceConfig.DynamicUser = true; + testScript = '' + with subtest("check if DynamicUser is working in chroot-only mode"): + paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + """ + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/systemd + /run/systemd/incoming + /sys + /usr + /var + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied""" + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") + ''; + } + { config.confinement.mode = "chroot-only"; + config.serviceConfig.DynamicUser = true; + config.serviceConfig.PrivateTmp = true; + testScript = '' + with subtest("check if DynamicUser and PrivateTmp=true are working in chroot-only mode"): + paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + """ + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/systemd + /run/systemd/incoming + /sys + /tmp + /usr + /var + /var/tmp + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied""" + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") + ''; + } (let symlink = pkgs.runCommand "symlink" { target = pkgs.writeText "symlink-target" "got me\n"; @@ -88,7 +234,7 @@ import ./make-test-python.nix { config.confinement.packages = lib.singleton symlink; testScript = '' with subtest("check if symlinks are properly bind-mounted"): - machine.fail("chroot-exec test -e /etc") + machine.succeed("chroot-exec rmdir /etc") text = machine.succeed('chroot-exec cat ${symlink}').strip() assert_eq(text, "got me") ''; @@ -176,9 +322,13 @@ import ./make-test-python.nix { }; testScript = { nodes, ... }: '' - def assert_eq(a, b): - assert a == b, f"{a} != {b}" + import difflib + def assert_eq(got, expected): + if got != expected: + diff = difflib.unified_diff(got.splitlines(keepends=True), expected.splitlines(keepends=True)) + print("".join(diff)) + assert got == expected, f"{got} != {expected}" machine.wait_for_unit("multi-user.target") - '' + nodes.machine.config.__testSteps; + '' + nodes.machine.__testSteps; } From ba31b3753e21f64a42d0b94f722752137dd07974 Mon Sep 17 00:00:00 2001 From: aszlig Date: Tue, 23 Apr 2024 09:53:19 +0200 Subject: [PATCH 2/7] nixos/tests/confinement: Re-add description attr The reason why I originally used the "description" attribute was that it can be easily used to parametrise the tests so that we can specify common constraints and apply it across a number of different configurations. When porting the tests to Python, the description attribute was replaced by inlining it into the Python code, most probably because it was easier to do in bulk since using Nix to generate the subtest parts would be very complicated to do since we also had to please Black (a Python code formatter that we no longer use in test scripts). Since we now also want to support DynamicUser in systemd-confinement, the need to parametrise the tests became apparent again because it's now easier to refactor our subtests to run both with *and* without DynamicUser set to true. Signed-off-by: aszlig --- nixos/tests/systemd-confinement.nix | 407 ++++++++++++++-------------- 1 file changed, 209 insertions(+), 198 deletions(-) diff --git a/nixos/tests/systemd-confinement.nix b/nixos/tests/systemd-confinement.nix index a4a0bf57f1cf..184c05b79274 100644 --- a/nixos/tests/systemd-confinement.nix +++ b/nixos/tests/systemd-confinement.nix @@ -18,6 +18,7 @@ import ./make-test-python.nix { ''; mkTestStep = num: { + description, testScript, config ? {}, serviceName ? "test${toString num}", @@ -38,192 +39,198 @@ import ./make-test-python.nix { }; } // removeAttrs config [ "confinement" "serviceConfig" ]; - __testSteps = lib.mkOrder num ('' - machine.succeed("echo ${toString num} > /teststep") - '' + testScript); + __testSteps = lib.mkOrder num '' + with subtest('${lib.escape ["'" "\\"] description}'): + machine.succeed("echo ${toString num} > /teststep") + ${lib.replaceStrings ["\n"] ["\n "] testScript} + ''; }; in { imports = lib.imap1 mkTestStep [ - { config.confinement.mode = "chroot-only"; + { description = "chroot-only confinement"; + config.confinement.mode = "chroot-only"; testScript = '' - with subtest("chroot-only confinement"): - # chroot-exec starts a socket-activated service, - # but, upon starting, a systemd system service - # calls setup_namespace() which calls base_filesystem_create() - # which creates some usual top level directories. - # In chroot-only mode, without additional BindPaths= or the like, - # they must be empty and thus removable by rmdir. - paths = machine.succeed('chroot-exec rmdir /dev /etc /proc /root /sys /usr /var "&&" ls -Am /').strip() - assert_eq(paths, "bin, nix, run") - uid = machine.succeed('chroot-exec id -u').strip() - assert_eq(uid, "0") - machine.succeed("chroot-exec chown 65534 /bin") + # chroot-exec starts a socket-activated service, + # but, upon starting, a systemd system service + # calls setup_namespace() which calls base_filesystem_create() + # which creates some usual top level directories. + # In chroot-only mode, without additional BindPaths= or the like, + # they must be empty and thus removable by rmdir. + paths = machine.succeed('chroot-exec rmdir /dev /etc /proc /root /sys /usr /var "&&" ls -Am /').strip() + assert_eq(paths, "bin, nix, run") + uid = machine.succeed('chroot-exec id -u').strip() + assert_eq(uid, "0") + machine.succeed("chroot-exec chown 65534 /bin") ''; } - { testScript = '' - with subtest("full confinement with APIVFS"): - machine.succeed('chroot-exec rmdir /etc') - machine.fail("chroot-exec chown 65534 /bin") - assert_eq(machine.succeed('chroot-exec id -u').strip(), "0") - machine.succeed("chroot-exec chown 0 /bin") - ''; - } - { config.serviceConfig.BindReadOnlyPaths = [ "/etc" ]; + { description = "full confinement with APIVFS"; testScript = '' - with subtest("check existence of bind-mounted /etc"): - passwd = machine.succeed('chroot-exec cat /etc/passwd').strip() - assert len(passwd) > 0, "/etc/passwd must not be empty" + machine.succeed('chroot-exec rmdir /etc') + machine.fail("chroot-exec chown 65534 /bin") + assert_eq(machine.succeed('chroot-exec id -u').strip(), "0") + machine.succeed("chroot-exec chown 0 /bin") ''; } - { config.serviceConfig.User = "chroot-testuser"; + { description = "check existence of bind-mounted /etc"; + config.serviceConfig.BindReadOnlyPaths = [ "/etc" ]; + testScript = '' + passwd = machine.succeed('chroot-exec cat /etc/passwd').strip() + assert len(passwd) > 0, "/etc/passwd must not be empty" + ''; + } + { description = "check if User/Group really runs as non-root"; + config.serviceConfig.User = "chroot-testuser"; config.serviceConfig.Group = "chroot-testgroup"; testScript = '' - with subtest("check if User/Group really runs as non-root"): - machine.succeed("chroot-exec ls -l /dev") - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of chroot-testuser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") + machine.succeed("chroot-exec ls -l /dev") + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of chroot-testuser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") ''; } - { config.confinement.mode = "full-apivfs"; + { description = "check if DynamicUser is working in full-apivfs mode"; + config.confinement.mode = "full-apivfs"; config.serviceConfig.DynamicUser = true; testScript = '' - with subtest("check if DynamicUser is working in full-apivfs mode"): - machine.succeed("chroot-exec ls -l /dev") - paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - """ - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/host - /run/host/.os-release-stage - /run/host/.os-release-stage/os-release - /run/host/os-release - /run/systemd - /run/systemd/incoming - /sys - /tmp - /usr - /var - /var/tmp - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied""" - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") - # DynamicUser=true implies ProtectSystem=strict - machine.fail("chroot-exec touch /etc/test") + machine.succeed("chroot-exec ls -l /dev") + paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + dedent(""" + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/host + /run/host/.os-release-stage + /run/host/.os-release-stage/os-release + /run/host/os-release + /run/systemd + /run/systemd/incoming + /sys + /tmp + /usr + /var + /var/tmp + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied + """).rstrip() + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") + # DynamicUser=true implies ProtectSystem=strict + machine.fail("chroot-exec touch /etc/test") ''; } - { config.confinement.mode = "full-apivfs"; + { description = "check if DynamicUser and PrivateTmp=false are working in full-apivfs mode"; + config.confinement.mode = "full-apivfs"; config.serviceConfig.DynamicUser = true; config.serviceConfig.PrivateTmp = false; testScript = '' - with subtest("check if DynamicUser and PrivateTmp=false are working in full-apivfs mode"): - machine.succeed("chroot-exec ls -l /dev") - paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - """ - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/host - /run/host/.os-release-stage - /run/host/.os-release-stage/os-release - /run/host/os-release - /run/systemd - /run/systemd/incoming - /sys - /usr - /var - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied""" - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") - # DynamicUser=true implies ProtectSystem=strict - machine.fail("chroot-exec touch /etc/test") + machine.succeed("chroot-exec ls -l /dev") + paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + dedent(""" + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/host + /run/host/.os-release-stage + /run/host/.os-release-stage/os-release + /run/host/os-release + /run/systemd + /run/systemd/incoming + /sys + /usr + /var + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied + """).rstrip() + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") + # DynamicUser=true implies ProtectSystem=strict + machine.fail("chroot-exec touch /etc/test") ''; } - { config.confinement.mode = "chroot-only"; + { description = "check if DynamicUser is working in chroot-only mode"; + config.confinement.mode = "chroot-only"; config.serviceConfig.DynamicUser = true; testScript = '' - with subtest("check if DynamicUser is working in chroot-only mode"): - paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - """ - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/systemd - /run/systemd/incoming - /sys - /usr - /var - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied""" - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") + paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + dedent(""" + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/systemd + /run/systemd/incoming + /sys + /usr + /var + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied + """).rstrip() + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") ''; } - { config.confinement.mode = "chroot-only"; + { description = "check if DynamicUser and PrivateTmp=true are working in chroot-only mode"; + config.confinement.mode = "chroot-only"; config.serviceConfig.DynamicUser = true; config.serviceConfig.PrivateTmp = true; testScript = '' - with subtest("check if DynamicUser and PrivateTmp=true are working in chroot-only mode"): - paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - """ - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/systemd - /run/systemd/incoming - /sys - /tmp - /usr - /var - /var/tmp - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied""" - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") + paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') + assert_eq( + '\n'.join(sorted(paths.split('\n'))), + dedent(""" + / + /bin + /bin/sh + /dev + /etc + /nix + /proc + /root + /run + /run/systemd + /run/systemd/incoming + /sys + /tmp + /usr + /var + /var/tmp + find: '/root': Permission denied + find: '/run/systemd/incoming': Permission denied + """).rstrip() + ) + uid = machine.succeed('chroot-exec id -u').strip() + assert uid != "0", "UID of a DynamicUser shouldn't be 0" + machine.fail("chroot-exec touch /bin/test") ''; } (let @@ -231,68 +238,70 @@ import ./make-test-python.nix { target = pkgs.writeText "symlink-target" "got me\n"; } "ln -s \"$target\" \"$out\""; in { + description = "check if symlinks are properly bind-mounted"; config.confinement.packages = lib.singleton symlink; testScript = '' - with subtest("check if symlinks are properly bind-mounted"): - machine.succeed("chroot-exec rmdir /etc") - text = machine.succeed('chroot-exec cat ${symlink}').strip() - assert_eq(text, "got me") + machine.succeed("chroot-exec rmdir /etc") + text = machine.succeed('chroot-exec cat ${symlink}').strip() + assert_eq(text, "got me") ''; }) - { config.serviceConfig.User = "chroot-testuser"; + { description = "check if StateDirectory works"; + config.serviceConfig.User = "chroot-testuser"; config.serviceConfig.Group = "chroot-testgroup"; config.serviceConfig.StateDirectory = "testme"; testScript = '' - with subtest("check if StateDirectory works"): - machine.succeed("chroot-exec touch /tmp/canary") - machine.succeed('chroot-exec "echo works > /var/lib/testme/foo"') - machine.succeed('test "$(< /var/lib/testme/foo)" = works') - machine.succeed("test ! -e /tmp/canary") + machine.succeed("chroot-exec touch /tmp/canary") + machine.succeed('chroot-exec "echo works > /var/lib/testme/foo"') + machine.succeed('test "$(< /var/lib/testme/foo)" = works') + machine.succeed("test ! -e /tmp/canary") ''; } - { testScript = '' - with subtest("check if /bin/sh works"): - machine.succeed( - "chroot-exec test -e /bin/sh", - 'test "$(chroot-exec \'/bin/sh -c "echo bar"\')" = bar', - ) - ''; - } - { config.confinement.binSh = null; + { description = "check if /bin/sh works"; testScript = '' - with subtest("check if suppressing /bin/sh works"): - machine.succeed("chroot-exec test ! -e /bin/sh") - machine.succeed('test "$(chroot-exec \'/bin/sh -c "echo foo"\')" != foo') + machine.succeed( + "chroot-exec test -e /bin/sh", + 'test "$(chroot-exec \'/bin/sh -c "echo bar"\')" = bar', + ) ''; } - { config.confinement.binSh = "${pkgs.hello}/bin/hello"; + { description = "check if suppressing /bin/sh works"; + config.confinement.binSh = null; testScript = '' - with subtest("check if we can set /bin/sh to something different"): - machine.succeed("chroot-exec test -e /bin/sh") - machine.succeed('test "$(chroot-exec /bin/sh -g foo)" = foo') + machine.succeed( + 'chroot-exec test ! -e /bin/sh', + 'test "$(chroot-exec \'/bin/sh -c "echo foo"\')" != foo', + ) ''; } - { config.environment.FOOBAR = pkgs.writeText "foobar" "eek\n"; + { description = "check if we can set /bin/sh to something different"; + config.confinement.binSh = "${pkgs.hello}/bin/hello"; testScript = '' - with subtest("check if only Exec* dependencies are included"): - machine.succeed('test "$(chroot-exec \'cat "$FOOBAR"\')" != eek') + machine.succeed("chroot-exec test -e /bin/sh") + machine.succeed('test "$(chroot-exec /bin/sh -g foo)" = foo') ''; } - { config.environment.FOOBAR = pkgs.writeText "foobar" "eek\n"; + { description = "check if only Exec* dependencies are included"; + config.environment.FOOBAR = pkgs.writeText "foobar" "eek\n"; + testScript = '' + machine.succeed('test "$(chroot-exec \'cat "$FOOBAR"\')" != eek') + ''; + } + { description = "check if all unit dependencies are included"; + config.environment.FOOBAR = pkgs.writeText "foobar" "eek\n"; config.confinement.fullUnit = true; testScript = '' - with subtest("check if all unit dependencies are included"): - machine.succeed('test "$(chroot-exec \'cat "$FOOBAR"\')" = eek') + machine.succeed('test "$(chroot-exec \'cat "$FOOBAR"\')" = eek') ''; } - { serviceName = "shipped-unitfile"; + { description = "check if shipped unit file still works"; + serviceName = "shipped-unitfile"; config.confinement.mode = "chroot-only"; testScript = '' - with subtest("check if shipped unit file still works"): - machine.succeed( - 'chroot-exec \'kill -9 $$ 2>&1 || :\' | ' - 'grep -q "Too many levels of symbolic links"' - ) + machine.succeed( + 'chroot-exec \'kill -9 $$ 2>&1 || :\' | ' + 'grep -q "Too many levels of symbolic links"' + ) ''; } ]; @@ -322,12 +331,14 @@ import ./make-test-python.nix { }; testScript = { nodes, ... }: '' + from textwrap import dedent import difflib + def assert_eq(got, expected): - if got != expected: - diff = difflib.unified_diff(got.splitlines(keepends=True), expected.splitlines(keepends=True)) - print("".join(diff)) - assert got == expected, f"{got} != {expected}" + if got != expected: + diff = difflib.unified_diff(got.splitlines(keepends=True), expected.splitlines(keepends=True)) + print("".join(diff)) + assert got == expected, f"{got} != {expected}" machine.wait_for_unit("multi-user.target") '' + nodes.machine.__testSteps; From f7d026b4312a3f50c44d97be32b0669e8fad2a76 Mon Sep 17 00:00:00 2001 From: aszlig Date: Wed, 24 Apr 2024 15:04:07 +0200 Subject: [PATCH 3/7] nixos/tests/confinement: Move to dedicated dir When experimenting on ways how to refactor the test, I wrote a significant enough amount of Python to warrant a dedicated Python file. This commit is mainly to prepare for that and make it easier to track renames. Signed-off-by: aszlig --- nixos/tests/all-tests.nix | 2 +- .../default.nix} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename nixos/tests/{systemd-confinement.nix => systemd-confinement/default.nix} (99%) diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix index f9e81f2bbd8d..a8f8c470e6e2 100644 --- a/nixos/tests/all-tests.nix +++ b/nixos/tests/all-tests.nix @@ -885,7 +885,7 @@ in { systemd-binfmt = handleTestOn ["x86_64-linux"] ./systemd-binfmt.nix {}; systemd-boot = handleTest ./systemd-boot.nix {}; systemd-bpf = handleTest ./systemd-bpf.nix {}; - systemd-confinement = handleTest ./systemd-confinement.nix {}; + systemd-confinement = handleTest ./systemd-confinement {}; systemd-coredump = handleTest ./systemd-coredump.nix {}; systemd-cryptenroll = handleTest ./systemd-cryptenroll.nix {}; systemd-credentials-tpm2 = handleTest ./systemd-credentials-tpm2.nix {}; diff --git a/nixos/tests/systemd-confinement.nix b/nixos/tests/systemd-confinement/default.nix similarity index 99% rename from nixos/tests/systemd-confinement.nix rename to nixos/tests/systemd-confinement/default.nix index 184c05b79274..815cb3a3bbdd 100644 --- a/nixos/tests/systemd-confinement.nix +++ b/nixos/tests/systemd-confinement/default.nix @@ -1,4 +1,4 @@ -import ./make-test-python.nix { +import ../make-test-python.nix { name = "systemd-confinement"; nodes.machine = { pkgs, lib, ... }: let From 51d3f3475c2fb35f0d9682ea600066b1cea459c6 Mon Sep 17 00:00:00 2001 From: aszlig Date: Wed, 24 Apr 2024 19:11:06 +0200 Subject: [PATCH 4/7] nixos/tests/confinement: Run test probes in Python So far the architecture for the tests was that we would use a systemd socket unit using the Accept option to start a small shell process where we can pipe commands into by connecting to the socket created by the socket unit. This is unnecessary since we can directly use the code snippets from the individual subtests and systemd will take care of checking the return code in case we get any assertions[^1]. Another advantage of this is that tests now run in parallel, so we can do rather expensive things such as looking in /nix to see whether anything is writable. The new assert_permissions() function is the main driver behind this and allows for a more fine-grained way to check whether we got the right permissions whilst also ignoring irrelevant things such as read-only empty directories. Our previous approach also just did a read-only check, which might be fine in full-apivfs mode where the attack surface already is large, but in chroot-only mode we really want to make sure nothing is every writable. A downside of the new approach is that currently the unit names are numbered via lib.imap1, which makes it annoying to track its definition. [^1]: Speaking of assertions, I wrapped the code to be run with pytest's assertion rewriting, so that we get more useful AssertionErrors. Signed-off-by: aszlig --- nixos/tests/systemd-confinement/checkperms.py | 187 ++++++++ nixos/tests/systemd-confinement/default.nix | 431 +++++++++--------- 2 files changed, 397 insertions(+), 221 deletions(-) create mode 100644 nixos/tests/systemd-confinement/checkperms.py diff --git a/nixos/tests/systemd-confinement/checkperms.py b/nixos/tests/systemd-confinement/checkperms.py new file mode 100644 index 000000000000..3c7ba279a3d2 --- /dev/null +++ b/nixos/tests/systemd-confinement/checkperms.py @@ -0,0 +1,187 @@ +import errno +import os + +from enum import IntEnum +from pathlib import Path + + +class Accessibility(IntEnum): + """ + The level of accessibility we have on a file or directory. + + This is needed to assess the attack surface on the file system namespace we + have within a confined service. Higher levels mean more permissions for the + user and thus a bigger attack surface. + """ + NONE = 0 + + # Directories can be listed or files can be read. + READABLE = 1 + + # This is for special file systems such as procfs and for stuff such as + # FIFOs or character special files. The reason why this has a lower value + # than WRITABLE is because those files are more restricted on what and how + # they can be written to. + SPECIAL = 2 + + # Another special case are sticky directories, which do allow write access + # but restrict deletion. This does *not* apply to sticky directories that + # are read-only. + STICKY = 3 + + # Essentially full permissions, the kind of accessibility we want to avoid + # in most cases. + WRITABLE = 4 + + def assert_on(self, path: Path) -> None: + """ + Raise an AssertionError if the given 'path' allows for more + accessibility than 'self'. + """ + actual = self.NONE + + if path.is_symlink(): + actual = self.READABLE + elif path.is_dir(): + writable = True + + dummy_file = path / 'can_i_write' + try: + dummy_file.touch() + except OSError as e: + if e.errno in [errno.EROFS, errno.EACCES]: + writable = False + else: + raise + else: + dummy_file.unlink() + + if writable: + # The reason why we test this *after* we made sure it's + # writable is because we could have a sticky directory where + # the current user doesn't have write access. + if path.stat().st_mode & 0o1000 == 0o1000: + actual = self.STICKY + else: + actual = self.WRITABLE + else: + actual = self.READABLE + elif path.is_file(): + try: + with path.open('rb') as fp: + fp.read(1) + actual = self.READABLE + except PermissionError: + pass + + writable = True + try: + with path.open('ab') as fp: + fp.write('x') + size = fp.tell() + fp.truncate(size) + except PermissionError: + writable = False + except OSError as e: + if e.errno == errno.ETXTBSY: + writable = os.access(path, os.W_OK) + elif e.errno == errno.EROFS: + writable = False + else: + raise + + # Let's always try to fail towards being writable, so if *either* + # access(2) or a real write is successful it's writable. This is to + # make sure we don't accidentally introduce no-ops if we have bugs + # in the more complicated real write code above. + if writable or os.access(path, os.W_OK): + actual = self.WRITABLE + else: + # We need to be very careful when writing to or reading from + # special files (eg. FIFOs), since they can possibly block. So if + # it's not a file, just trust that access(2) won't lie. + if os.access(path, os.R_OK): + actual = self.READABLE + + if os.access(path, os.W_OK): + actual = self.SPECIAL + + if actual > self: + stat = path.stat() + details = ', '.join([ + f'permissions: {stat.st_mode & 0o7777:o}', + f'uid: {stat.st_uid}', + f'group: {stat.st_gid}', + ]) + + raise AssertionError( + f'Expected at most {self!r} but got {actual!r} for path' + f' {path} ({details}).' + ) + + +def is_special_fs(path: Path) -> bool: + """ + Check whether the given path truly is a special file system such as procfs + or sysfs. + """ + try: + if path == Path('/proc'): + return (path / 'version').read_text().startswith('Linux') + elif path == Path('/sys'): + return b'Linux' in (path / 'kernel' / 'notes').read_bytes() + except FileNotFoundError: + pass + return False + + +def is_empty_dir(path: Path) -> bool: + try: + next(path.iterdir()) + return False + except (StopIteration, PermissionError): + return True + + +def _assert_permissions_in_directory( + directory: Path, + accessibility: Accessibility, + subdirs: dict[Path, Accessibility], +) -> None: + accessibility.assert_on(directory) + + for file in directory.iterdir(): + if is_special_fs(file): + msg = f'Got unexpected special filesystem at {file}.' + assert subdirs.pop(file) == Accessibility.SPECIAL, msg + elif not file.is_symlink() and file.is_dir(): + subdir_access = subdirs.pop(file, accessibility) + if is_empty_dir(file): + # Whenever we got an empty directory, we check the permission + # constraints on the current directory (except if specified + # explicitly in subdirs) because for example if we're non-root + # (the constraints of the current directory are thus + # Accessibility.READABLE), we really have to make sure that + # empty directories are *never* writable. + subdir_access.assert_on(file) + else: + _assert_permissions_in_directory(file, subdir_access, subdirs) + else: + subdirs.pop(file, accessibility).assert_on(file) + + +def assert_permissions(subdirs: dict[str, Accessibility]) -> None: + """ + Recursively check whether the file system conforms to the accessibility + specification we specified via 'subdirs'. + """ + root = Path('/') + absolute_subdirs = {root / p: a for p, a in subdirs.items()} + _assert_permissions_in_directory( + root, + Accessibility.WRITABLE if os.getuid() == 0 else Accessibility.READABLE, + absolute_subdirs, + ) + for file in absolute_subdirs.keys(): + msg = f'Expected {file} to exist, but it was nowwhere to be found.' + raise AssertionError(msg) diff --git a/nixos/tests/systemd-confinement/default.nix b/nixos/tests/systemd-confinement/default.nix index 815cb3a3bbdd..52e5f1620cb9 100644 --- a/nixos/tests/systemd-confinement/default.nix +++ b/nixos/tests/systemd-confinement/default.nix @@ -2,19 +2,41 @@ import ../make-test-python.nix { name = "systemd-confinement"; nodes.machine = { pkgs, lib, ... }: let - testServer = pkgs.writeScript "testserver.sh" '' - #!${pkgs.runtimeShell} - export PATH=${lib.makeBinPath [ pkgs.coreutils pkgs.findutils ]} - ${lib.escapeShellArg pkgs.runtimeShell} 2>&1 - echo "exit-status:$?" - ''; + testLib = pkgs.python3Packages.buildPythonPackage { + name = "confinement-testlib"; + unpackPhase = '' + cat > setup.py <", the actual values are + # printed rather than getting a generic AssertionError or the need to + # pass an explicit assertion error message. + import ast + from pathlib import Path + from _pytest.assertion.rewrite import rewrite_asserts + + script = Path('${pkgs.writeText "${name}-main.py" '' + import errno, os, pytest, signal + from subprocess import run + from checkperms import Accessibility, assert_permissions + + ${testScript} + ''}') # noqa + filename = str(script) + source = script.read_bytes() + + tree = ast.parse(source, filename=filename) + rewrite_asserts(tree, source, filename) + exec(compile(tree, filename, 'exec', dont_inherit=True)) ''; mkTestStep = num: { @@ -22,28 +44,23 @@ import ../make-test-python.nix { testScript, config ? {}, serviceName ? "test${toString num}", + rawUnit ? null, }: { - systemd.sockets.${serviceName} = { - description = "Socket for Test Service ${toString num}"; - wantedBy = [ "sockets.target" ]; - socketConfig.ListenStream = "/run/test${toString num}.sock"; - socketConfig.Accept = true; - }; + systemd.packages = lib.optional (rawUnit != null) (pkgs.writeTextFile { + name = serviceName; + destination = "/etc/systemd/system/${serviceName}.service"; + text = rawUnit; + }); - systemd.services."${serviceName}@" = { - description = "Confined Test Service ${toString num}"; + systemd.services.${serviceName} = { + inherit description; + requiredBy = [ "multi-user.target" ]; confinement = (config.confinement or {}) // { enable = true; }; serviceConfig = (config.serviceConfig or {}) // { - ExecStart = testServer; - StandardInput = "socket"; + ExecStart = mkTest serviceName testScript; + Type = "oneshot"; }; } // removeAttrs config [ "confinement" "serviceConfig" ]; - - __testSteps = lib.mkOrder num '' - with subtest('${lib.escape ["'" "\\"] description}'): - machine.succeed("echo ${toString num} > /teststep") - ${lib.replaceStrings ["\n"] ["\n "] testScript} - ''; }; in { @@ -51,82 +68,90 @@ import ../make-test-python.nix { { description = "chroot-only confinement"; config.confinement.mode = "chroot-only"; testScript = '' - # chroot-exec starts a socket-activated service, - # but, upon starting, a systemd system service - # calls setup_namespace() which calls base_filesystem_create() - # which creates some usual top level directories. - # In chroot-only mode, without additional BindPaths= or the like, - # they must be empty and thus removable by rmdir. - paths = machine.succeed('chroot-exec rmdir /dev /etc /proc /root /sys /usr /var "&&" ls -Am /').strip() - assert_eq(paths, "bin, nix, run") - uid = machine.succeed('chroot-exec id -u').strip() - assert_eq(uid, "0") - machine.succeed("chroot-exec chown 65534 /bin") + assert_permissions({ + 'bin': Accessibility.WRITABLE, + 'nix': Accessibility.WRITABLE, + 'run': Accessibility.WRITABLE, + }) + + assert os.getuid() == 0 + os.chown('/bin', 65534, 0) ''; } { description = "full confinement with APIVFS"; testScript = '' - machine.succeed('chroot-exec rmdir /etc') - machine.fail("chroot-exec chown 65534 /bin") - assert_eq(machine.succeed('chroot-exec id -u').strip(), "0") - machine.succeed("chroot-exec chown 0 /bin") + Path('/etc').rmdir() + + assert_permissions({ + 'bin': Accessibility.WRITABLE, + 'nix': Accessibility.WRITABLE, + 'tmp': Accessibility.WRITABLE, + 'run': Accessibility.WRITABLE, + + 'proc': Accessibility.SPECIAL, + 'sys': Accessibility.SPECIAL, + 'dev': Accessibility.WRITABLE, + }) + + bin_gid = Path('/bin').stat().st_gid + with pytest.raises(OSError) as excinfo: + os.chown('/bin', 65534, bin_gid) + assert excinfo.value.errno == errno.EINVAL + + assert os.getuid() == 0 + os.chown('/bin', 0, 0) ''; } { description = "check existence of bind-mounted /etc"; config.serviceConfig.BindReadOnlyPaths = [ "/etc" ]; testScript = '' - passwd = machine.succeed('chroot-exec cat /etc/passwd').strip() - assert len(passwd) > 0, "/etc/passwd must not be empty" + assert Path('/etc/passwd').read_text() ''; } { description = "check if User/Group really runs as non-root"; config.serviceConfig.User = "chroot-testuser"; config.serviceConfig.Group = "chroot-testgroup"; testScript = '' - machine.succeed("chroot-exec ls -l /dev") - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of chroot-testuser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") + assert list(Path('/dev').iterdir()) + + assert os.getuid() != 0 + assert os.getgid() != 0 + + with pytest.raises(PermissionError): + Path('/bin/test').touch() ''; } { description = "check if DynamicUser is working in full-apivfs mode"; config.confinement.mode = "full-apivfs"; config.serviceConfig.DynamicUser = true; testScript = '' - machine.succeed("chroot-exec ls -l /dev") - paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - dedent(""" - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/host - /run/host/.os-release-stage - /run/host/.os-release-stage/os-release - /run/host/os-release - /run/systemd - /run/systemd/incoming - /sys - /tmp - /usr - /var - /var/tmp - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied - """).rstrip() - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") - # DynamicUser=true implies ProtectSystem=strict - machine.fail("chroot-exec touch /etc/test") + assert_permissions({ + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, + 'tmp': Accessibility.WRITABLE, + 'run': Accessibility.STICKY, + + 'proc': Accessibility.SPECIAL, + 'sys': Accessibility.SPECIAL, + + 'dev': Accessibility.SPECIAL, + 'dev/shm': Accessibility.STICKY, + 'dev/mqueue': Accessibility.STICKY, + + 'var': Accessibility.READABLE, + 'var/tmp': Accessibility.WRITABLE, + }) + + assert os.getuid() != 0 + assert os.getgid() != 0 + + with pytest.raises(OSError) as excinfo: + Path('/bin/test').touch() + assert excinfo.value.errno == errno.EROFS + + with pytest.raises(OSError) as excinfo: + Path('/etc/test').touch() + assert excinfo.value.errno == errno.EROFS ''; } { description = "check if DynamicUser and PrivateTmp=false are working in full-apivfs mode"; @@ -134,69 +159,47 @@ import ../make-test-python.nix { config.serviceConfig.DynamicUser = true; config.serviceConfig.PrivateTmp = false; testScript = '' - machine.succeed("chroot-exec ls -l /dev") - paths = machine.succeed('chroot-exec find / -path /dev/"\\*" -prune -o -path /nix/"\\*" -prune -o -path /proc/"\\*" -prune -o -path /sys/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - dedent(""" - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/host - /run/host/.os-release-stage - /run/host/.os-release-stage/os-release - /run/host/os-release - /run/systemd - /run/systemd/incoming - /sys - /usr - /var - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied - """).rstrip() - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") - # DynamicUser=true implies ProtectSystem=strict - machine.fail("chroot-exec touch /etc/test") + assert_permissions({ + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, + 'run': Accessibility.STICKY, + + 'proc': Accessibility.SPECIAL, + 'sys': Accessibility.SPECIAL, + + 'dev': Accessibility.SPECIAL, + 'dev/shm': Accessibility.STICKY, + 'dev/mqueue': Accessibility.STICKY, + }) + + assert os.getuid() != 0 + assert os.getgid() != 0 + + with pytest.raises(OSError) as excinfo: + Path('/bin/test').touch() + assert excinfo.value.errno == errno.EROFS + + with pytest.raises(OSError) as excinfo: + Path('/etc/test').touch() + assert excinfo.value.errno == errno.EROFS ''; } { description = "check if DynamicUser is working in chroot-only mode"; config.confinement.mode = "chroot-only"; config.serviceConfig.DynamicUser = true; testScript = '' - paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - dedent(""" - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/systemd - /run/systemd/incoming - /sys - /usr - /var - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied - """).rstrip() - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") + assert_permissions({ + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, + 'run': Accessibility.READABLE, + }) + + assert os.getuid() != 0 + assert os.getgid() != 0 + + with pytest.raises(OSError) as excinfo: + Path('/bin/test').touch() + assert excinfo.value.errno == errno.EROFS ''; } { description = "check if DynamicUser and PrivateTmp=true are working in chroot-only mode"; @@ -204,124 +207,119 @@ import ../make-test-python.nix { config.serviceConfig.DynamicUser = true; config.serviceConfig.PrivateTmp = true; testScript = '' - paths = machine.succeed('chroot-exec find / -path /nix/"\\*" -prune -o -print || test $? = 1') - assert_eq( - '\n'.join(sorted(paths.split('\n'))), - dedent(""" - / - /bin - /bin/sh - /dev - /etc - /nix - /proc - /root - /run - /run/systemd - /run/systemd/incoming - /sys - /tmp - /usr - /var - /var/tmp - find: '/root': Permission denied - find: '/run/systemd/incoming': Permission denied - """).rstrip() - ) - uid = machine.succeed('chroot-exec id -u').strip() - assert uid != "0", "UID of a DynamicUser shouldn't be 0" - machine.fail("chroot-exec touch /bin/test") + assert_permissions({ + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, + 'run': Accessibility.READABLE, + 'tmp': Accessibility.WRITABLE, + + 'var': Accessibility.READABLE, + 'var/tmp': Accessibility.WRITABLE, + }) + + assert os.getuid() != 0 + assert os.getgid() != 0 + + with pytest.raises(OSError) as excinfo: + Path('/bin/test').touch() + assert excinfo.value.errno == errno.EROFS ''; } (let symlink = pkgs.runCommand "symlink" { - target = pkgs.writeText "symlink-target" "got me\n"; + target = pkgs.writeText "symlink-target" "got me"; } "ln -s \"$target\" \"$out\""; in { description = "check if symlinks are properly bind-mounted"; config.confinement.packages = lib.singleton symlink; testScript = '' - machine.succeed("chroot-exec rmdir /etc") - text = machine.succeed('chroot-exec cat ${symlink}').strip() - assert_eq(text, "got me") + Path('/etc').rmdir() + assert Path('${symlink}').read_text() == 'got me' ''; }) { description = "check if StateDirectory works"; config.serviceConfig.User = "chroot-testuser"; config.serviceConfig.Group = "chroot-testgroup"; config.serviceConfig.StateDirectory = "testme"; + + # We restart on purpose here since we want to check whether the state + # directory actually persists. + config.serviceConfig.Restart = "on-failure"; + config.serviceConfig.RestartMode = "direct"; + testScript = '' - machine.succeed("chroot-exec touch /tmp/canary") - machine.succeed('chroot-exec "echo works > /var/lib/testme/foo"') - machine.succeed('test "$(< /var/lib/testme/foo)" = works') - machine.succeed("test ! -e /tmp/canary") + assert not Path('/tmp/canary').exists() + Path('/tmp/canary').touch() + + if (foo := Path('/var/lib/testme/foo')).exists(): + assert Path('/var/lib/testme/foo').read_text() == 'works' + else: + Path('/var/lib/testme/foo').write_text('works') + print('<4>Exiting with failure to check persistence on restart.') + raise SystemExit(1) ''; } { description = "check if /bin/sh works"; testScript = '' - machine.succeed( - "chroot-exec test -e /bin/sh", - 'test "$(chroot-exec \'/bin/sh -c "echo bar"\')" = bar', + assert Path('/bin/sh').exists() + + result = run( + ['/bin/sh', '-c', 'echo -n bar'], + capture_output=True, + check=True, ) + assert result.stdout == b'bar' ''; } { description = "check if suppressing /bin/sh works"; config.confinement.binSh = null; testScript = '' - machine.succeed( - 'chroot-exec test ! -e /bin/sh', - 'test "$(chroot-exec \'/bin/sh -c "echo foo"\')" != foo', - ) + assert not Path('/bin/sh').exists() + with pytest.raises(FileNotFoundError): + run(['/bin/sh', '-c', 'echo foo']) ''; } { description = "check if we can set /bin/sh to something different"; config.confinement.binSh = "${pkgs.hello}/bin/hello"; testScript = '' - machine.succeed("chroot-exec test -e /bin/sh") - machine.succeed('test "$(chroot-exec /bin/sh -g foo)" = foo') + assert Path('/bin/sh').exists() + result = run( + ['/bin/sh', '-g', 'foo'], + capture_output=True, + check=True, + ) + assert result.stdout == b'foo\n' ''; } { description = "check if only Exec* dependencies are included"; - config.environment.FOOBAR = pkgs.writeText "foobar" "eek\n"; + config.environment.FOOBAR = pkgs.writeText "foobar" "eek"; testScript = '' - machine.succeed('test "$(chroot-exec \'cat "$FOOBAR"\')" != eek') + with pytest.raises(FileNotFoundError): + Path(os.environ['FOOBAR']).read_text() ''; } - { description = "check if all unit dependencies are included"; - config.environment.FOOBAR = pkgs.writeText "foobar" "eek\n"; + { description = "check if fullUnit includes all dependencies"; + config.environment.FOOBAR = pkgs.writeText "foobar" "eek"; config.confinement.fullUnit = true; testScript = '' - machine.succeed('test "$(chroot-exec \'cat "$FOOBAR"\')" = eek') + assert Path(os.environ['FOOBAR']).read_text() == 'eek' ''; } { description = "check if shipped unit file still works"; - serviceName = "shipped-unitfile"; config.confinement.mode = "chroot-only"; + rawUnit = '' + [Service] + SystemCallFilter=~kill + SystemCallErrorNumber=ELOOP + ''; testScript = '' - machine.succeed( - 'chroot-exec \'kill -9 $$ 2>&1 || :\' | ' - 'grep -q "Too many levels of symbolic links"' - ) + with pytest.raises(OSError) as excinfo: + os.kill(os.getpid(), signal.SIGKILL) + assert excinfo.value.errno == errno.ELOOP ''; } ]; - options.__testSteps = lib.mkOption { - type = lib.types.lines; - description = "All of the test steps combined as a single script."; - }; - - config.environment.systemPackages = lib.singleton testClient; - config.systemd.packages = lib.singleton (pkgs.writeTextFile { - name = "shipped-unitfile"; - destination = "/etc/systemd/system/shipped-unitfile@.service"; - text = '' - [Service] - SystemCallFilter=~kill - SystemCallErrorNumber=ELOOP - ''; - }); - config.users.groups.chroot-testgroup = {}; config.users.users.chroot-testuser = { isSystemUser = true; @@ -330,16 +328,7 @@ import ../make-test-python.nix { }; }; - testScript = { nodes, ... }: '' - from textwrap import dedent - import difflib - - def assert_eq(got, expected): - if got != expected: - diff = difflib.unified_diff(got.splitlines(keepends=True), expected.splitlines(keepends=True)) - print("".join(diff)) - assert got == expected, f"{got} != {expected}" - + testScript = '' machine.wait_for_unit("multi-user.target") - '' + nodes.machine.__testSteps; + ''; } From 27f36b5e57ca0eab08728eac58b12c3b3ca67c33 Mon Sep 17 00:00:00 2001 From: aszlig Date: Thu, 25 Apr 2024 14:31:26 +0200 Subject: [PATCH 5/7] nixos/tests/confinement: Parametrise subtests This is to make sure that we test all of the DynamicUser/User/Group and PrivateTmp options in a uniform way. The reason why we need to do this is because we recently introduced support for the DynamicUser option and since there are some corner cases where we might end up with more elevated privileges (eg. writable directories in some cases), we want to make sure that the environment is as restrictive as with a static User/Group assignment. I also removed various checks that try to os.chown(), since with our new recursive checker those are redundant. Signed-off-by: aszlig --- nixos/tests/systemd-confinement/default.nix | 215 +++++++------------- 1 file changed, 78 insertions(+), 137 deletions(-) diff --git a/nixos/tests/systemd-confinement/default.nix b/nixos/tests/systemd-confinement/default.nix index 52e5f1620cb9..44a1f9ca195b 100644 --- a/nixos/tests/systemd-confinement/default.nix +++ b/nixos/tests/systemd-confinement/default.nix @@ -63,168 +63,110 @@ import ../make-test-python.nix { } // removeAttrs config [ "confinement" "serviceConfig" ]; }; - in { - imports = lib.imap1 mkTestStep [ - { description = "chroot-only confinement"; - config.confinement.mode = "chroot-only"; - testScript = '' + parametrisedTests = lib.concatMap ({ user, privateTmp }: let + withTmp = if privateTmp then "with PrivateTmp" else "without PrivateTmp"; + + serviceConfig = if user == "static-user" then { + User = "chroot-testuser"; + Group = "chroot-testgroup"; + } else if user == "dynamic-user" then { + DynamicUser = true; + } else {}; + + in [ + { description = "${user}, chroot-only confinement ${withTmp}"; + config = { + confinement.mode = "chroot-only"; + # Only set if privateTmp is true to ensure that the default is false. + serviceConfig = serviceConfig // lib.optionalAttrs privateTmp { + PrivateTmp = true; + }; + }; + testScript = if user == "root" then '' + assert os.getuid() == 0 + assert os.getgid() == 0 + assert_permissions({ 'bin': Accessibility.WRITABLE, 'nix': Accessibility.WRITABLE, 'run': Accessibility.WRITABLE, + ${lib.optionalString privateTmp "'tmp': Accessibility.STICKY,"} + ${lib.optionalString privateTmp "'var': Accessibility.WRITABLE,"} + ${lib.optionalString privateTmp "'var/tmp': Accessibility.STICKY,"} }) + '' else '' + assert os.getuid() != 0 + assert os.getgid() != 0 - assert os.getuid() == 0 - os.chown('/bin', 65534, 0) + assert_permissions({ + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, + 'run': Accessibility.READABLE, + ${lib.optionalString privateTmp "'tmp': Accessibility.STICKY,"} + ${lib.optionalString privateTmp "'var': Accessibility.READABLE,"} + ${lib.optionalString privateTmp "'var/tmp': Accessibility.STICKY,"} + }) ''; } - { description = "full confinement with APIVFS"; - testScript = '' - Path('/etc').rmdir() + { description = "${user}, full APIVFS confinement ${withTmp}"; + config = { + # Only set if privateTmp is false to ensure that the default is true. + serviceConfig = serviceConfig // lib.optionalAttrs (!privateTmp) { + PrivateTmp = false; + }; + }; + testScript = if user == "root" then '' + assert os.getuid() == 0 + assert os.getgid() == 0 assert_permissions({ 'bin': Accessibility.WRITABLE, 'nix': Accessibility.WRITABLE, - 'tmp': Accessibility.WRITABLE, + ${lib.optionalString privateTmp "'tmp': Accessibility.STICKY,"} 'run': Accessibility.WRITABLE, 'proc': Accessibility.SPECIAL, 'sys': Accessibility.SPECIAL, 'dev': Accessibility.WRITABLE, + + ${lib.optionalString privateTmp "'var': Accessibility.WRITABLE,"} + ${lib.optionalString privateTmp "'var/tmp': Accessibility.STICKY,"} }) + '' else '' + assert os.getuid() != 0 + assert os.getgid() != 0 - bin_gid = Path('/bin').stat().st_gid - with pytest.raises(OSError) as excinfo: - os.chown('/bin', 65534, bin_gid) - assert excinfo.value.errno == errno.EINVAL + assert_permissions({ + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, + ${lib.optionalString privateTmp "'tmp': Accessibility.STICKY,"} + 'run': Accessibility.STICKY, - assert os.getuid() == 0 - os.chown('/bin', 0, 0) + 'proc': Accessibility.SPECIAL, + 'sys': Accessibility.SPECIAL, + + 'dev': Accessibility.SPECIAL, + 'dev/shm': Accessibility.STICKY, + 'dev/mqueue': Accessibility.STICKY, + + ${lib.optionalString privateTmp "'var': Accessibility.READABLE,"} + ${lib.optionalString privateTmp "'var/tmp': Accessibility.STICKY,"} + }) ''; } - { description = "check existence of bind-mounted /etc"; + ]) (lib.cartesianProductOfSets { + user = [ "root" "dynamic-user" "static-user" ]; + privateTmp = [ true false ]; + }); + + in { + imports = lib.imap1 mkTestStep (parametrisedTests ++ [ + { description = "existence of bind-mounted /etc"; config.serviceConfig.BindReadOnlyPaths = [ "/etc" ]; testScript = '' assert Path('/etc/passwd').read_text() ''; } - { description = "check if User/Group really runs as non-root"; - config.serviceConfig.User = "chroot-testuser"; - config.serviceConfig.Group = "chroot-testgroup"; - testScript = '' - assert list(Path('/dev').iterdir()) - - assert os.getuid() != 0 - assert os.getgid() != 0 - - with pytest.raises(PermissionError): - Path('/bin/test').touch() - ''; - } - { description = "check if DynamicUser is working in full-apivfs mode"; - config.confinement.mode = "full-apivfs"; - config.serviceConfig.DynamicUser = true; - testScript = '' - assert_permissions({ - 'bin': Accessibility.READABLE, - 'nix': Accessibility.READABLE, - 'tmp': Accessibility.WRITABLE, - 'run': Accessibility.STICKY, - - 'proc': Accessibility.SPECIAL, - 'sys': Accessibility.SPECIAL, - - 'dev': Accessibility.SPECIAL, - 'dev/shm': Accessibility.STICKY, - 'dev/mqueue': Accessibility.STICKY, - - 'var': Accessibility.READABLE, - 'var/tmp': Accessibility.WRITABLE, - }) - - assert os.getuid() != 0 - assert os.getgid() != 0 - - with pytest.raises(OSError) as excinfo: - Path('/bin/test').touch() - assert excinfo.value.errno == errno.EROFS - - with pytest.raises(OSError) as excinfo: - Path('/etc/test').touch() - assert excinfo.value.errno == errno.EROFS - ''; - } - { description = "check if DynamicUser and PrivateTmp=false are working in full-apivfs mode"; - config.confinement.mode = "full-apivfs"; - config.serviceConfig.DynamicUser = true; - config.serviceConfig.PrivateTmp = false; - testScript = '' - assert_permissions({ - 'bin': Accessibility.READABLE, - 'nix': Accessibility.READABLE, - 'run': Accessibility.STICKY, - - 'proc': Accessibility.SPECIAL, - 'sys': Accessibility.SPECIAL, - - 'dev': Accessibility.SPECIAL, - 'dev/shm': Accessibility.STICKY, - 'dev/mqueue': Accessibility.STICKY, - }) - - assert os.getuid() != 0 - assert os.getgid() != 0 - - with pytest.raises(OSError) as excinfo: - Path('/bin/test').touch() - assert excinfo.value.errno == errno.EROFS - - with pytest.raises(OSError) as excinfo: - Path('/etc/test').touch() - assert excinfo.value.errno == errno.EROFS - ''; - } - { description = "check if DynamicUser is working in chroot-only mode"; - config.confinement.mode = "chroot-only"; - config.serviceConfig.DynamicUser = true; - testScript = '' - assert_permissions({ - 'bin': Accessibility.READABLE, - 'nix': Accessibility.READABLE, - 'run': Accessibility.READABLE, - }) - - assert os.getuid() != 0 - assert os.getgid() != 0 - - with pytest.raises(OSError) as excinfo: - Path('/bin/test').touch() - assert excinfo.value.errno == errno.EROFS - ''; - } - { description = "check if DynamicUser and PrivateTmp=true are working in chroot-only mode"; - config.confinement.mode = "chroot-only"; - config.serviceConfig.DynamicUser = true; - config.serviceConfig.PrivateTmp = true; - testScript = '' - assert_permissions({ - 'bin': Accessibility.READABLE, - 'nix': Accessibility.READABLE, - 'run': Accessibility.READABLE, - 'tmp': Accessibility.WRITABLE, - - 'var': Accessibility.READABLE, - 'var/tmp': Accessibility.WRITABLE, - }) - - assert os.getuid() != 0 - assert os.getgid() != 0 - - with pytest.raises(OSError) as excinfo: - Path('/bin/test').touch() - assert excinfo.value.errno == errno.EROFS - ''; - } (let symlink = pkgs.runCommand "symlink" { target = pkgs.writeText "symlink-target" "got me"; @@ -233,7 +175,6 @@ import ../make-test-python.nix { description = "check if symlinks are properly bind-mounted"; config.confinement.packages = lib.singleton symlink; testScript = '' - Path('/etc').rmdir() assert Path('${symlink}').read_text() == 'got me' ''; }) @@ -318,7 +259,7 @@ import ../make-test-python.nix { assert excinfo.value.errno == errno.ELOOP ''; } - ]; + ]); config.users.groups.chroot-testgroup = {}; config.users.users.chroot-testuser = { From 0a9cecc35a651a020f66a4cc2a8333e33558650d Mon Sep 17 00:00:00 2001 From: aszlig Date: Mon, 6 May 2024 14:50:15 +0200 Subject: [PATCH 6/7] nixos/systemd-confinement: Make / read-only Our more thorough parametrised tests uncovered that with the changes for supporting DynamicUser, we now have the situation that for static users the root directory within the confined environment is now writable for the user in question. This is obviously not what we want and I'd consider that a regression. However while discussing this with @ju1m and my suggestion being to set TemporaryFileSystem to "/" (as we had previously), they had an even better idea[1]: > The goal is to deny write access to / to non-root users, > > * TemporaryFileSystem=/ gives us that through the ownership of / by > root (instead of the service's user inherited from > RuntimeDirectory=). > * ProtectSystem=strict gives us that by mounting / read-only (while > keeping its ownership to the service's user). > > To avoid the incompatibilities of TemporaryFileSystem=/ mentioned > above, I suggest to mount / read-only in all cases with > ReadOnlyPaths = [ "+/" ]: > > ... > > I guess this would require at least two changes to the current tests: > > 1. to no longer expect root to be able to write to some paths (like > /bin) (at least not without first remounting / in read-write > mode). > 2. to no longer expect non-root users to fail to write to certain > paths with a "permission denied" error code, but with a > "read-only file system" error code. I like the solution with ReadOnlyPaths even more because it further reduces the attack surface if the user is root. In chroot-only mode this is especially useful, since if there are no other bind-mounted paths involved in the unit configuration, the whole file system within the confined environment is read-only. [1]: https://github.com/NixOS/nixpkgs/pull/289593#discussion_r1586794215 Signed-off-by: aszlig --- nixos/modules/security/systemd-confinement.nix | 1 + nixos/tests/systemd-confinement/default.nix | 15 +++++++-------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/nixos/modules/security/systemd-confinement.nix b/nixos/modules/security/systemd-confinement.nix index ed33c41c79ae..fa58ea22f868 100644 --- a/nixos/modules/security/systemd-confinement.nix +++ b/nixos/modules/security/systemd-confinement.nix @@ -105,6 +105,7 @@ in { wantsAPIVFS = lib.mkDefault (config.confinement.mode == "full-apivfs"); in lib.mkIf config.confinement.enable { serviceConfig = { + ReadOnlyPaths = [ "+/" ]; RuntimeDirectory = [ "confinement/${mkPathSafeName name}" ]; RootDirectory = lib.mkDefault "/run/confinement/${mkPathSafeName name}"; InaccessiblePaths = [ diff --git a/nixos/tests/systemd-confinement/default.nix b/nixos/tests/systemd-confinement/default.nix index 44a1f9ca195b..15d442d476b0 100644 --- a/nixos/tests/systemd-confinement/default.nix +++ b/nixos/tests/systemd-confinement/default.nix @@ -87,11 +87,11 @@ import ../make-test-python.nix { assert os.getgid() == 0 assert_permissions({ - 'bin': Accessibility.WRITABLE, - 'nix': Accessibility.WRITABLE, - 'run': Accessibility.WRITABLE, + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, + 'run': Accessibility.READABLE, ${lib.optionalString privateTmp "'tmp': Accessibility.STICKY,"} - ${lib.optionalString privateTmp "'var': Accessibility.WRITABLE,"} + ${lib.optionalString privateTmp "'var': Accessibility.READABLE,"} ${lib.optionalString privateTmp "'var/tmp': Accessibility.STICKY,"} }) '' else '' @@ -120,8 +120,8 @@ import ../make-test-python.nix { assert os.getgid() == 0 assert_permissions({ - 'bin': Accessibility.WRITABLE, - 'nix': Accessibility.WRITABLE, + 'bin': Accessibility.READABLE, + 'nix': Accessibility.READABLE, ${lib.optionalString privateTmp "'tmp': Accessibility.STICKY,"} 'run': Accessibility.WRITABLE, @@ -129,7 +129,7 @@ import ../make-test-python.nix { 'sys': Accessibility.SPECIAL, 'dev': Accessibility.WRITABLE, - ${lib.optionalString privateTmp "'var': Accessibility.WRITABLE,"} + ${lib.optionalString privateTmp "'var': Accessibility.READABLE,"} ${lib.optionalString privateTmp "'var/tmp': Accessibility.STICKY,"} }) '' else '' @@ -144,7 +144,6 @@ import ../make-test-python.nix { 'proc': Accessibility.SPECIAL, 'sys': Accessibility.SPECIAL, - 'dev': Accessibility.SPECIAL, 'dev/shm': Accessibility.STICKY, 'dev/mqueue': Accessibility.STICKY, From e4bd1e8f92371efd9b48657cc03b04a755a05f49 Mon Sep 17 00:00:00 2001 From: aszlig Date: Mon, 13 May 2024 00:28:09 +0200 Subject: [PATCH 7/7] nixos/confinement: Use prio 100 for RootDirectory One of the module that already supports the systemd-confinement module is public-inbox. However with the changes to support DynamicUser and ProtectSystem, the module will now fail at runtime if confinement is enabled (it's optional and you'll need to override it via another module). The reason is that the RootDirectory is set to /var/empty in the public-inbox module, which doesn't work well with the InaccessiblePaths directive we now use to support DynamicUser/ProtectSystem. To make this issue more visible, I decided to just change the priority of the RootDirectory option definiton the default override priority so that whenever another different option is defined, we'll get a conflict at evaluation time. Signed-off-by: aszlig --- nixos/modules/security/systemd-confinement.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nixos/modules/security/systemd-confinement.nix b/nixos/modules/security/systemd-confinement.nix index fa58ea22f868..041c90033886 100644 --- a/nixos/modules/security/systemd-confinement.nix +++ b/nixos/modules/security/systemd-confinement.nix @@ -107,7 +107,7 @@ in { serviceConfig = { ReadOnlyPaths = [ "+/" ]; RuntimeDirectory = [ "confinement/${mkPathSafeName name}" ]; - RootDirectory = lib.mkDefault "/run/confinement/${mkPathSafeName name}"; + RootDirectory = "/run/confinement/${mkPathSafeName name}"; InaccessiblePaths = [ "-+/run/confinement/${mkPathSafeName name}" ];