~ruther/guix-local

ff1251de0bc327ec478fc66a562430fbf35aef42 — Ludovic Courtès 2 years ago fc1762f
daemon: Address shortcoming in previous security fix for CVE-2024-27297.

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
3 files changed, 14 insertions(+), 8 deletions(-)

M nix/libstore/build.cc
M nix/libutil/util.cc
M nix/libutil/util.hh
M nix/libstore/build.cc => nix/libstore/build.cc +6 -5
@@ 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);
		}
	    }
	}

M nix/libutil/util.cc => nix/libutil/util.cc +4 -0
@@ 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)

M nix/libutil/util.hh => nix/libutil/util.hh +4 -3
@@ 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);