From 8f4ffb3fae133bb21d7991e97c2f19a7108b1143 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Mon, 11 Mar 2024 10:59:42 +0100 Subject: daemon: Protect against FD escape when building fixed-output derivations (CVE-2024-27297). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a security issue (CVE-2024-27297) whereby a fixed-output derivation build process could open a writable file descriptor to its output, send it to some outside process for instance over an abstract AF_UNIX socket, which would then allow said process to modify the file in the store after it has been marked as “valid”. Vulnerability discovered by puck . Nix security advisory: https://github.com/NixOS/nix/security/advisories/GHSA-2ffj-w4mj-pg37 Nix fix: https://github.com/NixOS/nix/commit/244f3eee0bbc7f11e9b383a15ed7368e2c4becc9 * nix/libutil/util.cc (readDirectory): Add variants that take a DIR* and a file descriptor. Rewrite the ‘Path’ variant accordingly. (copyFile, copyFileRecursively): New functions. * nix/libutil/util.hh (copyFileRecursively): New declaration. * nix/libstore/build.cc (DerivationGoal::buildDone): When ‘fixedOutput’ is true, call ‘copyFileRecursively’ followed by ‘rename’ on each output. Change-Id: I7952d41093eed26e123e38c14a4c1424be1ce1c4 Reported-by: Picnoir , Théophane Hufschmitt Change-Id: Idb5f2757f35af86b032a9851cecb19b70227bd88 --- nix/libstore/build.cc | 16 ++++++++ nix/libutil/util.cc | 112 +++++++++++++++++++++++++++++++++++++++++++++++--- nix/libutil/util.hh | 6 +++ 3 files changed, 129 insertions(+), 5 deletions(-) (limited to 'nix') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index 461fcbc584..e2adee118b 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -1382,6 +1382,22 @@ void DerivationGoal::buildDone() % drvPath % statusToString(status)); } + if (fixedOutput) { + /* Replace the output, if it exists, by a fresh copy of itself to + make sure that there's no stale file descriptor pointing to it + (CVE-2024-27297). */ + foreach (DerivationOutputs::iterator, i, drv.outputs) { + if (pathExists(i->second.path)) { + Path pivot = i->second.path + ".tmp"; + copyFileRecursively(i->second.path, pivot, true); + int err = rename(pivot.c_str(), i->second.path.c_str()); + if (err != 0) + throw SysError(format("renaming `%1%' to `%2%'") + % pivot % i->second.path); + } + } + } + /* Compute the FS closure of the outputs and register them as being valid. */ registerOutputs(); diff --git a/nix/libutil/util.cc b/nix/libutil/util.cc index 82eac72120..493f06f357 100644 --- a/nix/libutil/util.cc +++ b/nix/libutil/util.cc @@ -215,14 +215,11 @@ bool isLink(const Path & path) } -DirEntries readDirectory(const Path & path) +static DirEntries readDirectory(DIR *dir) { 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)) { /* sic */ checkInterrupt(); @@ -230,11 +227,29 @@ DirEntries readDirectory(const Path & path) if (name == "." || name == "..") continue; entries.emplace_back(name, dirent->d_ino, dirent->d_type); } - if (errno) throw SysError(format("reading directory `%1%'") % path); + if (errno) throw SysError(format("reading directory")); 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); +} + +static DirEntries readDirectory(int fd) +{ + /* Since 'closedir' closes the underlying file descriptor, duplicate FD + beforehand. */ + int fdcopy = dup(fd); + if (fdcopy < 0) throw SysError("dup"); + + AutoCloseDir dir = fdopendir(fdcopy); + if (!dir) throw SysError(format("opening directory from file descriptor `%1%'") % fd); + return readDirectory(dir); +} unsigned char getFileType(const Path & path) { @@ -364,6 +379,93 @@ void deletePath(const Path & path, unsigned long long & bytesFreed, size_t linkT _deletePath(path, bytesFreed, linkThreshold); } +static void copyFile(int sourceFd, int destinationFd) +{ + struct stat st; + if (fstat(sourceFd, &st) == -1) throw SysError("statting file"); + + ssize_t result = copy_file_range(sourceFd, NULL, destinationFd, NULL, st.st_size, 0); + if (result < 0 && errno == ENOSYS) { + for (size_t remaining = st.st_size; remaining > 0; ) { + unsigned char buf[8192]; + size_t count = std::min(remaining, sizeof buf); + + readFull(sourceFd, buf, count); + writeFull(destinationFd, buf, count); + remaining -= count; + } + } else { + if (result < 0) + throw SysError(format("copy_file_range `%1%' to `%2%'") % sourceFd % destinationFd); + if (result < st.st_size) + throw SysError(format("short write in copy_file_range `%1%' to `%2%'") + % sourceFd % destinationFd); + } +} + +static void copyFileRecursively(int sourceroot, const Path &source, + int destinationroot, const Path &destination, + bool deleteSource) +{ + struct stat st; + if (fstatat(sourceroot, source.c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) + throw SysError(format("statting file `%1%'") % source); + + if (S_ISREG(st.st_mode)) { + AutoCloseFD sourceFd = openat(sourceroot, source.c_str(), + O_CLOEXEC | O_NOFOLLOW | O_RDONLY); + if (sourceFd == -1) throw SysError(format("opening `%1%'") % source); + + AutoCloseFD destinationFd = openat(destinationroot, destination.c_str(), + O_CLOEXEC | O_CREAT | O_WRONLY | O_TRUNC, + st.st_mode); + if (destinationFd == -1) throw SysError(format("opening `%1%'") % source); + + copyFile(sourceFd, destinationFd); + } else if (S_ISLNK(st.st_mode)) { + char target[st.st_size + 1]; + ssize_t result = readlinkat(sourceroot, source.c_str(), target, st.st_size); + if (result != st.st_size) throw SysError("reading symlink target"); + target[st.st_size] = '\0'; + int err = symlinkat(target, destinationroot, destination.c_str()); + if (err != 0) + throw SysError(format("creating symlink `%1%'") % destination); + } else if (S_ISDIR(st.st_mode)) { + int err = mkdirat(destinationroot, destination.c_str(), 0755); + if (err != 0) + throw SysError(format("creating directory `%1%'") % destination); + + AutoCloseFD destinationFd = openat(destinationroot, destination.c_str(), + O_CLOEXEC | O_RDONLY | O_DIRECTORY); + if (err != 0) + throw SysError(format("opening directory `%1%'") % destination); + + AutoCloseFD sourceFd = openat(sourceroot, source.c_str(), + O_CLOEXEC | O_NOFOLLOW | O_RDONLY); + if (sourceFd == -1) + throw SysError(format("opening `%1%'") % source); + + if (deleteSource && !(st.st_mode & S_IWUSR)) { + /* Ensure the directory writable so files within it can be + deleted. */ + if (fchmod(sourceFd, st.st_mode | S_IWUSR) == -1) + throw SysError(format("making `%1%' directory writable") % source); + } + + for (auto & i : readDirectory(sourceFd)) + copyFileRecursively((int)sourceFd, i.name, (int)destinationFd, i.name, + deleteSource); + } else throw Error(format("refusing to copy irregular file `%1%'") % source); + + if (deleteSource) + unlinkat(sourceroot, source.c_str(), + S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0); +} + +void copyFileRecursively(const Path &source, const Path &destination, bool deleteSource) +{ + copyFileRecursively(AT_FDCWD, source, AT_FDCWD, destination, deleteSource); +} static Path tempName(Path tmpRoot, const Path & prefix, bool includePid, int & counter) diff --git a/nix/libutil/util.hh b/nix/libutil/util.hh index 880b0e93b2..058f5f8446 100644 --- a/nix/libutil/util.hh +++ b/nix/libutil/util.hh @@ -102,6 +102,12 @@ void deletePath(const Path & path); void deletePath(const Path & path, unsigned long long & bytesFreed, size_t linkThreshold = 1); +/* Copy SOURCE to DESTINATION, recursively. Throw if SOURCE contains a file + that is not a regular file, symlink, or directory. When DELETESOURCE is + true, delete source files once they have been copied. */ +void copyFileRecursively(const Path &source, const Path &destination, + bool deleteSource = false); + /* Create a temporary directory. */ Path createTempDir(const Path & tmpRoot = "", const Path & prefix = "nix", bool includePid = true, bool useGlobalCounter = true, mode_t mode = 0755); -- cgit v1.2.3 From ff1251de0bc327ec478fc66a562430fbf35aef42 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Tue, 12 Mar 2024 11:53:35 +0100 Subject: daemon: Address shortcoming in previous security fix for CVE-2024-27297. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a followup to 8f4ffb3fae133bb21d7991e97c2f19a7108b1143. Commit 8f4ffb3fae133bb21d7991e97c2f19a7108b1143 fell short in two ways: (1) it didn’t have any effet for fixed-output derivations performed in a chroot, which is the case for all of them except those using “builtin:download” and “builtin:git-download”, and (2) it did not preserve ownership when copying, leading to “suspicious ownership or permission […] rejecting this build output” errors. * nix/libstore/build.cc (DerivationGoal::buildDone): Account for ‘chrootRootDir’ when copying ‘drv.outputs’. * nix/libutil/util.cc (copyFileRecursively): Add ‘fchown’ and ‘fchownat’ calls to preserve file ownership; this is necessary for chrooted fixed-output derivation builds. * nix/libutil/util.hh: Update comment. Change-Id: Ib59f040e98fed59d1af81d724b874b592cbef156 --- nix/libstore/build.cc | 11 ++++++----- nix/libutil/util.cc | 4 ++++ nix/libutil/util.hh | 7 ++++--- 3 files changed, 14 insertions(+), 8 deletions(-) (limited to 'nix') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index e2adee118b..d23c0944a4 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -1387,13 +1387,14 @@ void DerivationGoal::buildDone() make sure that there's no stale file descriptor pointing to it (CVE-2024-27297). */ foreach (DerivationOutputs::iterator, i, drv.outputs) { - if (pathExists(i->second.path)) { - Path pivot = i->second.path + ".tmp"; - copyFileRecursively(i->second.path, pivot, true); - int err = rename(pivot.c_str(), i->second.path.c_str()); + Path output = chrootRootDir + i->second.path; + if (pathExists(output)) { + Path pivot = output + ".tmp"; + copyFileRecursively(output, pivot, true); + int err = rename(pivot.c_str(), output.c_str()); if (err != 0) throw SysError(format("renaming `%1%' to `%2%'") - % pivot % i->second.path); + % pivot % output); } } } diff --git a/nix/libutil/util.cc b/nix/libutil/util.cc index 493f06f357..578d657293 100644 --- a/nix/libutil/util.cc +++ b/nix/libutil/util.cc @@ -422,6 +422,7 @@ static void copyFileRecursively(int sourceroot, const Path &source, if (destinationFd == -1) throw SysError(format("opening `%1%'") % source); copyFile(sourceFd, destinationFd); + fchown(destinationFd, st.st_uid, st.st_gid); } else if (S_ISLNK(st.st_mode)) { char target[st.st_size + 1]; ssize_t result = readlinkat(sourceroot, source.c_str(), target, st.st_size); @@ -430,6 +431,8 @@ static void copyFileRecursively(int sourceroot, const Path &source, int err = symlinkat(target, destinationroot, destination.c_str()); if (err != 0) throw SysError(format("creating symlink `%1%'") % destination); + fchownat(destinationroot, destination.c_str(), + st.st_uid, st.st_gid, AT_SYMLINK_NOFOLLOW); } else if (S_ISDIR(st.st_mode)) { int err = mkdirat(destinationroot, destination.c_str(), 0755); if (err != 0) @@ -455,6 +458,7 @@ static void copyFileRecursively(int sourceroot, const Path &source, for (auto & i : readDirectory(sourceFd)) copyFileRecursively((int)sourceFd, i.name, (int)destinationFd, i.name, deleteSource); + fchown(destinationFd, st.st_uid, st.st_gid); } else throw Error(format("refusing to copy irregular file `%1%'") % source); if (deleteSource) diff --git a/nix/libutil/util.hh b/nix/libutil/util.hh index 058f5f8446..377aac0684 100644 --- a/nix/libutil/util.hh +++ b/nix/libutil/util.hh @@ -102,9 +102,10 @@ void deletePath(const Path & path); void deletePath(const Path & path, unsigned long long & bytesFreed, size_t linkThreshold = 1); -/* Copy SOURCE to DESTINATION, recursively. Throw if SOURCE contains a file - that is not a regular file, symlink, or directory. When DELETESOURCE is - true, delete source files once they have been copied. */ +/* Copy SOURCE to DESTINATION, recursively, preserving ownership. Throw if + SOURCE contains a file that is not a regular file, symlink, or directory. + When DELETESOURCE is true, delete source files once they have been + copied. */ void copyFileRecursively(const Path &source, const Path &destination, bool deleteSource = false); -- cgit v1.2.3