From ff1251de0bc327ec478fc66a562430fbf35aef42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= Date: Tue, 12 Mar 2024 11:53:35 +0100 Subject: [PATCH] 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(-) 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);