[bug#69728] [PATCH security] daemon: Protect against FD escape when building fixed-output derivations (CVE-2024-27297).

* [bug#69728] [PATCH security] daemon: Protect against FD escape when building fixed-output derivations (CVE-2024-27297).
@ 2024-03-11 10:54 Ludovic Courtès
  2024-03-11 22:16 ` Ludovic Courtès
  0 siblings, 1 reply; 8+ messages in thread
From: Ludovic Courtès @ 2024-03-11 10:54 UTC (permalink / raw)
  To: 69728
  Cc: Picnoir, Ludovic Courtès, Théophane Hufschmitt,
	guix-security

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”.

Nix security advisory:
https://github.com/NixOS/nix/security/advisories/GHSA-2ffj-w4mj-pg37

* 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 <picnoir@alternativebit.fr>, Théophane Hufschmitt <theophane.hufschmitt@tweag.io>
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(-)

Hello,

On Friday, March 8th, fellow Nix developers Picnoir and Théophane
Hufschmitt contacted the Guix security team to let us know about
a security vulnerability in the Nix daemon they had just found and
addressed in Nix:

  advisory: https://github.com/NixOS/nix/security/advisories/GHSA-2ffj-w4mj-pg37
  PoC: https://hackmd.io/03UGerewRcy3db44JQoWvw
  fix: https://github.com/NixOS/nix/commit/244f3eee0bbc7f11e9b383a15ed7368e2c4becc9

By sending a file descriptor to another process on the same machine, a
fixed-output derivation build process could give write access to a store
item to an unprivileged process, effectively giving an unprivileged user
the ability to corrupt that store item.

The fix implemented by Nix hackers is nice and simple: upon build
completion, the output is copied to a new location and deleted, such
that any file descriptors that might have been shared now point to
unlinked files.  (The PoC above looks at various other ways to fix the
problem, and this one is by far the simplest.)

The patch below “backports” the Nix fix to our daemon.  I ended up
having to implement my own ‘copyFileRecursively’ function, which is
not great (recent versions of Nix use C++17 ‘std::filesystem’ but we’re
stuck on C++11 and making the switch didn’t feel appealing either.)

I tested the patch locally with things like:

  strace -o log.strace -f ./test-env guix build -S guile-ssh
  strace -o log.strace -f ./test-env guix build -S guile-ssh --check
  strace -o log.strace -f ./test-env guix build -S hello

… looking at the strace output to make sure things were happening as
expected.  Also, “make check” passes.

We’d like to push it probably today, but we’d very much like to get
more eyeballs on this code!

Thanks again to Picnoir and Théophane!

Ludo’.

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);

base-commit: c7836393be4d134861d652b2fcf09cf4e68275ca
-- 
2.41.0





^ permalink raw reply related	[flat|nested] 8+ messages in thread