From aa9739caa7e8cdf7e472e93b80f2486d691c747b Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Thu, 17 Jul 2025 00:44:38 +0200 Subject: [PATCH] libstore: add intermediate directory to build-dirs this makes the actual build directories used by builders invisible and inaccessible to other processes on the system, avoiding another vector for outside processes to interfere with builds or pass credentials the build sandbox should not have access to into the build sandbox anyway. fixes #919 Change-Id: Ifaa4d8e3940cfde1406e925f75c1375d2e86d81a (cherry picked from commit 9d5a5c4dc09c5fc924e7f343c2d75408a998dd9f) --- src/libstore/build/local-derivation-goal.cc | 47 +++++++++++++++++---- src/libstore/build/local-derivation-goal.hh | 4 +- tests/functional/build-remote.sh | 2 +- tests/functional/build.sh | 20 +++++++++ tests/functional/check.sh | 2 +- 5 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 8d98199f0..523fe701b 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -332,7 +332,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() if (statvfs(localStore.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) diskFull = true; } @@ -482,7 +482,7 @@ void LocalDerivationGoal::startBuilder() /* 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) { /* @@ -512,18 +512,47 @@ void LocalDerivationGoal::startBuilder() 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 + // use 0710 just to be extra safe; if we ever add more directories they + // will not be enumerable to other processes in the builder user group. + // + // 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(), 0710) == -1) { + throw SysError("cannot change mode of '%1%'", tmpDirRoot); + } + } + for (auto & [outputName, status] : initialOutputs) { /* Set scratch path we'll actually use during the build. @@ -2682,16 +2711,16 @@ void LocalDerivationGoal::checkOutputs(const std::mapisBuiltin()) { - printError("note: keeping build directory '%s'", tmpDir); - chmod(tmpDir.c_str(), 0755); + printError("note: keeping build directory '%s'", tmpDirRoot); + chmod(tmpDirRoot.c_str(), 0755); } else - deletePath(tmpDir); - tmpDir = ""; + deletePath(tmpDirRoot); + tmpDirRoot = ""; } } diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 05e7588ac..7bfb413ce 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/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. diff --git a/tests/functional/build-remote.sh b/tests/functional/build-remote.sh index 36059d8a8..b783f3a05 100644 --- a/tests/functional/build-remote.sh +++ b/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) ]] diff --git a/tests/functional/build.sh b/tests/functional/build.sh index b04282666..1178eb8f5 100644 --- a/tests/functional/build.sh +++ b/tests/functional/build.sh @@ -175,3 +175,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 diff --git a/tests/functional/check.sh b/tests/functional/check.sh index 38883c5d7..4a4bc63e6 100644 --- a/tests/functional/check.sh +++ b/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