From 6d1aa523de8ece115eb0504b2313c12dc5302383 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 8 May 2023 14:41:47 -0400 Subject: [PATCH 01/17] Create escape hatch for supplementary group sandboxing woes There is no obvious good solution for this that has occured to anyone. --- src/libstore/build/local-derivation-goal.cc | 13 ++++--------- src/libstore/globals.hh | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 21cd6e7ee..6d2d458da 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -907,15 +907,10 @@ void LocalDerivationGoal::startBuilder() openSlave(); /* Drop additional groups here because we can't do it - after we've created the new user namespace. FIXME: - this means that if we're not root in the parent - namespace, we can't drop additional groups; they will - be mapped to nogroup in the child namespace. There does - not seem to be a workaround for this. (But who can tell - from reading user_namespaces(7)?) - See also https://lwn.net/Articles/621612/. */ - if (getuid() == 0 && setgroups(0, 0) == -1) - throw SysError("setgroups failed"); + after we've created the new user namespace. */ + if (settings.dropSupplementaryGroups) + if (setgroups(0, 0) == -1) + throw SysError("setgroups failed"); ProcessOptions options; options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 609cf53b8..0b429cf98 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -515,6 +515,25 @@ public: Setting sandboxFallback{this, true, "sandbox-fallback", "Whether to disable sandboxing when the kernel doesn't allow it."}; + Setting dropSupplementaryGroups{this, getuid() == 0, "drop-supplementary-groups", + R"( + Whether to drop supplementary groups when building with sandboxing. + This is normally a good idea if we are root and have the capability to + do so. + + But if this "root" is mapped from a non-root user in a larger + namespace, we won't be able drop additional groups; they will be + mapped to nogroup in the child namespace. There does not seem to be a + workaround for this. + + (But who can tell from reading user_namespaces(7)? See also https://lwn.net/Articles/621612/.) + + TODO: It might be good to create a middle ground option that allows + `setgroups` to fail if all additional groups are "nogroup" / the value + of `/proc/sys/fs/overflowuid`. This would handle the common + nested-sandboxing case identified above. + )"}; + #if __linux__ Setting sandboxShmSize{ this, "50%", "sandbox-dev-shm-size", From 2524a2118647a4125dcae08fe0eb20de5f79a291 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 15 May 2023 12:38:39 -0400 Subject: [PATCH 02/17] Update src/libstore/build/local-derivation-goal.cc Co-authored-by: Guillaume Girol --- src/libstore/build/local-derivation-goal.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 6d2d458da..a50fe1a28 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -910,7 +910,7 @@ void LocalDerivationGoal::startBuilder() after we've created the new user namespace. */ if (settings.dropSupplementaryGroups) if (setgroups(0, 0) == -1) - throw SysError("setgroups failed"); + throw SysError("setgroups failed. Set the drop-supplementary-groups option to false to skip this step."); ProcessOptions options; options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; From d8ef0c949523324615b66059b3d48c4c445f478b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 15 May 2023 17:41:51 -0400 Subject: [PATCH 03/17] Add some tests for `drop-supplementary-groups` --- tests/common.sh | 2 +- tests/common/vars-and-functions.sh.in | 2 +- tests/hermetic.nix | 56 +++++++++++++++++++++++++++ tests/local.mk | 1 + tests/supplementary-groups.sh | 33 ++++++++++++++++ 5 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 tests/hermetic.nix create mode 100644 tests/supplementary-groups.sh diff --git a/tests/common.sh b/tests/common.sh index 8941671d6..7b0922c9f 100644 --- a/tests/common.sh +++ b/tests/common.sh @@ -4,7 +4,7 @@ if [[ -z "${COMMON_SH_SOURCED-}" ]]; then COMMON_SH_SOURCED=1 -source "$(readlink -f "$(dirname "${BASH_SOURCE[0]}")")/common/vars-and-functions.sh" +source "$(readlink -f "$(dirname "${BASH_SOURCE[0]-$0}")")/common/vars-and-functions.sh" if [[ -n "${NIX_DAEMON_PACKAGE:-}" ]]; then startDaemon fi diff --git a/tests/common/vars-and-functions.sh.in b/tests/common/vars-and-functions.sh.in index a9e6c802f..dc7ce13cc 100644 --- a/tests/common/vars-and-functions.sh.in +++ b/tests/common/vars-and-functions.sh.in @@ -4,7 +4,7 @@ if [[ -z "${COMMON_VARS_AND_FUNCTIONS_SH_SOURCED-}" ]]; then COMMON_VARS_AND_FUNCTIONS_SH_SOURCED=1 -export PS4='+(${BASH_SOURCE[0]}:$LINENO) ' +export PS4='+(${BASH_SOURCE[0]-$0}:$LINENO) ' export TEST_ROOT=$(realpath ${TMPDIR:-/tmp}/nix-test)/${TEST_NAME:-default} export NIX_STORE_DIR diff --git a/tests/hermetic.nix b/tests/hermetic.nix new file mode 100644 index 000000000..4c9d7a51f --- /dev/null +++ b/tests/hermetic.nix @@ -0,0 +1,56 @@ +{ busybox, seed }: + +with import ./config.nix; + +let + contentAddressedByDefault = builtins.getEnv "NIX_TESTS_CA_BY_DEFAULT" == "1"; + caArgs = if contentAddressedByDefault then { + __contentAddressed = true; + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; + } else {}; + + mkDerivation = args: + derivation ({ + inherit system; + builder = busybox; + args = ["sh" "-e" args.builder or (builtins.toFile "builder-${args.name}.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; + } // removeAttrs args ["builder" "meta" "passthru"] + // caArgs) + // { meta = args.meta or {}; passthru = args.passthru or {}; }; + + input1 = mkDerivation { + shell = busybox; + name = "hermetic-input-1"; + buildCommand = "echo hi-input1 seed=${toString seed}; echo FOO > $out"; + }; + + input2 = mkDerivation { + shell = busybox; + name = "hermetic-input-2"; + buildCommand = "echo hi; echo BAR > $out"; + }; + + input3 = mkDerivation { + shell = busybox; + name = "hermetic-input-3"; + buildCommand = '' + echo hi-input3 + read x < ${input2} + echo $x BAZ > $out + ''; + }; + +in + + mkDerivation { + shell = busybox; + name = "hermetic"; + passthru = { inherit input1 input2 input3; }; + buildCommand = + '' + read x < ${input1} + read y < ${input3} + echo "$x $y" > $out + ''; + } diff --git a/tests/local.mk b/tests/local.mk index 9e340e2e2..9cb81e1f0 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -93,6 +93,7 @@ nix_tests = \ misc.sh \ dump-db.sh \ linux-sandbox.sh \ + supplementary-groups.sh \ build-dry.sh \ structured-attrs.sh \ shell.sh \ diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh new file mode 100644 index 000000000..fd3da2945 --- /dev/null +++ b/tests/supplementary-groups.sh @@ -0,0 +1,33 @@ +source common.sh + +requireSandboxSupport +[[ $busybox =~ busybox ]] || skipTest "no busybox" +if ! command -p -v unshare; then skipTest "Need unshare"; fi +needLocalStore "The test uses --store always so we would just be bypassing the daemon" + +unshare --mount --map-root-user bash < Date: Mon, 15 May 2023 17:49:04 -0400 Subject: [PATCH 04/17] Avoid out links in supplementary groups test This gets in the way of the tests running in parallel. --- tests/supplementary-groups.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh index fd3da2945..47debc5e3 100644 --- a/tests/supplementary-groups.sh +++ b/tests/supplementary-groups.sh @@ -13,7 +13,7 @@ unshare --mount --map-root-user bash < Date: Tue, 11 Jul 2023 10:44:03 +0100 Subject: [PATCH 05/17] Always attempt setgroups but allow failure to be ignored. --- src/libstore/build/local-derivation-goal.cc | 9 ++++++--- src/libstore/globals.hh | 2 +- tests/supplementary-groups.sh | 8 ++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 53e6998e8..068b47f93 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -909,9 +909,12 @@ void LocalDerivationGoal::startBuilder() /* Drop additional groups here because we can't do it after we've created the new user namespace. */ - if (settings.dropSupplementaryGroups) - if (setgroups(0, 0) == -1) - throw SysError("setgroups failed. Set the drop-supplementary-groups option to false to skip this step."); + if (setgroups(0, 0) == -1) { + if (errno != EPERM) + throw SysError("setgroups failed"); + if (settings.requireDropSupplementaryGroups) + throw Error("setgroups failed. Set the require-drop-supplementary-groups option to false to skip this step."); + } ProcessOptions options; options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index a19b43086..dbabf116a 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -524,7 +524,7 @@ public: Setting sandboxFallback{this, true, "sandbox-fallback", "Whether to disable sandboxing when the kernel doesn't allow it."}; - Setting dropSupplementaryGroups{this, getuid() == 0, "drop-supplementary-groups", + Setting requireDropSupplementaryGroups{this, true, "require-drop-supplementary-groups", R"( Whether to drop supplementary groups when building with sandboxing. This is normally a good idea if we are root and have the capability to diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh index 47debc5e3..47c6ef605 100644 --- a/tests/supplementary-groups.sh +++ b/tests/supplementary-groups.sh @@ -20,14 +20,14 @@ unshare --mount --map-root-user bash < Date: Tue, 11 Jul 2023 10:57:03 +0100 Subject: [PATCH 06/17] Update description for require-drop-supplementary-groups. --- src/libstore/globals.hh | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index dbabf116a..601626d00 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -526,21 +526,15 @@ public: Setting requireDropSupplementaryGroups{this, true, "require-drop-supplementary-groups", R"( - Whether to drop supplementary groups when building with sandboxing. - This is normally a good idea if we are root and have the capability to - do so. + Following the principle of least privilege, + Nix will attempt to drop supplementary groups when building with sandboxing. - But if this "root" is mapped from a non-root user in a larger - namespace, we won't be able drop additional groups; they will be - mapped to nogroup in the child namespace. There does not seem to be a - workaround for this. + However this can fail under some circumstances. + For example, if the user lacks the CAP_SETGID capability. + Search setgroups(2) for EPERM to find more detailed information on this. - (But who can tell from reading user_namespaces(7)? See also https://lwn.net/Articles/621612/.) - - TODO: It might be good to create a middle ground option that allows - `setgroups` to fail if all additional groups are "nogroup" / the value - of `/proc/sys/fs/overflowuid`. This would handle the common - nested-sandboxing case identified above. + If you encounter such a failure, + you can instruct Nix to continue without dropping supplementary groups by setting this option to `false`. )"}; #if __linux__ From 2b4c59dd997c72069b6039783fea4c3b35f5cee7 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Jul 2023 11:09:25 +0100 Subject: [PATCH 07/17] Be clearer about the security implications. --- src/libstore/globals.hh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 601626d00..dec132ff0 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -533,8 +533,9 @@ public: For example, if the user lacks the CAP_SETGID capability. Search setgroups(2) for EPERM to find more detailed information on this. - If you encounter such a failure, - you can instruct Nix to continue without dropping supplementary groups by setting this option to `false`. + If you encounter such a failure, setting this option to `false` will let you ignore it and continue. + But before doing so, you should consider the security implications carefully. + Not dropping supplementary groups means the build sandbox will be less restricted than intended. )"}; #if __linux__ From a193ec4052d9efa895681c438cc335296c7affea Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Jul 2023 11:13:39 +0100 Subject: [PATCH 08/17] Default should depend on whether we are root. --- src/libstore/globals.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index dec132ff0..9a9b4903f 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -524,7 +524,7 @@ public: Setting sandboxFallback{this, true, "sandbox-fallback", "Whether to disable sandboxing when the kernel doesn't allow it."}; - Setting requireDropSupplementaryGroups{this, true, "require-drop-supplementary-groups", + Setting requireDropSupplementaryGroups{this, getuid() == 0, "require-drop-supplementary-groups", R"( Following the principle of least privilege, Nix will attempt to drop supplementary groups when building with sandboxing. From b8e8dfc3e8581a08bc179f75fbe61e04030088de Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Jul 2023 11:24:11 +0100 Subject: [PATCH 09/17] Say a bit about default value in setting description. --- src/libstore/globals.hh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 9a9b4903f..81fa154bb 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -536,6 +536,10 @@ public: If you encounter such a failure, setting this option to `false` will let you ignore it and continue. But before doing so, you should consider the security implications carefully. Not dropping supplementary groups means the build sandbox will be less restricted than intended. + + This option defaults to `true` when the user is root + (since root usually has permissions to call setgroups) + and `false` otherwise. )"}; #if __linux__ From c1d39de1fbceebbb6b46abc4a21fb8e34423df99 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Thu, 15 Jun 2023 12:48:06 +0100 Subject: [PATCH 10/17] Check _NIX_TEST_NO_SANDBOX when setting _canUseSandbox. --- tests/common/vars-and-functions.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/common/vars-and-functions.sh.in b/tests/common/vars-and-functions.sh.in index dc7ce13cc..ad0623871 100644 --- a/tests/common/vars-and-functions.sh.in +++ b/tests/common/vars-and-functions.sh.in @@ -141,7 +141,7 @@ restartDaemon() { startDaemon } -if [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then +if [[ -z "${_NIX_TEST_NO_SANDBOX:-}" ]] && [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then _canUseSandbox=1 fi From 41412dc4ae0fec2d08335c19724276d99e0c6056 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Thu, 15 Jun 2023 12:58:59 +0100 Subject: [PATCH 11/17] Skip build-remote-trustless unless sandbox is supported. --- tests/build-remote-trustless-should-fail-0.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/build-remote-trustless-should-fail-0.sh b/tests/build-remote-trustless-should-fail-0.sh index fad1def59..b14101f83 100644 --- a/tests/build-remote-trustless-should-fail-0.sh +++ b/tests/build-remote-trustless-should-fail-0.sh @@ -2,6 +2,7 @@ source common.sh enableFeatures "daemon-trust-override" +requireSandboxSupport restartDaemon [[ $busybox =~ busybox ]] || skipTest "no busybox" From 0309f6b5b8ca354e1a43d320928d3d546ce7f878 Mon Sep 17 00:00:00 2001 From: Ben Radford <104896700+benradf@users.noreply.github.com> Date: Wed, 12 Jul 2023 12:32:57 +0100 Subject: [PATCH 12/17] Update src/libstore/globals.hh Co-authored-by: Valentin Gagarin --- src/libstore/globals.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 81fa154bb..d8fda9707 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -530,8 +530,8 @@ public: Nix will attempt to drop supplementary groups when building with sandboxing. However this can fail under some circumstances. - For example, if the user lacks the CAP_SETGID capability. - Search setgroups(2) for EPERM to find more detailed information on this. + For example, if the user lacks the `CAP_SETGID` capability. + Search `setgroups(2)` for `EPERM` to find more detailed information on this. If you encounter such a failure, setting this option to `false` will let you ignore it and continue. But before doing so, you should consider the security implications carefully. From a2acd23466108a1a10ed3b6b874c558bd95e7645 Mon Sep 17 00:00:00 2001 From: Ben Radford <104896700+benradf@users.noreply.github.com> Date: Wed, 12 Jul 2023 12:33:05 +0100 Subject: [PATCH 13/17] Update src/libstore/globals.hh Co-authored-by: Valentin Gagarin --- src/libstore/globals.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index d8fda9707..d4b8fb1f9 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -538,7 +538,7 @@ public: Not dropping supplementary groups means the build sandbox will be less restricted than intended. This option defaults to `true` when the user is root - (since root usually has permissions to call setgroups) + (since `root` usually has permissions to call setgroups) and `false` otherwise. )"}; From 9e64f243406ff2b3fbe06adbc762cf965e6825a8 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 13 Jul 2023 15:06:34 -0400 Subject: [PATCH 14/17] Revert "Check _NIX_TEST_NO_SANDBOX when setting _canUseSandbox." This reverts commit c1d39de1fbceebbb6b46abc4a21fb8e34423df99. --- tests/common/vars-and-functions.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/common/vars-and-functions.sh.in b/tests/common/vars-and-functions.sh.in index ad0623871..dc7ce13cc 100644 --- a/tests/common/vars-and-functions.sh.in +++ b/tests/common/vars-and-functions.sh.in @@ -141,7 +141,7 @@ restartDaemon() { startDaemon } -if [[ -z "${_NIX_TEST_NO_SANDBOX:-}" ]] && [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then +if [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then _canUseSandbox=1 fi From 84c4e6f0acfc902955d580aa090aa52f20ee82ad Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 13 Jul 2023 15:06:50 -0400 Subject: [PATCH 15/17] Revert "Skip build-remote-trustless unless sandbox is supported." This reverts commit 41412dc4ae0fec2d08335c19724276d99e0c6056. --- tests/build-remote-trustless-should-fail-0.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/build-remote-trustless-should-fail-0.sh b/tests/build-remote-trustless-should-fail-0.sh index b14101f83..fad1def59 100644 --- a/tests/build-remote-trustless-should-fail-0.sh +++ b/tests/build-remote-trustless-should-fail-0.sh @@ -2,7 +2,6 @@ source common.sh enableFeatures "daemon-trust-override" -requireSandboxSupport restartDaemon [[ $busybox =~ busybox ]] || skipTest "no busybox" From 1a13757880479921966adeca839cf182b3ffcbd0 Mon Sep 17 00:00:00 2001 From: cidkidnix Date: Thu, 13 Jul 2023 14:17:22 -0500 Subject: [PATCH 16/17] Add comment regarding the unset of NIX_STORE_DIR in build-remote.sh and supplementary-groups.sh --- tests/build-remote.sh | 1 + tests/supplementary-groups.sh | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/tests/build-remote.sh b/tests/build-remote.sh index 78e12b477..d2a2132c1 100644 --- a/tests/build-remote.sh +++ b/tests/build-remote.sh @@ -1,6 +1,7 @@ requireSandboxSupport [[ $busybox =~ busybox ]] || skipTest "no busybox" +# Avoid store dir being inside sandbox build-dir unset NIX_STORE_DIR unset NIX_STATE_DIR diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh index 47c6ef605..474ba7503 100644 --- a/tests/supplementary-groups.sh +++ b/tests/supplementary-groups.sh @@ -5,6 +5,10 @@ requireSandboxSupport if ! command -p -v unshare; then skipTest "Need unshare"; fi needLocalStore "The test uses --store always so we would just be bypassing the daemon" +# Avoid store dir being inside sandbox build-dir +unset NIX_STORE_DIR +unset NIX_STATE_DIR + unshare --mount --map-root-user bash < Date: Thu, 13 Jul 2023 14:23:24 -0500 Subject: [PATCH 17/17] move unset NIX_STORE_DIR in supplementary-groups.sh to inside the unshare --- tests/supplementary-groups.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh index 474ba7503..d18fb2414 100644 --- a/tests/supplementary-groups.sh +++ b/tests/supplementary-groups.sh @@ -5,13 +5,13 @@ requireSandboxSupport if ! command -p -v unshare; then skipTest "Need unshare"; fi needLocalStore "The test uses --store always so we would just be bypassing the daemon" -# Avoid store dir being inside sandbox build-dir -unset NIX_STORE_DIR -unset NIX_STATE_DIR - unshare --mount --map-root-user bash <