From 9d5a5c4dc09c5fc924e7f343c2d75408a998dd9f 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 --- lix/libstore/build/local-derivation-goal.cc | 52 ++++++++++++++++----- lix/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, 65 insertions(+), 15 deletions(-) diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 4de0a41c0..b687fc3bd 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -359,9 +359,10 @@ 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 && - (uint64_t) st.f_bavail * st.f_bsize < required) + if (statvfs(tmpDirRoot.c_str(), &st) == 0 && (uint64_t) st.f_bavail * st.f_bsize < required) + { diskFull = true; + } } #endif @@ -439,7 +440,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) { /* @@ -470,17 +471,46 @@ 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 + // 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. @@ -2357,18 +2387,18 @@ try { void LocalDerivationGoal::deleteTmpDir(bool force, bool duringDestruction) { - if (tmpDir != "") { + 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); - chmod(tmpDir.c_str(), 0755); + printError("note: keeping build directory '%s'", tmpDirRoot); + chmod(tmpDirRoot.c_str(), 0755); } else if (duringDestruction) - deletePathUninterruptible(tmpDir); + deletePathUninterruptible(tmpDirRoot); else - deletePath(tmpDir); - tmpDir = ""; + deletePath(tmpDirRoot); + tmpDirRoot = ""; } } diff --git a/lix/libstore/build/local-derivation-goal.hh b/lix/libstore/build/local-derivation-goal.hh index a0031e141..ff3afa150 100644 --- a/lix/libstore/build/local-derivation-goal.hh +++ b/lix/libstore/build/local-derivation-goal.hh @@ -40,12 +40,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 c7aa09745..509978291 100644 --- a/tests/functional/build-remote.sh +++ b/tests/functional/build-remote.sh @@ -77,4 +77,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 fc83f61f3..70f79e32e 100644 --- a/tests/functional/build.sh +++ b/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 diff --git a/tests/functional/check.sh b/tests/functional/check.sh index e6d017aa1..241810f6b 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