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

Message #16 received at 69728@debbugs.gnu.org (full text, mbox, reply):

Received: (at 69728) by debbugs.gnu.org; 12 Mar 2024 13:31:51 +0000
From debbugs-submit-bounces@debbugs.gnu.org Tue Mar 12 09:31:51 2024
Received: from localhost ([127.0.0.1]:42083 helo=debbugs.gnu.org)
	by debbugs.gnu.org with esmtp (Exim 4.84_2)
	(envelope-from <debbugs-submit-bounces@debbugs.gnu.org>)
	id 1rk2ER-0005xH-0V
	for submit@debbugs.gnu.org; Tue, 12 Mar 2024 09:31:51 -0400
Received: from eggs.gnu.org ([209.51.188.92]:37266)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <ludo@gnu.org>) id 1rk2EO-0005x4-5l
 for 69728@debbugs.gnu.org; Tue, 12 Mar 2024 09:31:49 -0400
Received: from fencepost.gnu.org ([2001:470:142:3::e])
 by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <ludo@gnu.org>)
 id 1rk2Di-0001n0-Bp; Tue, 12 Mar 2024 09:31:07 -0400
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org;
 s=fencepost-gnu-org; h=MIME-Version:Date:References:In-Reply-To:Subject:To:
 From; bh=ApG/gHDiTE1k3aAxBiJqo9n/srVBwLyCkr8lEWPgc+U=; b=JFZl/t5Vs1DXkerv0xqF
 KHOBGrygJPO1XFkcYKiLo52Pk7fFXB8szXqxdoNZfwWNTbW1rFKKGxb0LVlhakNZeVhvg4R8UsI9N
 X0QG0oWQZiPl8u5bq0xdyJGybOqSX4EUVnE0l2xH5NCNrO77leB6I/y/eNU9riOyStag+Q5VPTpgf
 mS6HSUsaNKKihcSluQN5TJ6Dt+dCYI4eWD+7tq/995f+rbKnqnhIEoU9TYMFMGUrucHG2h4FYqT+c
 vaR64BWbvo6VAXMsQ4uMhqBPyxF5j0AkxrGX/oqvEQKlodDxyzOhFxJHT7QpMzpyac7QZ+M7Tc1KZ
 R22Hb6onELQ1wg==;
From: Ludovic Courtès <ludo@gnu.org>
To: 69728@debbugs.gnu.org
Subject: Re: bug#69728: [PATCH security] daemon: Protect against FD escape
 when building fixed-output derivations (CVE-2024-27297).
In-Reply-To: <87frwwo1mo.fsf@gnu.org> ("Ludovic Courtès"'s message of "Mon, 11 Mar 2024 23:16:31 +0100")
References: <f541e64f128d82e6d9eca3b1d40e833dc06fd968.1710154382.git.ludo@gnu.org>
 <87frwwo1mo.fsf@gnu.org>
Date: Tue, 12 Mar 2024 14:31:00 +0100
Message-ID: <87ttlblgq3.fsf_-_@gnu.org>
User-Agent: Gnus/5.13 (Gnus v5.13)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
X-Spam-Score: -2.3 (--)
X-Debbugs-Envelope-To: 69728
Cc: Picnoir <picnoir@alternativebit.fr>,
 Théophane Hufschmitt <theophane.hufschmitt@tweag.io>,
 guix-security@gnu.org
X-BeenThere: debbugs-submit@debbugs.gnu.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: <debbugs-submit.debbugs.gnu.org>
List-Unsubscribe: <https://debbugs.gnu.org/cgi-bin/mailman/options/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=unsubscribe>
List-Archive: <https://debbugs.gnu.org/cgi-bin/mailman/private/debbugs-submit/>
List-Post: <mailto:debbugs-submit@debbugs.gnu.org>
List-Help: <mailto:debbugs-submit-request@debbugs.gnu.org?subject=help>
List-Subscribe: <https://debbugs.gnu.org/cgi-bin/mailman/listinfo/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=subscribe>
Errors-To: debbugs-submit-bounces@debbugs.gnu.org
Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
X-Spam-Score: -3.3 (---)

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/x-patch, inline)]

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

Send a report that this bug log contains spam.