From 7226a116a0bbc1f304d8160615525cb0aeb46096 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Wed, 26 Mar 2025 01:04:12 +0100 Subject: [PATCH 01/15] libutil: guess or invent a path from file descriptors This is useful for certain error recovery paths (no pun intended) that does not thread through the original path name. Change-Id: I2d800740cb4f9912e64c923120d3f977c58ccb7e Signed-off-by: Raito Bezarius --- src/libutil/file-descriptor.cc | 26 +++++++++++++++++++ .../include/nix/util/file-descriptor.hh | 17 ++++++++++++ src/libutil/meson.build | 2 ++ 3 files changed, 45 insertions(+) diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 9e0827442..9193a30bb 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -1,5 +1,8 @@ #include "nix/util/serialise.hh" #include "nix/util/util.hh" +#include "nix/util/file-system.hh" + +#include "util-config-private.hh" #include #include @@ -74,6 +77,29 @@ Descriptor AutoCloseFD::get() const return fd; } +std::string guessOrInventPathFromFD(Descriptor fd) + { + assert(fd >= 0); + /* On Linux, there's no F_GETPATH available. + * But we can read /proc/ */ + #if defined(__linux__) + try { + return readLink(fmt("/proc/self/fd/%1%", fd)); + } catch (...) { + } + #elif defined (HAVE_F_GETPATH) && HAVE_F_GETPATH + std::string fdName(PATH_MAX, '\0'); + if (fcntl(fd, F_GETPATH, fdName.data()) != -1) { + fdName.resize(strlen(fdName.c_str())); + return fdName; + } + #else + #error "No implementation for retrieving file descriptors path." + #endif + + return fmt("", fd); + } + void AutoCloseFD::close() { diff --git a/src/libutil/include/nix/util/file-descriptor.hh b/src/libutil/include/nix/util/file-descriptor.hh index e2bcce2a2..35b359fb5 100644 --- a/src/libutil/include/nix/util/file-descriptor.hh +++ b/src/libutil/include/nix/util/file-descriptor.hh @@ -106,6 +106,14 @@ void drainFD( #endif ); + /* + * Will attempt to guess *A* path associated that might lead to the same file as used by this + * file descriptor. + * + * The returned string should NEVER be used as a valid path. + */ + std::string guessOrInventPathFromFD(Descriptor fd); + /** * Get [Standard Input](https://en.wikipedia.org/wiki/Standard_streams#Standard_input_(stdin)) */ @@ -160,6 +168,15 @@ public: AutoCloseFD& operator =(const AutoCloseFD & fd) = delete; AutoCloseFD& operator =(AutoCloseFD&& fd); Descriptor get() const; + + /** + * Will attempt to guess *A* path associated that might lead to the same file as used by this + * file descriptor. + * + * The returned string should NEVER be used as a valid path. + */ + std::string guessOrInventPath() const { return guessOrInventPathFromFD(fd); } + explicit operator bool() const; Descriptor release(); void close(); diff --git a/src/libutil/meson.build b/src/libutil/meson.build index f5ad2b1f6..2203e2294 100644 --- a/src/libutil/meson.build +++ b/src/libutil/meson.build @@ -37,6 +37,8 @@ foreach funcspec : check_funcs configdata.set(define_name, define_value, description: funcspec[1]) endforeach +configdata.set('HAVE_F_GETPATH', cxx.has_header_symbol('fcntl.h', 'F_GETPATH').to_int()) + subdir('nix-meson-build-support/libatomic') if host_machine.system() == 'windows' From 6a5b6ad3b75a1032ee495cec456e8d28b7e0595e Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Wed, 26 Mar 2025 01:04:59 +0100 Subject: [PATCH 02/15] libstore: open build directory as a dirfd as well We now keep around a proper AutoCloseFD around the temporary directory which we plan to use for openat operations and avoiding the build directory being swapped out while we are doing something else. Change-Id: I18d387b0f123ebf2d20c6405cd47ebadc5505f2a Signed-off-by: Raito Bezarius --- src/libstore/unix/build/derivation-builder.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 85b586373..e2dfc5860 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -95,6 +95,11 @@ protected: */ Path topTmpDir; + /** + * The file descriptor of the temporary directory. + */ + AutoCloseFD tmpDirFd; + /** * The sort of derivation we are building. * @@ -710,6 +715,13 @@ void DerivationBuilderImpl::startBuilder() topTmpDir = createTempDir(settings.buildDir.get().value_or(""), "nix-build-" + std::string(drvPath.name()), 0700); setBuildTmpDir(); assert(!tmpDir.empty()); + + /* 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)}; + if (!tmpDirFd) + throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir); + chownToBuilder(tmpDir); for (auto & [outputName, status] : initialOutputs) { From 002d202653a2750872ed7b943363ce4029af4dec Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Wed, 26 Mar 2025 01:05:34 +0100 Subject: [PATCH 03/15] libstore: chown to builder variant for file descriptors We use it immediately for the build temporary directory. Change-Id: I180193c63a2b98721f5fb8e542c4e39c099bb947 Signed-off-by: Raito Bezarius --- src/libstore/unix/build/derivation-builder.cc | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index e2dfc5860..30468d3b2 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -305,9 +305,17 @@ protected: /** * Make a file owned by the builder. + * + * SAFETY: this function is prone to TOCTOU as it receives a path and not a descriptor. + * It's only safe to call in a child of a directory only visible to the owner. */ void chownToBuilder(const Path & path); + /** + * Make a file owned by the builder addressed by its file descriptor. + */ + void chownToBuilder(const AutoCloseFD & fd); + /** * Run the builder's process. */ @@ -722,7 +730,7 @@ void DerivationBuilderImpl::startBuilder() if (!tmpDirFd) throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir); - chownToBuilder(tmpDir); + chownToBuilder(tmpDirFd); for (auto & [outputName, status] : initialOutputs) { /* Set scratch path we'll actually use during the build. @@ -1267,6 +1275,13 @@ void DerivationBuilderImpl::chownToBuilder(const Path & path) throw SysError("cannot change ownership of '%1%'", path); } +void DerivationBuilderImpl::chownToBuilder(const AutoCloseFD & fd) +{ + if (!buildUser) return; + if (fchown(fd.get(), buildUser->getUID(), buildUser->getGID()) == -1) + throw SysError("cannot change ownership of file '%1%'", fd.guessOrInventPath()); +} + void DerivationBuilderImpl::runChild() { /* Warning: in the child we should absolutely not make any SQLite From 034f59bbb9a8c6a5febfed042fa9119410a3f123 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Wed, 26 Mar 2025 01:06:03 +0100 Subject: [PATCH 04/15] libutil: writeFile variant for file descriptors `writeFile` lose its `sync` boolean flag to make things simpler. A new `writeFileAndSync` function is created and all call sites are converted to it. Change-Id: Ib871a5283a9c047db1e4fe48a241506e4aab9192 Signed-off-by: Raito Bezarius --- src/libstore/local-store.cc | 4 +-- src/libutil/file-system.cc | 35 ++++++++++++++++----- src/libutil/include/nix/util/file-system.hh | 13 ++++++-- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e53cab2dc..00c1ac6dd 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -247,7 +247,7 @@ LocalStore::LocalStore(ref config) else if (curSchema == 0) { /* new store */ curSchema = nixSchemaVersion; openDB(*state, true); - writeFile(schemaPath, fmt("%1%", curSchema), 0666, true); + writeFileAndSync(schemaPath, fmt("%1%", curSchema), 0666); } else if (curSchema < nixSchemaVersion) { @@ -298,7 +298,7 @@ LocalStore::LocalStore(ref config) txn.commit(); } - writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, true); + writeFileAndSync(schemaPath, fmt("%1%", nixSchemaVersion), 0666); lockFile(globalLock.get(), ltRead, true); } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index fee2945ff..4c3a56e65 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -304,7 +304,7 @@ void readFile(const Path & path, Sink & sink, bool memory_map) } -void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync) +void writeFile(const Path & path, std::string_view s, mode_t mode) { AutoCloseFD fd = toDescriptor(open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT // TODO @@ -314,20 +314,39 @@ void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync) , mode)); if (!fd) throw SysError("opening file '%1%'", path); + + writeFile(fd, s, mode); + + /* Close explicitly to propagate the exceptions. */ + fd.close(); +} + +void writeFile(AutoCloseFD & fd, std::string_view s, mode_t mode) +{ + assert(fd); try { writeFull(fd.get(), s); } catch (Error & e) { - e.addTrace({}, "writing file '%1%'", path); + e.addTrace({}, "writing file '%1%'", fd.guessOrInventPath()); throw; } - if (sync) - fd.fsync(); - // Explicitly close to make sure exceptions are propagated. - fd.close(); - if (sync) - syncParent(path); } +void writeFileAndSync(const Path & path, std::string_view s, mode_t mode) +{ + { + AutoCloseFD fd{open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode)}; + if (!fd) + throw SysError("opening file '%1%'", path); + + writeFile(fd, s, mode); + fd.fsync(); + /* Close explicitly to ensure that exceptions are propagated. */ + fd.close(); + } + + syncParent(path); +} void writeFile(const Path & path, Source & source, mode_t mode, bool sync) { diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index 0121745ab..77d5f2aa1 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -172,10 +172,10 @@ void readFile(const Path & path, Sink & sink, bool memory_map = true); /** * Write a string to a file. */ -void writeFile(const Path & path, std::string_view s, mode_t mode = 0666, bool sync = false); -static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666, bool sync = false) +void writeFile(const Path & path, std::string_view s, mode_t mode = 0666); +static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666) { - return writeFile(path.string(), s, mode, sync); + return writeFile(path.string(), s, mode); } void writeFile(const Path & path, Source & source, mode_t mode = 0666, bool sync = false); @@ -184,6 +184,13 @@ static inline void writeFile(const std::filesystem::path & path, Source & source return writeFile(path.string(), source, mode, sync); } +void writeFile(AutoCloseFD & fd, std::string_view s, mode_t mode = 0666 ); + +/** + * Write a string to a file and flush the file and its parents direcotry to disk. + */ +void writeFileAndSync(const Path & path, std::string_view s, mode_t mode = 0666); + /** * Flush a path's parent directory to disk. */ From 4e59d3fdb25335d7ab2612e72ff191e9bfbba8f4 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Wed, 26 Mar 2025 01:07:47 +0100 Subject: [PATCH 05/15] libstore: ensure that `passAsFile` is created in the original temp dir This ensures that `passAsFile` data is created inside the expected temporary build directory by `openat()` from the parent directory file descriptor. This avoids a TOCTOU which is part of the attack chain of CVE-????. Change-Id: Ie5273446c4a19403088d0389ae8e3f473af8879a Signed-off-by: Raito Bezarius --- src/libstore/unix/build/derivation-builder.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 30468d3b2..22445d547 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1070,8 +1070,11 @@ void DerivationBuilderImpl::initEnv() auto hash = hashString(HashAlgorithm::SHA256, i.first); std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false); Path p = tmpDir + "/" + fn; - writeFile(p, rewriteStrings(i.second, inputRewrites)); - chownToBuilder(p); + AutoCloseFD passAsFileFd{openat(tmpDirFd.get(), fn.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666)}; + if (!passAsFileFd) + throw SysError("opening `passAsFile` file in the sandbox '%1%'", p); + writeFile(passAsFileFd, rewriteStrings(i.second, inputRewrites)); + chownToBuilder(passAsFileFd); env[i.first + "Path"] = tmpDirInSandbox() + "/" + fn; } } From 5ec047f348d747d079bfb5d92a10a6c8d446089b Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Wed, 26 Mar 2025 12:42:55 +0100 Subject: [PATCH 06/15] libutil: ensure that `_deletePath` does NOT use absolute paths with dirfds When calling `_deletePath` with a parent file descriptor, `openat` is made effective by using relative paths to the directory file descriptor. To avoid the problem, the signature is changed to resist misuse with an assert in the prologue of the function. Change-Id: I6b3fc766bad2afe54dc27d47d1df3873e188de96 Signed-off-by: Raito Bezarius --- src/libutil/file-system.cc | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 4c3a56e65..12ada6d5c 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -444,6 +444,8 @@ void recursiveSync(const Path & path) static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, uint64_t & bytesFreed, std::exception_ptr & ex MOUNTEDPATHS_PARAM) { #ifndef _WIN32 + /* This ensures that `name` is an immediate child of `parentfd`. */ + assert(!path.empty() && path.string().find('/') == std::string::npos && "`name` is an immediate child to `parentfd`"); checkInterrupt(); #ifdef __FreeBSD__ @@ -454,13 +456,13 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, } #endif - std::string name(baseNameOf(path.native())); + std::string name(path.native()); struct stat st; if (fstatat(parentfd, name.c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) { if (errno == ENOENT) return; - throw SysError("getting status of %1%", path); + throw SysError("getting status of '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); } if (!S_ISDIR(st.st_mode)) { @@ -491,23 +493,24 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, /* Make the directory accessible. */ const auto PERM_MASK = S_IRUSR | S_IWUSR | S_IXUSR; if ((st.st_mode & PERM_MASK) != PERM_MASK) { - if (fchmodat(parentfd, name.c_str(), st.st_mode | PERM_MASK, 0) == -1) - throw SysError("chmod %1%", path); + if (fchmodat(parentfd, name.c_str(), st.st_mode | PERM_MASK, 0) == -1) { + throw SysError("chmod '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); + } } - int fd = openat(parentfd, path.c_str(), O_RDONLY); + int fd = openat(parentfd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW); if (fd == -1) - throw SysError("opening directory %1%", path); + throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); AutoCloseDir dir(fdopendir(fd)); if (!dir) - throw SysError("opening directory %1%", path); + throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); struct dirent * dirent; while (errno = 0, dirent = readdir(dir.get())) { /* sic */ checkInterrupt(); std::string childName = dirent->d_name; if (childName == "." || childName == "..") continue; - _deletePath(dirfd(dir.get()), path + "/" + childName, bytesFreed, ex MOUNTEDPATHS_ARG); + _deletePath(dirfd(dir.get()), childName, bytesFreed, ex MOUNTEDPATHS_ARG); } if (errno) throw SysError("reading directory %1%", path); } @@ -516,7 +519,7 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, if (unlinkat(parentfd, name.c_str(), flags) == -1) { if (errno == ENOENT) return; try { - throw SysError("cannot unlink %1%", path); + throw SysError("cannot unlink '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); } catch (...) { if (!ex) ex = std::current_exception(); @@ -544,7 +547,7 @@ static void _deletePath(const std::filesystem::path & path, uint64_t & bytesFree std::exception_ptr ex; - _deletePath(dirfd.get(), path, bytesFreed, ex MOUNTEDPATHS_ARG); + _deletePath(dirfd.get(), path.filename(), bytesFreed, ex MOUNTEDPATHS_ARG); if (ex) std::rethrow_exception(ex); From 4ea4813753c895118b7377495647b48f240ff4d8 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Thu, 27 Mar 2025 12:22:26 +0100 Subject: [PATCH 07/15] libstore: ensure that temporary directory is always 0o000 before deletion In the case the deletion fails, we should ensure that the temporary directory cannot be used for nefarious purposes. Change-Id: I498a2dd0999a74195d13642f44a5de1e69d46120 Signed-off-by: Raito Bezarius --- src/libstore/unix/build/derivation-builder.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 22445d547..c61fe7001 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -2093,6 +2093,15 @@ void DerivationBuilderImpl::checkOutputs(const std::map Date: Thu, 12 Jun 2025 11:15:58 +0200 Subject: [PATCH 08/15] Tweak comment --- src/libstore/unix/build/derivation-builder.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index c61fe7001..a125676e5 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -2093,8 +2093,8 @@ void DerivationBuilderImpl::checkOutputs(const std::map Date: Thu, 12 Jun 2025 11:35:28 +0200 Subject: [PATCH 09/15] Drop guessOrInventPathFromFD() No need to do hacky stuff like that when we already know the original path. --- src/libstore/unix/build/derivation-builder.cc | 14 +++---- src/libutil/file-descriptor.cc | 26 ------------- src/libutil/file-system.cc | 39 +++++++++---------- .../include/nix/util/file-descriptor.hh | 17 -------- src/libutil/include/nix/util/file-system.hh | 4 +- src/libutil/meson.build | 2 - 6 files changed, 28 insertions(+), 74 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index a125676e5..36405a8a2 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -314,7 +314,7 @@ protected: /** * Make a file owned by the builder addressed by its file descriptor. */ - void chownToBuilder(const AutoCloseFD & fd); + void chownToBuilder(int fd, const Path & path); /** * Run the builder's process. @@ -730,7 +730,7 @@ void DerivationBuilderImpl::startBuilder() if (!tmpDirFd) throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir); - chownToBuilder(tmpDirFd); + chownToBuilder(tmpDirFd.get(), tmpDir); for (auto & [outputName, status] : initialOutputs) { /* Set scratch path we'll actually use during the build. @@ -1073,8 +1073,8 @@ void DerivationBuilderImpl::initEnv() AutoCloseFD passAsFileFd{openat(tmpDirFd.get(), fn.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666)}; if (!passAsFileFd) throw SysError("opening `passAsFile` file in the sandbox '%1%'", p); - writeFile(passAsFileFd, rewriteStrings(i.second, inputRewrites)); - chownToBuilder(passAsFileFd); + writeFile(passAsFileFd, p, rewriteStrings(i.second, inputRewrites)); + chownToBuilder(passAsFileFd.get(), p); env[i.first + "Path"] = tmpDirInSandbox() + "/" + fn; } } @@ -1278,11 +1278,11 @@ void DerivationBuilderImpl::chownToBuilder(const Path & path) throw SysError("cannot change ownership of '%1%'", path); } -void DerivationBuilderImpl::chownToBuilder(const AutoCloseFD & fd) +void DerivationBuilderImpl::chownToBuilder(int fd, const Path & path) { if (!buildUser) return; - if (fchown(fd.get(), buildUser->getUID(), buildUser->getGID()) == -1) - throw SysError("cannot change ownership of file '%1%'", fd.guessOrInventPath()); + if (fchown(fd, buildUser->getUID(), buildUser->getGID()) == -1) + throw SysError("cannot change ownership of file '%1%'", path); } void DerivationBuilderImpl::runChild() diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 9193a30bb..9e0827442 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -1,8 +1,5 @@ #include "nix/util/serialise.hh" #include "nix/util/util.hh" -#include "nix/util/file-system.hh" - -#include "util-config-private.hh" #include #include @@ -77,29 +74,6 @@ Descriptor AutoCloseFD::get() const return fd; } -std::string guessOrInventPathFromFD(Descriptor fd) - { - assert(fd >= 0); - /* On Linux, there's no F_GETPATH available. - * But we can read /proc/ */ - #if defined(__linux__) - try { - return readLink(fmt("/proc/self/fd/%1%", fd)); - } catch (...) { - } - #elif defined (HAVE_F_GETPATH) && HAVE_F_GETPATH - std::string fdName(PATH_MAX, '\0'); - if (fcntl(fd, F_GETPATH, fdName.data()) != -1) { - fdName.resize(strlen(fdName.c_str())); - return fdName; - } - #else - #error "No implementation for retrieving file descriptors path." - #endif - - return fmt("", fd); - } - void AutoCloseFD::close() { diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 12ada6d5c..3fd5ae669 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -315,19 +315,19 @@ void writeFile(const Path & path, std::string_view s, mode_t mode) if (!fd) throw SysError("opening file '%1%'", path); - writeFile(fd, s, mode); + writeFile(fd, path, s, mode); /* Close explicitly to propagate the exceptions. */ fd.close(); } -void writeFile(AutoCloseFD & fd, std::string_view s, mode_t mode) +void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode) { assert(fd); try { writeFull(fd.get(), s); } catch (Error & e) { - e.addTrace({}, "writing file '%1%'", fd.guessOrInventPath()); + e.addTrace({}, "writing file '%1%'", origPath); throw; } } @@ -339,7 +339,7 @@ void writeFileAndSync(const Path & path, std::string_view s, mode_t mode) if (!fd) throw SysError("opening file '%1%'", path); - writeFile(fd, s, mode); + writeFile(fd, path, s, mode); fd.fsync(); /* Close explicitly to ensure that exceptions are propagated. */ fd.close(); @@ -444,8 +444,6 @@ void recursiveSync(const Path & path) static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, uint64_t & bytesFreed, std::exception_ptr & ex MOUNTEDPATHS_PARAM) { #ifndef _WIN32 - /* This ensures that `name` is an immediate child of `parentfd`. */ - assert(!path.empty() && path.string().find('/') == std::string::npos && "`name` is an immediate child to `parentfd`"); checkInterrupt(); #ifdef __FreeBSD__ @@ -456,13 +454,14 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, } #endif - std::string name(path.native()); + std::string name(path.filename()); + assert(name != "." && name != ".." && !name.empty()); struct stat st; if (fstatat(parentfd, name.c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) { if (errno == ENOENT) return; - throw SysError("getting status of '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); + throw SysError("getting status of %1%", path); } if (!S_ISDIR(st.st_mode)) { @@ -493,24 +492,23 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, /* Make the directory accessible. */ const auto PERM_MASK = S_IRUSR | S_IWUSR | S_IXUSR; if ((st.st_mode & PERM_MASK) != PERM_MASK) { - if (fchmodat(parentfd, name.c_str(), st.st_mode | PERM_MASK, 0) == -1) { - throw SysError("chmod '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); - } + if (fchmodat(parentfd, name.c_str(), st.st_mode | PERM_MASK, 0) == -1) + throw SysError("chmod %1%", path); } int fd = openat(parentfd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW); if (fd == -1) - throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); + throw SysError("opening directory %1%", path); AutoCloseDir dir(fdopendir(fd)); if (!dir) - throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); + throw SysError("opening directory %1%", path); struct dirent * dirent; while (errno = 0, dirent = readdir(dir.get())) { /* sic */ checkInterrupt(); std::string childName = dirent->d_name; if (childName == "." || childName == "..") continue; - _deletePath(dirfd(dir.get()), childName, bytesFreed, ex MOUNTEDPATHS_ARG); + _deletePath(dirfd(dir.get()), path / childName, bytesFreed, ex MOUNTEDPATHS_ARG); } if (errno) throw SysError("reading directory %1%", path); } @@ -519,7 +517,7 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, if (unlinkat(parentfd, name.c_str(), flags) == -1) { if (errno == ENOENT) return; try { - throw SysError("cannot unlink '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); + throw SysError("cannot unlink %1%", path); } catch (...) { if (!ex) ex = std::current_exception(); @@ -535,19 +533,18 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path, static void _deletePath(const std::filesystem::path & path, uint64_t & bytesFreed MOUNTEDPATHS_PARAM) { - Path dir = dirOf(path.string()); - if (dir == "") - dir = "/"; + assert(path.is_absolute()); + assert(path.parent_path() != path); - AutoCloseFD dirfd = toDescriptor(open(dir.c_str(), O_RDONLY)); + AutoCloseFD dirfd = toDescriptor(open(path.parent_path().string().c_str(), O_RDONLY)); if (!dirfd) { if (errno == ENOENT) return; - throw SysError("opening directory '%1%'", path); + throw SysError("opening directory %s", path.parent_path()); } std::exception_ptr ex; - _deletePath(dirfd.get(), path.filename(), bytesFreed, ex MOUNTEDPATHS_ARG); + _deletePath(dirfd.get(), path, bytesFreed, ex MOUNTEDPATHS_ARG); if (ex) std::rethrow_exception(ex); diff --git a/src/libutil/include/nix/util/file-descriptor.hh b/src/libutil/include/nix/util/file-descriptor.hh index 35b359fb5..e2bcce2a2 100644 --- a/src/libutil/include/nix/util/file-descriptor.hh +++ b/src/libutil/include/nix/util/file-descriptor.hh @@ -106,14 +106,6 @@ void drainFD( #endif ); - /* - * Will attempt to guess *A* path associated that might lead to the same file as used by this - * file descriptor. - * - * The returned string should NEVER be used as a valid path. - */ - std::string guessOrInventPathFromFD(Descriptor fd); - /** * Get [Standard Input](https://en.wikipedia.org/wiki/Standard_streams#Standard_input_(stdin)) */ @@ -168,15 +160,6 @@ public: AutoCloseFD& operator =(const AutoCloseFD & fd) = delete; AutoCloseFD& operator =(AutoCloseFD&& fd); Descriptor get() const; - - /** - * Will attempt to guess *A* path associated that might lead to the same file as used by this - * file descriptor. - * - * The returned string should NEVER be used as a valid path. - */ - std::string guessOrInventPath() const { return guessOrInventPathFromFD(fd); } - explicit operator bool() const; Descriptor release(); void close(); diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index 77d5f2aa1..5f062412d 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -173,18 +173,20 @@ void readFile(const Path & path, Sink & sink, bool memory_map = true); * Write a string to a file. */ void writeFile(const Path & path, std::string_view s, mode_t mode = 0666); + static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666) { return writeFile(path.string(), s, mode); } void writeFile(const Path & path, Source & source, mode_t mode = 0666, bool sync = false); + static inline void writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 0666, bool sync = false) { return writeFile(path.string(), source, mode, sync); } -void writeFile(AutoCloseFD & fd, std::string_view s, mode_t mode = 0666 ); +void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode = 0666); /** * Write a string to a file and flush the file and its parents direcotry to disk. diff --git a/src/libutil/meson.build b/src/libutil/meson.build index 2203e2294..f5ad2b1f6 100644 --- a/src/libutil/meson.build +++ b/src/libutil/meson.build @@ -37,8 +37,6 @@ foreach funcspec : check_funcs configdata.set(define_name, define_value, description: funcspec[1]) endforeach -configdata.set('HAVE_F_GETPATH', cxx.has_header_symbol('fcntl.h', 'F_GETPATH').to_int()) - subdir('nix-meson-build-support/libatomic') if host_machine.system() == 'windows' From a4b5584fb10e967f753a54b8c02cc5229b4cab8e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Jun 2025 12:14:04 +0200 Subject: [PATCH 10/15] Replace 'bool sync' with an enum for clarity And drop writeFileAndSync(). --- src/libstore/local-store.cc | 4 +-- src/libutil/file-content-address.cc | 2 +- src/libutil/file-system.cc | 32 +++++++-------------- src/libutil/include/nix/util/file-system.hh | 19 ++++++------ 4 files changed, 21 insertions(+), 36 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 00c1ac6dd..9e1324262 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -247,7 +247,7 @@ LocalStore::LocalStore(ref config) else if (curSchema == 0) { /* new store */ curSchema = nixSchemaVersion; openDB(*state, true); - writeFileAndSync(schemaPath, fmt("%1%", curSchema), 0666); + writeFile(schemaPath, fmt("%1%", curSchema), 0666, FsSync::Yes); } else if (curSchema < nixSchemaVersion) { @@ -298,7 +298,7 @@ LocalStore::LocalStore(ref config) txn.commit(); } - writeFileAndSync(schemaPath, fmt("%1%", nixSchemaVersion), 0666); + writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, FsSync::Yes); lockFile(globalLock.get(), ltRead, true); } diff --git a/src/libutil/file-content-address.cc b/src/libutil/file-content-address.cc index 142bc70d5..d95781691 100644 --- a/src/libutil/file-content-address.cc +++ b/src/libutil/file-content-address.cc @@ -93,7 +93,7 @@ void restorePath( { switch (method) { case FileSerialisationMethod::Flat: - writeFile(path, source, 0666, startFsync); + writeFile(path, source, 0666, startFsync ? FsSync::Yes : FsSync::No); break; case FileSerialisationMethod::NixArchive: restorePath(path, source, startFsync); diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 3fd5ae669..79e6cf354 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -304,7 +304,7 @@ void readFile(const Path & path, Sink & sink, bool memory_map) } -void writeFile(const Path & path, std::string_view s, mode_t mode) +void writeFile(const Path & path, std::string_view s, mode_t mode, FsSync sync) { AutoCloseFD fd = toDescriptor(open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT // TODO @@ -315,40 +315,28 @@ void writeFile(const Path & path, std::string_view s, mode_t mode) if (!fd) throw SysError("opening file '%1%'", path); - writeFile(fd, path, s, mode); + writeFile(fd, path, s, mode, sync); /* Close explicitly to propagate the exceptions. */ fd.close(); } -void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode) +void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode, FsSync sync) { assert(fd); try { writeFull(fd.get(), s); + + if (sync == FsSync::Yes) + fd.fsync(); + } catch (Error & e) { e.addTrace({}, "writing file '%1%'", origPath); throw; } } -void writeFileAndSync(const Path & path, std::string_view s, mode_t mode) -{ - { - AutoCloseFD fd{open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode)}; - if (!fd) - throw SysError("opening file '%1%'", path); - - writeFile(fd, path, s, mode); - fd.fsync(); - /* Close explicitly to ensure that exceptions are propagated. */ - fd.close(); - } - - syncParent(path); -} - -void writeFile(const Path & path, Source & source, mode_t mode, bool sync) +void writeFile(const Path & path, Source & source, mode_t mode, FsSync sync) { AutoCloseFD fd = toDescriptor(open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT // TODO @@ -372,11 +360,11 @@ void writeFile(const Path & path, Source & source, mode_t mode, bool sync) e.addTrace({}, "writing file '%1%'", path); throw; } - if (sync) + if (sync == FsSync::Yes) fd.fsync(); // Explicitly close to make sure exceptions are propagated. fd.close(); - if (sync) + if (sync == FsSync::Yes) syncParent(path); } diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index 5f062412d..c45cb55aa 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -169,29 +169,26 @@ std::string readFile(const Path & path); std::string readFile(const std::filesystem::path & path); void readFile(const Path & path, Sink & sink, bool memory_map = true); +enum struct FsSync { Yes, No }; + /** * Write a string to a file. */ -void writeFile(const Path & path, std::string_view s, mode_t mode = 0666); +void writeFile(const Path & path, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No); -static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666) +static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No) { - return writeFile(path.string(), s, mode); + return writeFile(path.string(), s, mode, sync); } -void writeFile(const Path & path, Source & source, mode_t mode = 0666, bool sync = false); +void writeFile(const Path & path, Source & source, mode_t mode = 0666, FsSync sync = FsSync::No); -static inline void writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 0666, bool sync = false) +static inline void writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 0666, FsSync sync = FsSync::No) { return writeFile(path.string(), source, mode, sync); } -void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode = 0666); - -/** - * Write a string to a file and flush the file and its parents direcotry to disk. - */ -void writeFileAndSync(const Path & path, std::string_view s, mode_t mode = 0666); +void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No); /** * Flush a path's parent directory to disk. From 9af4c267c6cf89ccd27c4f11a32b15533d20a20e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Jun 2025 12:30:32 +0200 Subject: [PATCH 11/15] Chown structured attr files safely --- src/libstore/unix/build/derivation-builder.cc | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 36405a8a2..e2a148aeb 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -316,6 +316,13 @@ protected: */ void chownToBuilder(int fd, const Path & path); + /** + * Create a file in `tmpDir` owned by the builder. + */ + void writeBuilderFile( + const std::string & name, + std::string_view contents); + /** * Run the builder's process. */ @@ -1069,16 +1076,10 @@ void DerivationBuilderImpl::initEnv() } else { auto hash = hashString(HashAlgorithm::SHA256, i.first); std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false); - Path p = tmpDir + "/" + fn; - AutoCloseFD passAsFileFd{openat(tmpDirFd.get(), fn.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666)}; - if (!passAsFileFd) - throw SysError("opening `passAsFile` file in the sandbox '%1%'", p); - writeFile(passAsFileFd, p, rewriteStrings(i.second, inputRewrites)); - chownToBuilder(passAsFileFd.get(), p); + writeBuilderFile(fn, rewriteStrings(i.second, inputRewrites)); env[i.first + "Path"] = tmpDirInSandbox() + "/" + fn; } } - } /* For convenience, set an environment pointing to the top build @@ -1153,11 +1154,9 @@ void DerivationBuilderImpl::writeStructuredAttrs() auto jsonSh = StructuredAttrs::writeShell(json); - writeFile(tmpDir + "/.attrs.sh", rewriteStrings(jsonSh, inputRewrites)); - chownToBuilder(tmpDir + "/.attrs.sh"); + writeBuilderFile(".attrs.sh", rewriteStrings(jsonSh, inputRewrites)); env["NIX_ATTRS_SH_FILE"] = tmpDirInSandbox() + "/.attrs.sh"; - writeFile(tmpDir + "/.attrs.json", rewriteStrings(json.dump(), inputRewrites)); - chownToBuilder(tmpDir + "/.attrs.json"); + writeBuilderFile(".attrs.json", rewriteStrings(json.dump(), inputRewrites)); env["NIX_ATTRS_JSON_FILE"] = tmpDirInSandbox() + "/.attrs.json"; } } @@ -1285,6 +1284,18 @@ void DerivationBuilderImpl::chownToBuilder(int fd, const Path & path) throw SysError("cannot change ownership of file '%1%'", path); } +void DerivationBuilderImpl::writeBuilderFile( + const std::string & name, + std::string_view contents) +{ + auto path = std::filesystem::path(tmpDir) / name; + AutoCloseFD fd{openat(tmpDirFd.get(), name.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666)}; + if (!fd) + throw SysError("creating file %s", path); + writeFile(fd, path, contents); + chownToBuilder(fd.get(), path); +} + void DerivationBuilderImpl::runChild() { /* Warning: in the child we should absolutely not make any SQLite From 88b7db1ba455926868dd61f5ea39e454e5fb0433 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 30 Mar 2025 16:45:34 +0200 Subject: [PATCH 12/15] libstore: Don't default build-dir to temp-dir, store setting If a build directory is accessible to other users it is possible to smuggle data in and out of build directories. Usually this is only a build purity problem, but in combination with other issues it can be used to break out of a build sandbox. to prevent this we default to using a subdirectory of nixStateDir (which is more restrictive). (cherry picked from pennae Lix commit 55b416f6897fb0d8a9315a530a9b7f0914458ded) (store setting done by roberth) --- doc/manual/rl-next/build-dir-mandatory.md | 9 +++++ misc/systemd/nix-daemon.conf.in | 3 +- src/libstore/globals.cc | 1 + src/libstore/include/nix/store/globals.hh | 9 +---- src/libstore/include/nix/store/local-store.hh | 33 ++++++++++++++++++- src/libstore/local-store.cc | 12 +++++++ src/libstore/unix/build/derivation-builder.cc | 6 +++- .../build-remote-trustless-should-fail-0.sh | 1 - tests/functional/build-remote-trustless.sh | 1 - tests/functional/build-remote.sh | 1 - tests/functional/supplementary-groups.sh | 1 - 11 files changed, 62 insertions(+), 15 deletions(-) create mode 100644 doc/manual/rl-next/build-dir-mandatory.md diff --git a/doc/manual/rl-next/build-dir-mandatory.md b/doc/manual/rl-next/build-dir-mandatory.md new file mode 100644 index 000000000..cb45a4315 --- /dev/null +++ b/doc/manual/rl-next/build-dir-mandatory.md @@ -0,0 +1,9 @@ +--- +synopsis: "`build-dir` no longer defaults to `$TMPDIR`" +--- + +The directory in which temporary build directories are created no longer defaults +to `TMPDIR` or `/tmp`, to avoid builders making their directories +world-accessible. This behavior allowed escaping the build sandbox and can +cause build impurities even when not used maliciously. We now default to `builds` +in `NIX_STATE_DIR` (which is `/nix/var/nix/builds` in the default configuration). diff --git a/misc/systemd/nix-daemon.conf.in b/misc/systemd/nix-daemon.conf.in index e7b264234..a0ddc4019 100644 --- a/misc/systemd/nix-daemon.conf.in +++ b/misc/systemd/nix-daemon.conf.in @@ -1 +1,2 @@ -d @localstatedir@/nix/daemon-socket 0755 root root - - +d @localstatedir@/nix/daemon-socket 0755 root root - - +d @localstatedir@/nix/builds 0755 root root 7d - diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index de5128347..721491def 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -6,6 +6,7 @@ #include "nix/util/abstract-setting-to-json.hh" #include "nix/util/compute-levels.hh" #include "nix/util/signals.hh" +#include "nix/util/types.hh" #include #include diff --git a/src/libstore/include/nix/store/globals.hh b/src/libstore/include/nix/store/globals.hh index b5157b4f4..0ac689b55 100644 --- a/src/libstore/include/nix/store/globals.hh +++ b/src/libstore/include/nix/store/globals.hh @@ -697,14 +697,7 @@ public: Setting> buildDir{this, std::nullopt, "build-dir", R"( - The directory on the host, in which derivations' temporary build directories are created. - - If not set, Nix uses the system temporary directory indicated by the `TMPDIR` environment variable. - Note that builds are often performed by the Nix daemon, so its `TMPDIR` is used, and not that of the Nix command line interface. - - This is also the location where [`--keep-failed`](@docroot@/command-ref/opt-common.md#opt-keep-failed) leaves its files. - - If Nix runs without sandbox, or if the platform does not support sandboxing with bind mounts (e.g. macOS), then the [`builder`](@docroot@/language/derivations.md#attr-builder)'s environment contains this directory instead of the virtual location [`sandbox-build-dir`](#conf-sandbox-build-dir). + Override the `build-dir` store setting for all stores that have this setting. )"}; Setting allowedImpureHostPrefixes{this, {}, "allowed-impure-host-deps", diff --git a/src/libstore/include/nix/store/local-store.hh b/src/libstore/include/nix/store/local-store.hh index 9a118fcc5..e52d51f75 100644 --- a/src/libstore/include/nix/store/local-store.hh +++ b/src/libstore/include/nix/store/local-store.hh @@ -34,7 +34,38 @@ struct OptimiseStats uint64_t bytesFreed = 0; }; -struct LocalStoreConfig : std::enable_shared_from_this, virtual LocalFSStoreConfig +struct LocalBuildStoreConfig : virtual LocalFSStoreConfig { + +private: + /** + Input for computing the build directory. See `getBuildDir()`. + */ + Setting> buildDir{this, std::nullopt, "build-dir", + R"( + The directory on the host, in which derivations' temporary build directories are created. + + If not set, Nix will use the `builds` subdirectory of its configured state directory. + + Note that builds are often performed by the Nix daemon, so its `build-dir` applies. + + Nix will create this directory automatically with suitable permissions if it does not exist. + Otherwise its permissions must allow all users to traverse the directory (i.e. it must have `o+x` set, in unix parlance) for non-sandboxed builds to work correctly. + + This is also the location where [`--keep-failed`](@docroot@/command-ref/opt-common.md#opt-keep-failed) leaves its files. + + If Nix runs without sandbox, or if the platform does not support sandboxing with bind mounts (e.g. macOS), then the [`builder`](@docroot@/language/derivations.md#attr-builder)'s environment will contain this directory, instead of the virtual location [`sandbox-build-dir`](#conf-sandbox-build-dir). + + > **Warning** + > + > `build-dir` must not be set to a world-writable directory. + > Placing temporary build directories in a world-writable place allows other users to access or modify build data that is currently in use. + > This alone is merely an impurity, but combined with another factor this has allowed malicious derivations to escape the build sandbox. + )"}; +public: + Path getBuildDir() const; +}; + +struct LocalStoreConfig : std::enable_shared_from_this, virtual LocalFSStoreConfig, virtual LocalBuildStoreConfig { using LocalFSStoreConfig::LocalFSStoreConfig; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 9e1324262..e25a802ec 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -77,6 +77,18 @@ std::string LocalStoreConfig::doc() ; } +Path LocalBuildStoreConfig::getBuildDir() const +{ + if (settings.buildDir.get().has_value()) { + return *settings.buildDir.get(); + } + if (buildDir.get().has_value()) { + return *buildDir.get(); + } + + return stateDir.get() + "/builds"; +} + ref LocalStore::Config::openStore() const { return make_ref(ref{shared_from_this()}); diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index e2a148aeb..a036c95f1 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -725,9 +725,13 @@ void DerivationBuilderImpl::startBuilder() throw BuildError(msg); } + auto buildDir = getLocalStore(store).config->getBuildDir(); + + createDirs(buildDir); + /* Create a temporary directory where the build will take place. */ - topTmpDir = createTempDir(settings.buildDir.get().value_or(""), "nix-build-" + std::string(drvPath.name()), 0700); + topTmpDir = createTempDir(buildDir, "nix-build-" + std::string(drvPath.name()), 0700); setBuildTmpDir(); assert(!tmpDir.empty()); diff --git a/tests/functional/build-remote-trustless-should-fail-0.sh b/tests/functional/build-remote-trustless-should-fail-0.sh index 3401de1b0..e79527d72 100755 --- a/tests/functional/build-remote-trustless-should-fail-0.sh +++ b/tests/functional/build-remote-trustless-should-fail-0.sh @@ -12,7 +12,6 @@ requiresUnprivilegedUserNamespaces [[ $busybox =~ busybox ]] || skipTest "no busybox" unset NIX_STORE_DIR -unset NIX_STATE_DIR # We first build a dependency of the derivation we eventually want to # build. diff --git a/tests/functional/build-remote-trustless.sh b/tests/functional/build-remote-trustless.sh index 9f91a91a9..6014b57bb 100644 --- a/tests/functional/build-remote-trustless.sh +++ b/tests/functional/build-remote-trustless.sh @@ -9,7 +9,6 @@ requiresUnprivilegedUserNamespaces [[ "$busybox" =~ busybox ]] || skipTest "no busybox" unset NIX_STORE_DIR -unset NIX_STATE_DIR remoteDir=$TEST_ROOT/remote diff --git a/tests/functional/build-remote.sh b/tests/functional/build-remote.sh index 765cd71b4..f396bc72e 100644 --- a/tests/functional/build-remote.sh +++ b/tests/functional/build-remote.sh @@ -8,7 +8,6 @@ requiresUnprivilegedUserNamespaces # Avoid store dir being inside sandbox build-dir unset NIX_STORE_DIR -unset NIX_STATE_DIR function join_by { local d=$1; shift; echo -n "$1"; shift; printf "%s" "${@/#/$d}"; } diff --git a/tests/functional/supplementary-groups.sh b/tests/functional/supplementary-groups.sh index 400333f7d..a667d3e99 100755 --- a/tests/functional/supplementary-groups.sh +++ b/tests/functional/supplementary-groups.sh @@ -14,7 +14,6 @@ execUnshare < Date: Thu, 12 Jun 2025 11:04:07 +0200 Subject: [PATCH 13/15] Disallow the build directory having world-writable parents --- src/libstore/unix/build/derivation-builder.cc | 15 +++++++++++++++ tests/nixos/chroot-store.nix | 4 ++++ 2 files changed, 19 insertions(+) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index a036c95f1..b089a0169 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -698,6 +698,18 @@ static void handleChildException(bool sendException) } } +static bool checkNotWorldWritable(std::filesystem::path path) +{ + while (true) { + auto st = lstat(path); + if (st.st_mode & S_IWOTH) + return false; + if (path == path.parent_path()) break; + path = path.parent_path(); + } + return true; +} + void DerivationBuilderImpl::startBuilder() { /* Make sure that no other processes are executing under the @@ -729,6 +741,9 @@ void DerivationBuilderImpl::startBuilder() createDirs(buildDir); + if (buildUser && !checkNotWorldWritable(buildDir)) + throw Error("Path %s or a parent directory is world-writable or a symlink. That's not allowed for security.", buildDir); + /* Create a temporary directory where the build will take place. */ topTmpDir = createTempDir(buildDir, "nix-build-" + std::string(drvPath.name()), 0700); diff --git a/tests/nixos/chroot-store.nix b/tests/nixos/chroot-store.nix index f89a20bc4..0a4fff992 100644 --- a/tests/nixos/chroot-store.nix +++ b/tests/nixos/chroot-store.nix @@ -41,5 +41,9 @@ in # Test that /nix/store is available via an overlayfs mount. machine.succeed("nix shell --store /tmp/nix ${pkgA} --command cowsay foo >&2") + + # Building in /tmp should fail for security reasons. + err = machine.fail("nix build --offline --store /tmp/nix --expr 'builtins.derivation { name = \"foo\"; system = \"x86_64-linux\"; builder = \"/foo\"; }' 2>&1") + assert "is world-writable" in err ''; } From 2e2fe4cb07c81f4af4798d1f67e0f6b8d6263e44 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Jun 2025 11:12:23 +0200 Subject: [PATCH 14/15] Cleanup --- src/libstore/globals.cc | 1 - src/libstore/include/nix/store/local-store.hh | 3 ++- src/libstore/local-store.cc | 14 ++++++-------- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 721491def..de5128347 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -6,7 +6,6 @@ #include "nix/util/abstract-setting-to-json.hh" #include "nix/util/compute-levels.hh" #include "nix/util/signals.hh" -#include "nix/util/types.hh" #include #include diff --git a/src/libstore/include/nix/store/local-store.hh b/src/libstore/include/nix/store/local-store.hh index e52d51f75..fd7e6fc36 100644 --- a/src/libstore/include/nix/store/local-store.hh +++ b/src/libstore/include/nix/store/local-store.hh @@ -34,7 +34,8 @@ struct OptimiseStats uint64_t bytesFreed = 0; }; -struct LocalBuildStoreConfig : virtual LocalFSStoreConfig { +struct LocalBuildStoreConfig : virtual LocalFSStoreConfig +{ private: /** diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e25a802ec..0d2d96e61 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -79,14 +79,12 @@ std::string LocalStoreConfig::doc() Path LocalBuildStoreConfig::getBuildDir() const { - if (settings.buildDir.get().has_value()) { - return *settings.buildDir.get(); - } - if (buildDir.get().has_value()) { - return *buildDir.get(); - } - - return stateDir.get() + "/builds"; + return + settings.buildDir.get().has_value() + ? *settings.buildDir.get() + : buildDir.get().has_value() + ? *buildDir.get() + : stateDir.get() + "/builds"; } ref LocalStore::Config::openStore() const From 37685b1c9c936da8644822d6ae60242b8ffcee8e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 19 Jun 2025 13:26:10 +0200 Subject: [PATCH 15/15] Fix Darwin test failure in repl.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes error: … while processing sandbox path '/private/tmp/nix-shell.0MDgyx/nix-test/ca/repl/store/nix/var/nix/builds/nix-build-simple.drv-65916-3910734210' (/private/tmp/nix-shell.0MDgyx/nix-test/ca/repl/store) error: 'nix' is too short to be a valid store path which happened because we were now putting the build directory underneath the store directory. --- src/libstore/unix/build/derivation-builder.cc | 2 +- tests/functional/repl.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index b089a0169..fd62aa664 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -922,7 +922,7 @@ DerivationBuilderImpl::PathsInChroot DerivationBuilderImpl::getPathsInSandbox() store.computeFSClosure(store.toStorePath(i.second.source).first, closure); } catch (InvalidPath & e) { } catch (Error & e) { - e.addTrace({}, "while processing 'sandbox-paths'"); + e.addTrace({}, "while processing sandbox path '%s'", i.second.source); throw; } for (auto & i : closure) { diff --git a/tests/functional/repl.sh b/tests/functional/repl.sh index 6db9e2d3d..bfe18c9e5 100755 --- a/tests/functional/repl.sh +++ b/tests/functional/repl.sh @@ -67,7 +67,7 @@ testRepl () { # Simple test, try building a drv testRepl # Same thing (kind-of), but with a remote store. -testRepl --store "$TEST_ROOT/store?real=$NIX_STORE_DIR" +testRepl --store "$TEST_ROOT/other-root?real=$NIX_STORE_DIR" # Remove ANSI escape sequences. They can prevent grep from finding a match. stripColors () {