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