From patchwork Mon Mar 11 10:54:00 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ludovic_Court=C3=A8s?= X-Patchwork-Id: 61623 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id E438927BBEA; Mon, 11 Mar 2024 10:54:40 +0000 (GMT) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SPF_HELO_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id 33D7727BBE2 for ; Mon, 11 Mar 2024 10:54:39 +0000 (GMT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rjdIb-0000qF-F1; Mon, 11 Mar 2024 06:54:29 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rjdIa-0000q6-S2 for guix-patches@gnu.org; Mon, 11 Mar 2024 06:54:28 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rjdIa-0006bV-Ju for guix-patches@gnu.org; Mon, 11 Mar 2024 06:54:28 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rjdJ8-0002C1-15 for guix-patches@gnu.org; Mon, 11 Mar 2024 06:55:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#69728] [PATCH security] daemon: Protect against FD escape when building fixed-output derivations (CVE-2024-27297). Resent-From: Ludovic =?utf-8?q?Court=C3=A8s?= Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Mon, 11 Mar 2024 10:55:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: report 69728 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 69728@debbugs.gnu.org Cc: Picnoir , Ludovic =?utf-8?q?Court=C3=A8s?= , =?utf-8?q?Th=C3=A9ophane?= Hufschmitt , guix-security@gnu.org X-Debbugs-Original-To: guix-patches@gnu.org Received: via spool by submit@debbugs.gnu.org id=B.17101545008416 (code B ref -1); Mon, 11 Mar 2024 10:55:01 +0000 Received: (at submit) by debbugs.gnu.org; 11 Mar 2024 10:55:00 +0000 Received: from localhost ([127.0.0.1]:38958 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rjdJ5-0002Bf-5S for submit@debbugs.gnu.org; Mon, 11 Mar 2024 06:54:59 -0400 Received: from lists.gnu.org ([209.51.188.17]:43120) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rjdJ2-0002BX-QN for submit@debbugs.gnu.org; Mon, 11 Mar 2024 06:54:58 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rjdIU-0000nx-DA; Mon, 11 Mar 2024 06:54:22 -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 ) id 1rjdIT-0006Zs-58; Mon, 11 Mar 2024 06:54:21 -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:Subject:To:From:in-reply-to: references; bh=l+5IyV/i7GICh3SjFJjAcftWFvuvO+vhOlf4jLTrQGA=; b=bpmAUwQ8cSsB9s 0qSrDHmtmmqccunDIhp4tNaD4MUz5ZuB3w4j+5nI76bYTJFVhoQHUfc+lCR/x8//W6NwxoqTI4sn7 fJx11qUQFu9Qa971Tdg8CDs4J8dMxJwnIDSV6fpcG7kCqt0C5yF9Fo7i9FxXjGTcrFp/XbHf46JeE Cw9wGxa5YIgVtHUuvyshOzNIcF2Q/fGpQ8W02h5nUBtDcVACMlfJceWGoXb/J3Z4QD1MWZk8JZmsh gNyp8IuItgZk8Zp9rxx77lj7eOmRRV8dUyPAVjm04Vb0xwAWttJzASVhfgf0p0DoGEox/A3fWn9jz q/fBU9yqs/0ysSOWcrng==; From: Ludovic =?utf-8?q?Court=C3=A8s?= Date: Mon, 11 Mar 2024 11:54:00 +0100 Message-ID: X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org X-getmail-retrieved-from-mailbox: Patches 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 , Théophane Hufschmitt 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’. base-commit: c7836393be4d134861d652b2fcf09cf4e68275ca 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);