From c7745b2718f59d9cdb933672d96185d3e1512212 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Sun, 29 Jun 2025 09:03:13 +0000 Subject: [PATCH 1/9] 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/util.cc | 22 ++++++++++++++++++++++ src/libutil/util.hh | 15 +++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index f4b66e174ae..44cd22035fb 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -747,6 +747,28 @@ int AutoCloseFD::get() const return fd; } +std::string guessOrInventPathFromFD(int fd) +{ + assert(fd >= 0); + /* On Linux, there's no F_GETPATH available. + * But we can read /proc/ */ +#if __linux__ + try { + return readLink(fmt("/proc/self/fd/%1%", fd).c_str()); + } 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/util.hh b/src/libutil/util.hh index 7fd08ab09e2..78df3f8a922 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -186,6 +186,13 @@ public: operator Path() const { return path; } }; +/* + * 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(int fd); class AutoCloseFD { @@ -202,6 +209,14 @@ public: explicit operator bool() const; int release(); void close(); + + /* + * 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); } }; From 49333a0c506ba16e174e2adc29b474786b4cb833 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Sun, 29 Jun 2025 09:08:13 +0000 Subject: [PATCH 2/9] 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/build.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 371ec2383a9..996f91348f3 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -819,6 +819,9 @@ class DerivationGoal : public Goal /* The temporary directory. */ Path tmpDir; + /* The temporary directory file descriptor */ + AutoCloseFD tmpDirFd; + /* The path of the temporary directory in the sandbox. */ Path tmpDirInSandbox; @@ -1980,6 +1983,12 @@ void DerivationGoal::startBuilder() auto drvName = storePathToName(drvPath); tmpDir = createTempDir("", "nix-build-" + drvName, 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)}; + if (!tmpDirFd) + throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir); + chownToBuilder(tmpDir); /* Substitute output placeholders with the actual output paths. */ From e47a47293c0353fcad2ea2e8c0563974de5dd01a Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Sun, 29 Jun 2025 09:13:02 +0000 Subject: [PATCH 3/9] 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/build.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 996f91348f3..71d6c2c0622 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -988,9 +988,15 @@ class DerivationGoal : public Goal /* Write a JSON file containing the derivation attributes. */ void writeStructuredAttrs(); - /* Make a file owned by the builder. */ + /* Make a file owned by the builder addressed by its path. + * + * 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. */ void runChild(); @@ -1989,7 +1995,7 @@ void DerivationGoal::startBuilder() if (!tmpDirFd) throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir); - chownToBuilder(tmpDir); + chownToBuilder(tmpDirFd); /* Substitute output placeholders with the actual output paths. */ for (auto & output : drv->outputs) @@ -2701,6 +2707,12 @@ void DerivationGoal::chownToBuilder(const Path & path) throw SysError(format("cannot change ownership of '%1%'") % path); } +void DerivationGoal::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 setupSeccomp() { From 825167227a9951e9a7fde2118b08e6ef3f32c649 Mon Sep 17 00:00:00 2001 From: squalus Date: Sun, 29 Jun 2025 09:30:32 +0000 Subject: [PATCH 4/9] Improve durability of schema version file writes - call close explicitly in writeFile to prevent the close exception from being ignored - fsync after writing schema file to flush data to disk - fsync schema file parent to flush metadata to disk https://github.com/NixOS/nix/issues/7064 --- src/libstore/local-store.cc | 4 ++-- src/libutil/util.cc | 37 +++++++++++++++++++++++++++++++++++-- src/libutil/util.hh | 8 ++++++-- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 31a6cc46f36..d495d5b31a8 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -159,7 +159,7 @@ LocalStore::LocalStore(const Params & params) else if (curSchema == 0) { /* new store */ curSchema = nixSchemaVersion; openDB(*state, true); - writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str()); + writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666, true); } else if (curSchema < nixSchemaVersion) { @@ -207,7 +207,7 @@ LocalStore::LocalStore(const Params & params) txn.commit(); } - writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str()); + writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666, true); lockFile(globalLock.get(), ltRead, true); } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 44cd22035fb..4ff5890bfb6 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -333,16 +333,22 @@ void readFile(const Path & path, Sink & sink) } -void writeFile(const Path & path, const string & s, mode_t mode) +void writeFile(const Path & path, const string & s, mode_t mode, bool sync) { AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode); if (!fd) throw SysError(format("opening file '%1%'") % path); writeFull(fd.get(), s); + if (sync) + fd.fsync(); + // Explicitly close to make sure exceptions are propagated. + fd.close(); + if (sync) + syncParent(path); } -void writeFile(const Path & path, Source & source, mode_t mode) +void writeFile(const Path & path, Source & source, mode_t mode, bool sync) { AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode); if (!fd) @@ -356,8 +362,22 @@ void writeFile(const Path & path, Source & source, mode_t mode) writeFull(fd.get(), (unsigned char *) buf.data(), n); } catch (EndOfFile &) { break; } } + + if (sync) + fd.fsync(); + // Explicitly close to make sure exceptions are propagated. + fd.close(); + if (sync) + syncParent(path); } +void syncParent(const Path & path) +{ + AutoCloseFD fd = open(dirOf(path).c_str(), O_RDONLY, 0); + if (!fd) + throw SysError("opening file '%1%'", path); + fd.fsync(); +} string readLine(int fd) { @@ -780,6 +800,19 @@ void AutoCloseFD::close() } } +void AutoCloseFD::fsync() +{ + if (fd != -1) { + int result; +#if __APPLE__ + result = ::fcntl(fd, F_FULLFSYNC); +#else + result = ::fsync(fd); +#endif + if (result == -1) + throw SysError("fsync file descriptor %1%", fd); + } +} AutoCloseFD::operator bool() const { diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 78df3f8a922..1da6de56121 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -105,9 +105,12 @@ string readFile(const Path & path, bool drain = false); void readFile(const Path & path, Sink & sink); /* Write a string to a file. */ -void writeFile(const Path & path, const string & s, mode_t mode = 0666); +void writeFile(const Path & path, const string & s, mode_t mode = 0666, bool sync = false); -void writeFile(const Path & path, Source & source, mode_t mode = 0666); +void writeFile(const Path & path, Source & source, mode_t mode = 0666, bool sync = false); + +/* Flush a file's parent directory to disk */ +void syncParent(const Path & path); /* Read a line from a file descriptor. */ string readLine(int fd); @@ -209,6 +212,7 @@ public: explicit operator bool() const; int release(); void close(); + void fsync(); /* * Will attempt to guess *A* path associated that might lead to the same file as used by this From 305d2e5a58cd1b10ecd4386a99c7bd4f907da7c6 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Sun, 29 Jun 2025 09:42:26 +0000 Subject: [PATCH 5/9] 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/util.cc | 42 +++++++++++++++++++++++++++++-------- src/libutil/util.hh | 10 +++++++-- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index d495d5b31a8..72b6ca38fee 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -159,7 +159,7 @@ LocalStore::LocalStore(const Params & params) else if (curSchema == 0) { /* new store */ curSchema = nixSchemaVersion; openDB(*state, true); - writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666, true); + writeFileAndSync(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666); } else if (curSchema < nixSchemaVersion) { @@ -207,7 +207,7 @@ LocalStore::LocalStore(const Params & params) txn.commit(); } - writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666, true); + writeFileAndSync(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666); lockFile(globalLock.get(), ltRead, true); } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 4ff5890bfb6..255f994063a 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -333,12 +333,42 @@ void readFile(const Path & path, Sink & sink) } -void writeFile(const Path & path, const string & s, mode_t mode, bool sync) +void writeFile(const Path & path, const string & s, mode_t mode) { AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode); if (!fd) throw SysError(format("opening file '%1%'") % path); + + writeFile(fd, s, mode); + + /* Close explicitly to propagate the exceptions. */ + fd.close(); +} + +void writeFile(AutoCloseFD & fd, const std::string& s, mode_t mode) +{ + assert(fd); writeFull(fd.get(), s); +} + +void writeFileAndSync(const Path & path, const std::string& 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); +} + +static void closeForWrite(const Path & path, AutoCloseFD & fd, bool sync) +{ if (sync) fd.fsync(); // Explicitly close to make sure exceptions are propagated. @@ -347,8 +377,7 @@ void writeFile(const Path & path, const string & s, mode_t mode, bool sync) syncParent(path); } - -void writeFile(const Path & path, Source & source, mode_t mode, bool sync) +void writeFile(const Path & path, Source & source, mode_t mode) { AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode); if (!fd) @@ -363,12 +392,7 @@ void writeFile(const Path & path, Source & source, mode_t mode, bool sync) } catch (EndOfFile &) { break; } } - if (sync) - fd.fsync(); - // Explicitly close to make sure exceptions are propagated. - fd.close(); - if (sync) - syncParent(path); + closeForWrite(path, fd, false); } void syncParent(const Path & path) diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 1da6de56121..b76fdf38927 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -105,9 +105,15 @@ string readFile(const Path & path, bool drain = false); void readFile(const Path & path, Sink & sink); /* Write a string to a file. */ -void writeFile(const Path & path, const string & s, mode_t mode = 0666, bool sync = false); +void writeFile(const Path & path, const string & s, mode_t mode = 0666); -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); + +class AutoCloseFD; // forward-declaration needed because this moved around in Lix +void writeFile(AutoCloseFD & fd, const std::string& s, mode_t mode = 0666); + +/* Write a string to a file and flush the file and its parent directory to disk. */ +void writeFileAndSync(const Path & path, const std::string& s, mode_t mode = 0666); /* Flush a file's parent directory to disk */ void syncParent(const Path & path); From 70f5c25b10ce8c39eb34c7ff688f25545660bea7 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Sun, 29 Jun 2025 09:53:17 +0000 Subject: [PATCH 6/9] libstore: ensure that `passAsFile` is created in the original tmpdir This ensures that `passAsFile` data is created inside the expected temporary build directory by `openat()` from the parent directory file descriptor. Fixes CVE-2025-52993. Change-Id: Ie5273446c4a19403088d0389ae8e3f473af8879a Signed-off-by: Raito Bezarius --- src/libstore/build.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 71d6c2c0622..db9d7219f39 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2511,8 +2511,15 @@ void DerivationGoal::initTmpDir() { } else { string fn = ".attr-" + std::to_string(fileNr++); 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 cb34f224af19c98747f5930056376f775496860e Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Mon, 27 Apr 2020 14:15:15 +0000 Subject: [PATCH 7/9] Fix long paths permanently breaking GC Suppose I have a path /nix/store/[hash]-[name]/a/a/a/a/a/[...]/a, long enough that everything after "/nix/store/" is longer than 4096 (MAX_PATH) bytes. Nix will happily allow such a path to be inserted into the store, because it doesn't look at all the nested structure. It just cares about the /nix/store/[hash]-[name] part. But, when the path is deleted, we encounter a problem. Nix will move the path to /nix/store/trash, but then when it's trying to recursively delete the trash directory, it will at some point try to unlink /nix/store/trash/[hash]-[name]/a/a/a/a/a/[...]/a. This will fail, because the path is too long. After this has failed, any store deletion operation will never work again, because Nix needs to delete the trash directory before recreating it to move new things to it. (I assume this is because otherwise a path being deleted could already exist in the trash, and then moving it would fail.) This means that if I can trick somebody into just fetching a tarball containing a path of the right length, they won't be able to delete store paths or garbage collect ever again, until the offending path is manually removed from /nix/store/trash. (And even fixing this manually is quite difficult if you don't understand the issue, because the absolute path that Nix says it failed to remove is also too long for rm(1).) This patch fixes the issue by making Nix's recursive delete operation use unlinkat(2). This function takes a relative path and a directory file descriptor. We ensure that the relative path is always just the name of the directory entry, and therefore its length will never exceed 255 bytes. This means that it will never even come close to AX_PATH, and Nix will therefore be able to handle removing arbitrarily deep directory hierachies. Since the directory file descriptor is used for recursion after being used in readDirectory, I made a variant of readDirectory that takes an already open directory stream, to avoid the directory being opened multiple times. As we have seen from this issue, the less we have to interact with paths, the better, and so it's good to reuse file descriptors where possible. I left _deletePath as succeeding even if the parent directory doesn't exist, even though that feels wrong to me, because without that early return, the linux-sandbox test failed. Reported-by: Alyssa Ross Thanks-to: Puck Meerburg Tested-by: Puck Meerburg Reviewed-by: Puck Meerburg --- src/libutil/util.cc | 54 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 255f994063a..7be0e637edc 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -266,16 +266,13 @@ bool isLink(const Path & path) } -DirEntries readDirectory(const Path & path) +DirEntries readDirectory(DIR *dir, const Path & path) { DirEntries entries; entries.reserve(64); - AutoCloseDir dir(opendir(path.c_str())); - if (!dir) throw SysError(format("opening directory '%1%'") % path); - struct dirent * dirent; - while (errno = 0, dirent = readdir(dir.get())) { /* sic */ + while (errno = 0, dirent = readdir(dir)) { /* sic */ checkInterrupt(); string name = dirent->d_name; if (name == "." || name == "..") continue; @@ -292,6 +289,14 @@ DirEntries readDirectory(const Path & path) return entries; } +DirEntries readDirectory(const Path & path) +{ + AutoCloseDir dir(opendir(path.c_str())); + if (!dir) throw SysError(format("opening directory '%1%'") % path); + + return readDirectory(dir.get(), path); +} + unsigned char getFileType(const Path & path) { @@ -431,12 +436,14 @@ void writeLine(int fd, string s) } -static void _deletePath(const Path & path, unsigned long long & bytesFreed) +static void _deletePath(int parentfd, const Path & path, unsigned long long & bytesFreed) { checkInterrupt(); + string name(baseNameOf(path)); + struct stat st; - if (lstat(path.c_str(), &st) == -1) { + if (fstatat(parentfd, name.c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) { if (errno == ENOENT) return; throw SysError(format("getting status of '%1%'") % path); } @@ -448,20 +455,45 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed) /* Make the directory accessible. */ const auto PERM_MASK = S_IRUSR | S_IWUSR | S_IXUSR; if ((st.st_mode & PERM_MASK) != PERM_MASK) { - if (chmod(path.c_str(), st.st_mode | PERM_MASK) == -1) + if (fchmodat(parentfd, name.c_str(), st.st_mode | PERM_MASK, 0) == -1) throw SysError(format("chmod '%1%'") % path); } - for (auto & i : readDirectory(path)) - _deletePath(path + "/" + i.name, bytesFreed); + int fd = openat(parentfd, path.c_str(), O_RDONLY); + if (!fd) + throw SysError(format("opening directory '%1%'") % path); + AutoCloseDir dir(fdopendir(fd)); + if (!dir) + throw SysError(format("opening directory '%1%'") % path); + for (auto & i : readDirectory(dir.get(), path)) + _deletePath(dirfd(dir.get()), path + "/" + i.name, bytesFreed); } - if (remove(path.c_str()) == -1) { + int flags = S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0; + if (unlinkat(parentfd, name.c_str(), flags) == -1) { if (errno == ENOENT) return; throw SysError(format("cannot unlink '%1%'") % path); } } +static void _deletePath(const Path & path, unsigned long long & bytesFreed) +{ + Path dir = dirOf(path); + if (dir == "") + dir = "/"; + + AutoCloseFD dirfd(open(dir.c_str(), O_RDONLY)); + if (!dirfd) { + // This really shouldn't fail silently, but it's left this way + // for backwards compatibility. + if (errno == ENOENT) return; + + throw SysError(format("opening directory '%1%'") % path); + } + + _deletePath(dirfd.get(), path, bytesFreed); +} + void deletePath(const Path & path) { From 2e04bd114f808b561d39f61622ab769f97d5d8d1 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 29 Jun 2025 10:10:19 +0000 Subject: [PATCH 8/9] deletePath(): Return ENFILE instead of EBADF when out of file descriptors Also remove an erroneous comment. --- src/libutil/util.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 7be0e637edc..1b3bcd9c544 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -460,7 +460,7 @@ static void _deletePath(int parentfd, const Path & path, unsigned long long & by } int fd = openat(parentfd, path.c_str(), O_RDONLY); - if (!fd) + if (fd = -1) throw SysError(format("opening directory '%1%'") % path); AutoCloseDir dir(fdopendir(fd)); if (!dir) @@ -484,10 +484,7 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed) AutoCloseFD dirfd(open(dir.c_str(), O_RDONLY)); if (!dirfd) { - // This really shouldn't fail silently, but it's left this way - // for backwards compatibility. if (errno == ENOENT) return; - throw SysError(format("opening directory '%1%'") % path); } From 3f3c4e8fd77117050c8b2c695704e98c1825591f Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Sun, 29 Jun 2025 10:14:01 +0000 Subject: [PATCH 9/9] 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. Fixes CVE-2025-46415. Change-Id: I6b3fc766bad2afe54dc27d47d1df3873e188de96 Signed-off-by: Raito Bezarius --- src/libutil/util.cc | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 1b3bcd9c544..756bdf86710 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -436,16 +436,16 @@ void writeLine(int fd, string s) } -static void _deletePath(int parentfd, const Path & path, unsigned long long & bytesFreed) +static void _deletePath(int parentfd, const Path& name, unsigned long long & bytesFreed) { + /* This ensures that `name` is an immediate child of `parentfd`. */ + assert(!name.empty() && name.find('/') == std::string::npos && "`name` is an immediate child to `parentfd`"); checkInterrupt(); - string name(baseNameOf(path)); - struct stat st; if (fstatat(parentfd, name.c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) { if (errno == ENOENT) return; - throw SysError(format("getting status of '%1%'") % path); + throw SysError("getting status of '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); } if (!S_ISDIR(st.st_mode) && st.st_nlink == 1) @@ -455,24 +455,25 @@ static void _deletePath(int parentfd, const Path & path, unsigned long long & by /* 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(format("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(format("opening directory '%1%'") % path); + throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); AutoCloseDir dir(fdopendir(fd)); if (!dir) - throw SysError(format("opening directory '%1%'") % path); - for (auto & i : readDirectory(dir.get(), path)) - _deletePath(dirfd(dir.get()), path + "/" + i.name, bytesFreed); + throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); + for (auto & i : readDirectory(dir.get(), name)) + _deletePath(dirfd(dir.get()), i.name, bytesFreed); } int flags = S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0; if (unlinkat(parentfd, name.c_str(), flags) == -1) { if (errno == ENOENT) return; - throw SysError(format("cannot unlink '%1%'") % path); + throw SysError("cannot unlink '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); } } @@ -488,7 +489,7 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed) throw SysError(format("opening directory '%1%'") % path); } - _deletePath(dirfd.get(), path, bytesFreed); + _deletePath(dirfd.get(), baseNameOf(path).data(), bytesFreed); }