release: 2.93.3 "Bici Bici"

Release produced with releng/create_release.xsh

Change-Id: I49a2c0c8bd79e864809b64d4c8d2b0049d570c02

doc/manual/src/release-notes/rl-2.93.md

@@ -1,4 +1,20 @@
 # Lix 2.93 "Bici Bici" (2025-05-09)
+# Lix 2.93.3 (2025-07-22)
+## Improvements
+
+- `--keep-failed` chowns the build directory to the user that request the build [cl/3678](https://gerrit.lix.systems/c/lix/+/3678)
+
+  Running a build with `--keep-failed` now chowns the temporary directory from the
+  builder user and group to the user that request the build if the build came from
+  a local user connected to the daemon. This makes inspecting failed derivations a
+  lot easier. On Linux the build directory made visible to the user will not be in
+  the same path as it was in the sandbox and continuing builds will usually break.
+
+  Many thanks to [eldritch horrors](https://git.lix.systems/pennae) for this.
+
+
+
+
 # Lix 2.93.2 (2025-06-30)
 ## Fixes
 

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

@@ -30,11 +30,13 @@
 #include "platform/linux.hh"
 
 #include <cstddef>
+#include <dirent.h>
 #include <exception>
 #include <regex>
 #include <queue>
 
 #include <stdexcept>
+#include <sys/stat.h>
 #include <sys/un.h>
 #include <fcntl.h>
 #include <termios.h>
@@ -113,7 +115,11 @@ LocalDerivationGoal::~LocalDerivationGoal() noexcept(false)
     /* Careful: we should never ever throw an exception from a
        destructor. */
     try { killChild(); } catch (...) { ignoreExceptionInDestructor(); }
-    try { deleteTmpDir(false, true); } catch (...) { ignoreExceptionInDestructor(); }
+    try {
+        finalizeTmpDir(false, true);
+    } catch (...) {
+        ignoreExceptionInDestructor();
+    }
 }
 
 
@@ -363,13 +369,14 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
         if (statvfs(localStore.config().realStoreDir.get().c_str(), &st) == 0 &&
             (uint64_t) st.f_bavail * st.f_bsize < required)
             diskFull = true;
-        if (statvfs(tmpDir.c_str(), &st) == 0 &&
+        if (statvfs(tmpDirRoot.c_str(), &st) == 0 && (uint64_t) st.f_bavail * st.f_bsize < required)
-            (uint64_t) st.f_bavail * st.f_bsize < required)
+        {
             diskFull = true;
+        }
     }
 #endif
 
-    deleteTmpDir(false);
+    finalizeTmpDir(false);
 
     /* Move paths out of the chroot for easier debugging of
        build failures. */
@@ -388,7 +395,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
 
 void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck()
 {
-    deleteTmpDir(true);
+    finalizeTmpDir(true);
 }
 
 
