02/02: daemon: Protect against FD escape when building fixed-output derivations (CVE-2024-27297).

[guix-commits]

civodul pushed a commit to branch master
in repository guix.

commit 8f4ffb3fae133bb21d7991e97c2f19a7108b1143
Author: Ludovic Courtès <ludo@gnu.org>
AuthorDate: Mon Mar 11 10:59:42 2024 +0100

    daemon: Protect against FD escape when building fixed-output derivations 
(CVE-2024-27297).
    
    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 <https://github.com/puckipedia>.
    
    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 <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(-)

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