From 255704a98c5a0be592896f89beb63c70294f5609 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Wed, 26 Mar 2025 12:42:55 +0100 Subject: [PATCH] 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 | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 39ba7699e..c8a1cb8ad 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -391,16 +391,26 @@ void syncParent(const Path & path) fd.fsync(); } -static void _deletePath(int parentfd, const Path & path, uint64_t & bytesFreed) +/* TODO(horrors): a better structure that links all parent fds for the traversal root + * should be considered for this code + */ +static void _deletePath(int parentfd, const Path & name, uint64_t & 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(); - std::string name(baseNameOf(path)); + /* FIXME(horrors): there's a minor TOCTOU here. + * we fstatat the inode nofollow, check if this is a directory + * and then open it. + * a better alternative is open it as O_PATH as a namefd. + * if it's a directory, it can be openat with the namefd. + */ 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)) { @@ -431,24 +441,25 @@ static void _deletePath(int parentfd, const Path & path, uint64_t & bytesFreed) /* 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); - 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("cannot unlink '%1%'", path); + throw SysError("cannot unlink '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd)); } } @@ -464,7 +475,7 @@ static void _deletePath(const Path & path, uint64_t & bytesFreed) throw SysError("opening directory '%1%'", path); } - _deletePath(dirfd.get(), path, bytesFreed); + _deletePath(dirfd.get(), baseNameOf(path).data(), bytesFreed); }