Merge pull request #8342 from NixLayeredStore/best-effort-supplementary-groups

Best effort supplementary groups

src/libstore/build/local-derivation-goal.cc

@@ -912,15 +912,13 @@ 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)
+               after we've created the new user namespace. */
+            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;

src/libstore/globals.hh

@@ -524,6 +524,24 @@ public:
     Setting<bool> sandboxFallback{this, true, "sandbox-fallback",
         "Whether to disable sandboxing when the kernel doesn't allow it."};
 
+    Setting<bool> 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.
+
+          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.
+
+          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__
     Setting<std::string> sandboxShmSize{
         this, "50%", "sandbox-dev-shm-size",

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
 

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

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

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
+      '';
+  }

tests/local.mk

@@ -95,6 +95,7 @@ nix_tests = \
   misc.sh \
   dump-db.sh \
   linux-sandbox.sh \
+  supplementary-groups.sh \
   build-dry.sh \
   structured-attrs.sh \
   shell.sh \

tests/supplementary-groups.sh

@@ -0,0 +1,37 @@
+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 <<EOF
+  source common.sh
+
+  # Avoid store dir being inside sandbox build-dir
+  unset NIX_STORE_DIR
+  unset NIX_STATE_DIR
+
+  setLocalStore () {
+    export NIX_REMOTE=\$TEST_ROOT/\$1
+    mkdir -p \$NIX_REMOTE
+  }
+
+  cmd=(nix-build ./hermetic.nix --arg busybox "$busybox" --arg seed 1 --no-out-link)
+
+  # Fails with default setting
+  # TODO better error
+  setLocalStore store1
+  expectStderr 1 "\${cmd[@]}" | grepQuiet "unable to start build process"
+
+  # Fails with `require-drop-supplementary-groups`
+  # TODO better error
+  setLocalStore store2
+  NIX_CONFIG='require-drop-supplementary-groups = true' \
+    expectStderr 1 "\${cmd[@]}" | grepQuiet "unable to start build process"
+
+  # Works without `require-drop-supplementary-groups`
+  setLocalStore store3
+  NIX_CONFIG='require-drop-supplementary-groups = false' \
+    "\${cmd[@]}"
+EOF