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

[Ludovic Courtès]

Hello,

Ludovic Courtès <ludo@gnu.org> skribis:

> Pushed (with a slightly different commit message) as
> 8f4ffb3fae133bb21d7991e97c2f19a7108b1143.
>
> Updated the ‘guix’ package in b8954a7faeccae11c32add7cd0f408d139af3a43:
> Guix System users can now reconfigure!
>
> Added a news entry in 4003c60abf7a6e59e47cc2deb9eef2f104ebb994.

It turns out that the previous fix was incomplete due to a mistake of
mine.  I pushed ff1251de0bc327ec478fc66a562430fbf35aef42 to address that
(patch attached for clarity).

Commit 30a8de0bcdadfb55cbcaa34760527c1b767808c7 updates the ‘guix’
package again.

Now is the time to reconfigure.  I’ll send a reproducer in a separate
message.

Apologies for the mishap.

Ludo’.

commit ff1251de0bc327ec478fc66a562430fbf35aef42
Author: Ludovic Courtès <ludo@gnu.org>
Date:   Tue Mar 12 11:53:35 2024 +0100

    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

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