@@ -490,7 +497,7 @@ try {
 
         /* Create a temporary directory where the build will take
            place. */
-        tmpDir =
+        tmpDirRoot =
             createTempDir(buildDir, "nix-build-" + std::string(drvPath.name()), false, false, 0700);
     } catch (SysError & e) {
         /*
@@ -520,17 +527,50 @@ try {
             nixBuildsTmp
         );
         worker.buildDirOverride = nixBuildsTmp;
-        tmpDir = createTempDir(
+        tmpDirRoot = createTempDir(
             nixBuildsTmp, "nix-build-" + std::string(drvPath.name()), false, false, 0700
         );
     }
     /* The TOCTOU between the previous mkdir call and this open call is unavoidable due to
      * POSIX semantics.*/
-    tmpDirFd = AutoCloseFD{open(tmpDir.c_str(), O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
+    tmpDirRootFd = AutoCloseFD{open(tmpDirRoot.c_str(), O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
+    if (!tmpDirRootFd) {
+        throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDirRoot);
+    }
+
+    // place the actual build directory in a subdirectory of tmpDirRoot. if
+    // we do not do this a build can `chown 777` its build directory and so
+    // make it accessible to everyone in the system, breaking isolation. we
+    // also need the intermediate level to be inaccessible to others. build
+    // processes must be able to at least traverse to the directory though,
+    // without being able to chmod. this means either mode 0750 or 0710. we
+    // cannot use 0710 because the libarchive we link with is compiled with
+    // an old apple sdk that does not have O_SEARCH, which makes libarchive
+    // try to open tmpDirRoot for *read* and fail because g+r is not set. a
+    // future update to nixpkgs may fix this. until then we do not lose any
+    // security by setting mode 0750 because we use only a single subdir in
+    // tmpDirRoot, so being able to list its parent doesn't break anything.
+    //
+    // use a short name to not increase the path length too much on darwin.
+    // darwin has a severe sockaddr_un path length limitation, so this does
+    // make a difference over more evocative names. we use `b` for `build`.
+    tmpDir = tmpDirRoot + "/b";
+    if (mkdirat(tmpDirRootFd.get(), "b", 0700)) {
+        throw SysError("failed to create the build temporary directory '%1%'", tmpDir);
+    }
+    tmpDirFd = AutoCloseFD{openat(tmpDirRootFd.get(), "b", O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
     if (!tmpDirFd)
         throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir);
 
     chownToBuilder(tmpDirFd);
+    if (buildUser) {
+        if (fchown(tmpDirRootFd.get(), -1, buildUser->getGID()) == -1) {
+            throw SysError("cannot change ownership of '%1%'", tmpDirRoot);
+        }
+        if (fchmod(tmpDirRootFd.get(), 0750) == -1) {
+            throw SysError("cannot change mode of '%1%'", tmpDirRoot);
+        }
+    }
 
     for (auto & [outputName, status] : initialOutputs) {
         /* Set scratch path we'll actually use during the build.
@@ -2386,21 +2426,78 @@ try {
     co_return result::current_exception();
 }
 
-
+// make `entry` in `parentFd` visible to the given user and group, preserving
-void LocalDerivationGoal::deleteTmpDir(bool force, bool duringDestruction)
+// inode modes as much as possible. if the builder sets the mode of any inode
+// to not be readable by the owner we keep this; not doing so could interfere
+// with error analysis. if the builder used multiple uids or gids we will not
+// keep them around and instead collapse them all onto the uid/gid given here
+// to not leave around inodes owned by unassigned uids/gids in the system. we
+// also clear setuid/setgid/sticky bits just to be safe even though a builder
+// should not be able to set them to begin, otherwise we may leave setuid/gid
+// executables in the tree even with user/group set to -1/-1. there have been
+// enough bugs of this kind in the past to warrant some extra attention here.
+static void makeVisible(int parentFd, const char * entry, uid_t user, gid_t group)
 {
-    if (tmpDir != "") {
+    struct stat st;
+    if (fstatat(parentFd, entry, &st, AT_SYMLINK_NOFOLLOW)) {
+        throw SysError("fstat(%s)", guessOrInventPathFromFD(parentFd));
+    }
+    if (S_ISDIR(st.st_mode)) {
+        int dirfd = openat(parentFd, entry, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+        if (dirfd < 0) {
+            throw SysError("openat(%s/%s)", guessOrInventPathFromFD(parentFd), entry);
+        }
+        AutoCloseDir dir(fdopendir(dirfd));
+        if (!dir) {
+            close(dirfd);
+            throw SysError("fdopendir(%s/%s)", guessOrInventPathFromFD(parentFd), entry);
+        }
+
+        struct dirent * dirent;
+        while (errno = 0, dirent = readdir(dir.get())) {
+            if (strcmp(dirent->d_name, ".") == 0 || strcmp(dirent->d_name, "..") == 0) {
+                continue;
+            }
+            makeVisible(dirfd, dirent->d_name, user, group);
+        }
+    }
+
+    // ignore permissions errors for symlinks. linux can't chmod them.
+    // clear special permission bits while we're here, just to be safe
+    if (fchmodat(parentFd, entry, st.st_mode & 0777, AT_SYMLINK_NOFOLLOW) && !S_ISLNK(st.st_mode)) {
+        throw SysError("fchmod(%s)", guessOrInventPathFromFD(parentFd));
+    }
+    if (user != uid_t(-1) && group != gid_t(-1)
+        && fchownat(parentFd, entry, user, group, AT_SYMLINK_NOFOLLOW))
+    {
+        throw SysError("fchown(%s)", guessOrInventPathFromFD(parentFd));
+    }
+}
+
+void LocalDerivationGoal::finalizeTmpDir(bool force, bool duringDestruction)
+{
+    if (tmpDirRoot != "") {
         /* Don't keep temporary directories for builtins because they
            might have privileged stuff (like a copy of netrc). */
         if (settings.keepFailed && !force && !drv->isBuiltin()) {
-            printError("note: keeping build directory '%s'", tmpDir);
+            printError("note: keeping build directory '%s'", tmpDirRoot);
-            chmod(tmpDir.c_str(), 0755);
+            try {
+                // always make visible, but don't always chown. if we run as
+                // root we may not want to chown things to root:root so much
+                auto creds = worker.store.associatedCredentials();
+                makeVisible(
+                    tmpDirFd.get(), ".", creds ? creds->user : -1, creds ? creds->group : -1
+                );
+            } catch (SysError & e) {
+                printError("error making '%s' accessible: %s", tmpDir, e.what());
+            }
+            chmod(tmpDirRoot.c_str(), 0755);
         }
         else if (duringDestruction)
-            deletePathUninterruptible(tmpDir);
+            deletePathUninterruptible(tmpDirRoot);
         else
-            deletePath(tmpDir);
+            deletePath(tmpDirRoot);
-        tmpDir = "";
+        tmpDirRoot = "";
     }
 }
 

lix/libstore/build/local-derivation-goal.hh

@@ -29,12 +29,12 @@ struct LocalDerivationGoal : public DerivationGoal
     /**
      * The temporary directory.
      */
-    Path tmpDir;
+    Path tmpDirRoot, tmpDir;
 
     /**
      * The temporary directory file descriptor
      */
-    AutoCloseFD tmpDirFd;
+    AutoCloseFD tmpDirRootFd, tmpDirFd;
 
     /**
      * The path of the temporary directory in the sandbox.
@@ -246,9 +246,12 @@ struct LocalDerivationGoal : public DerivationGoal
     void cleanupPostOutputsRegisteredModeNonCheck() override;
 
     /**
-     * Delete the temporary directory, if we have one.
+     * Delete the temporary directory or make it visible to the user requesting
+     * this build, if a temporary directory was created at all. Temporary files
+     * of derivations using builtin builders are deleted even for `keep-failed`
+     * builds as otherwise we may expose secrets (e.g. from the system .netrc).
      */
-    void deleteTmpDir(bool force, bool duringDestruction = false);
+    void finalizeTmpDir(bool force, bool duringDestruction = false);
 
     /**
      * Forcibly kill the child process, if any.

lix/libstore/local-store.hh

@@ -135,6 +135,8 @@ private:
 
     Sync<GCState> _gcState;
 
+    std::optional<AssociatedCredentials> association;
+
 public:
 
     const Path dbDir;
@@ -148,6 +150,16 @@ public:
     LocalStoreConfig & config() override { return config_; }
     const LocalStoreConfig & config() const override { return config_; }
 
+    std::optional<AssociatedCredentials> associatedCredentials() const override
+    {
+        return association;
+    }
+
+    void associateWithCredentials(uid_t user, gid_t group)
+    {
+        association = {user, group};
+    }
+
 private:
 
     const PublicKeys & getPublicKeys();

lix/libstore/store-api.hh

@@ -234,6 +234,22 @@ protected:
     Store(const StoreConfig & config);
 
 public:
+    struct AssociatedCredentials
+    {
+        uid_t user;
+        gid_t group;
+    };
+
+    /**
+     * Credentials of the context using this store if this store is proxied
+     * to somewhere else and the peer context is known. Only the daemon can
+     * set this to values that make any sense, using unix peer credentials.
+     */
+    virtual std::optional<AssociatedCredentials> associatedCredentials() const
+    {
+        return {};
+    }
+
     /**
      * Perform any necessary effectful operation to make the store up and
      * running

lix/nix/daemon.cc

@@ -364,11 +364,17 @@ static void daemonLoopImpl(std::optional<TrustedFlag> forceTrustClientOpt)
                     strncpy(savedArgv[1], processName.c_str(), strlen(savedArgv[1]));
                 }
 
+                auto store = aio.blockOn(openUncachedStore());
+                if (auto local = dynamic_cast<LocalStore *>(&*store); local && peer.uidKnown && peer.gidKnown) {
+                    local->associateWithCredentials(peer.uid, peer.gid);
+                }
+
                 //  Handle the connection.
                 FdSource from(remote.get());
                 FdSink to(remote.get());
+
                 processConnection(
-                    aio, aio.blockOn(openUncachedStore()), from, to, trusted
+                    aio, store, from, to, trusted
                 );
 
                 exit(0);

package.nix

@@ -135,10 +135,10 @@ let
 
   # This could be the dtrace for macOS, etc, but I have no idea if it is
   # packaged or if it works.
-  dtrace-generator = lib.optional withDtrace systemtap-lix;
+  dtrace-generator = if withDtrace then systemtap-lix else null;
 
   # This is for sys/sdt.h
-  dtrace-headers = lib.optional withDtrace libsystemtap;
+  dtrace-headers = if withDtrace then libsystemtap else null;
 
   aws-sdk-cpp-nix =
     if aws-sdk-cpp == null then
@@ -209,6 +209,8 @@ assert (lintInsteadOfBuild -> lix-clang-tidy != null);
 stdenv.mkDerivation (finalAttrs: {
   inherit pname version;
 
+  __structuredAttrs = true;
+
   src = fileset.toSource {
     root = ./.;
     fileset = fileset.intersection baseFiles (
@@ -229,8 +231,6 @@ stdenv.mkDerivation (finalAttrs: {
     );
   };
 
-  VERSION_SUFFIX = versionSuffix;
-
   outputs =
     [ "out" ]
     ++ lib.optionals (!finalAttrs.dontBuild) [
@@ -364,9 +364,9 @@ stdenv.mkDerivation (finalAttrs: {
     lixPythonForBuild
   ];
 
-  # Needed for Meson to find Boost.
-  # https://github.com/NixOS/nixpkgs/issues/86131.
   env = {
+    # Needed for Meson to find Boost.
+    # https://github.com/NixOS/nixpkgs/issues/86131.
     BOOST_INCLUDEDIR = "${lib.getDev boost}/include";
     BOOST_LIBRARYDIR = "${lib.getLib boost}/lib";
 
@@ -374,6 +374,8 @@ stdenv.mkDerivation (finalAttrs: {
     # Turns out the Nix-generated Cargo dependencies are named the same as they
     # would be in a Cargo registry cache.
     MESON_PACKAGE_CACHE_DIR = finalAttrs.cargoDeps;
+
+    VERSION_SUFFIX = versionSuffix;
   };
 
   cargoDeps = rustPlatform.importCargoLock { lockFile = ./Cargo.lock; };

tests/functional/build-remote.sh

@@ -80,4 +80,4 @@ out="$(nix-build 2>&1 failing.nix \
 [[ "$out" =~ .*"note: keeping build directory".* ]]
 
 build_dir="$(grep "note: keeping build" <<< "$out" | sed -E "s/^(.*)note: keeping build directory '(.*)'(.*)$/\2/")"
-[[ "foo" = $(<"$build_dir"/bar) ]]
+[[ "foo" = $(<"$build_dir"/b/bar) ]]

tests/functional/build.sh

@@ -179,3 +179,23 @@ test "$(<<<"$out" grep -E '^error:' | wc -l)" = 3
 BUILD_DIR=$(mktemp -d)
 chmod 0000 "$BUILD_DIR"
 nix --build-dir "$BUILD_DIR" build -E 'with import ./config.nix; mkDerivation { name = "test"; buildCommand = "echo rawr > $out"; }' --impure --no-link
+
+# ensure that the build directory parent is not world-accessible
+chmod 0755 "$BUILD_DIR"
+FIFO="$BUILD_DIR/fifo"
+mkfifo "$FIFO"
+(
+  echo > "$FIFO"
+  trap 'echo > "$FIFO"' EXIT
+  mode=$(stat -c %a $BUILD_DIR/b/*)
+  [ "$mode" = "700" -o "$mode" = "710" ]
+) &
+nix build --build-dir "$BUILD_DIR/b" -E '
+  with import ./config.nix; mkDerivation {
+    name = "test";
+    buildCommand = "cat '"$FIFO"'; cat '"$FIFO"' > $out";
+  }' \
+  --extra-sandbox-paths "$FIFO" \
+  --impure \
+  --no-link
+wait

tests/functional/check.sh

@@ -45,7 +45,7 @@ test_custom_build_dir() {
   [ "$status" = "100" ]
   [[ 1 == "$(count "$customBuildDir/nix-build-"*)" ]]
   local buildDir="$customBuildDir/nix-build-"*
-  grep $checkBuildId $buildDir/checkBuildId
+  grep $checkBuildId $buildDir/b/checkBuildId
 }
 test_custom_build_dir
 

tests/nixos/chown-to-user.nix

@@ -0,0 +1,74 @@
+{ lib, config, pkgs, ... }:
+
+let
+  failedNormal = config: pkgs.writeText "failed.nix" ''
+    let utils = builtins.storePath ${config.system.build.extraUtils}; in
+    derivation {
+      name = "failed";
+      system = builtins.currentSystem;
+      PATH = "''${utils}/bin";
+      builder = "''${utils}/bin/sh";
+      args = [ "-c" "mkdir dir; echo test > dir/file" ];
+    }
+  '';
+
+  failedBuiltin = pkgs.writeText "failed.nix" ''
+    derivation {
+      name = "failed";
+      system = builtins.currentSystem;
+      builder = "builtin:fetchurl";
+      url = "http://localhost/foo";
+      outputHashMode = "flat";
+    }
+  '';
+in
+{
+  name = "chown-to-user";
+
+  nodes = {
+    machine = { lib, pkgs, ... }: {
+      virtualisation.writableStore = true;
+
+      users.users.test = {
+        isNormalUser = true;
+        group = "test";
+      };
+      users.groups.test = {};
+      nix.nrBuildUsers = 1;
+    };
+  };
+
+  testScript = { nodes, ... }: ''
+    import re
+
+    machine.wait_for_unit("multi-user.target")
+
+    # builds using the daemon chown tempdirs
+    out = machine.fail("runuser -u test -- nix-build ${failedNormal nodes.machine} --keep-failed 2>&1")
+    dir = re.search("keeping build directory '(.+?)'", out)
+    assert dir
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}").strip() == "root:nixbld:755"
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b").strip() == "test:test:700"
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir").strip() == "test:test:755"
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir/file").strip() == "test:test:644"
+
+    # builds not using the daemon do not chown tempdirs
+    out = machine.fail("NIX_REMOTE=local nix-build ${failedNormal nodes.machine} --keep-failed 2>&1")
+    dir = re.search("keeping build directory '(.+?)'", out)
+    assert dir
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}").strip() == "root:nixbld:755"
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b").strip() == "nixbld1:nixbld:700"
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir").strip() == "nixbld1:nixbld:755"
+    assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir/file").strip() == "nixbld1:nixbld:644"
+
+    # builds using builtin builders using the daemon do not keep tempdirs
+    out = machine.fail("runuser -u test -- nix-build ${failedBuiltin} --keep-failed 2>&1")
+    dir = re.search("keeping build directory '(.+?)'", out)
+    assert not dir
+
+    # builds using builtin builders not using the daemon do not keep tempdirs
+    out = machine.fail("NIX_REMOTE=local nix-build ${failedBuiltin} --keep-failed 2>&1")
+    dir = re.search("keeping build directory '(.+?)'", out)
+    assert not dir
+  '';
+}

tests/nixos/default.nix

@@ -180,4 +180,6 @@ in
   io_uring = runNixOSTestFor "x86_64-linux" ./io_uring;
 
   fetchurl = runNixOSTestFor "x86_64-linux" ./fetchurl.nix;
+
+  chown-to-user = runNixOSTestFor "x86_64-linux" ./chown-to-user.nix;
 }

version.json

@@ -1,5 +1,5 @@
 {
-    "version": "2.93.2",
+    "version": "2.93.3",
     "official_release": true,
     "release_name": "Bici Bici"
 }