(address . guix-patches@gnu.org)
87y12ih1q9.fsf@gnu.org
(Due to email issues, I’m sending the message below on behalf of
Reepca Russelstein <reepca@russelstein.xyz>.)
For a very long time, guix-daemon has helpfully made the outputs of
failed derivation builds available at the same location they were at in
the build container (see
This has proven quite useful for debugging of various packages, but
unfortunately it is implemented by a plain "rename" of the top-level
store items from the chroot's store to the real store. This does not
change the permissions or ownership of these files, which allows a
setuid / setgid binary created by a malicious build to become exposed to
the rest of the users, who can then use it to gain control over that
build user. They can exploit this control to overwrite the output of
any builds run by that user using /proc/PID/fd and SIGSTOP.
Also, there is a window of time for /successful/ build outputs between
when they are moved from the chroot and when their permissions are
canonicalized, which likewise allows for setuid / setgid binaries to be
exposed to other users (see
The first patch fixes the former, the second patch fixes the latter. We
then need to update the guix package to use these new commits, which I
leave to whoever applies this to do since my local repository is in a
rather unclean state and a fresh work tree may take some time to be
ready to run 'make update-guix-package'.
From e936861263d9bafdfbe395c12526f2dc48ac17d7 Mon Sep 17 00:00:00 2001
Message-ID: <e936861263d9bafdfbe395c12526f2dc48ac17d7.1729457080.git.reepca@russelstein.xyz>
From: Reepca Russelstein <reepca@russelstein.xyz>
Date: Sun, 20 Oct 2024 15:36:06 -0500
Subject: [PATCH 1/2] nix: build: sanitize failed build outputs prior to
exposing them.
The only thing keeping a rogue builder and a local user from collaborating to
usurp control over the builder's user during the build is the fact that
whatever files the builder may produce are not accessible to any other users
yet. If we're going to make them accessible, we should probably do some
sanity checking to ensure that sort of collaborating can't happen.
Currently this isn't happening when failed build outputs are moved from the
chroot as an aid to debugging.
* nix/libstore/build.cc (secureFilePerms): new function.
(DerivationGoal::buildDone): use it.
Change-Id: I9dce1e3d8813b31cabd87a0e3219bf9830d8be96
---
nix/libstore/build.cc | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
Toggle diff (58 lines)
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index d23c0944a4..67ebfe2f14 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -1301,6 +1301,34 @@ void replaceValidPath(const Path & storePath, const Path tmpPath)
MakeError(NotDeterministic, BuildError)
+/* Recursively make the file permissions of a path safe for exposure to
+ arbitrary users, but without canonicalising its permissions, timestamp, and
+ user. Throw an exception if a file type that isn't explicitly known to be
+ safe is found. */
+static void secureFilePerms(Path path)
+{
+ struct stat st;
+ if (lstat(path.c_str(), &st)) return;
+
+ switch(st.st_mode & S_IFMT) {
+ case S_IFLNK:
+ return;
+
+ case S_IFDIR:
+ for (auto & i : readDirectory(path)) {
+ secureFilePerms(path + "/" + i.name);
+ }
+ /* FALLTHROUGH */
+
+ case S_IFREG:
+ chmod(path.c_str(), (st.st_mode & ~S_IFMT) & ~(S_ISUID | S_ISGID | S_IWOTH));
+ break;
+
+ default:
+ throw Error(format("file `%1%' has an unsupported type") % path);
+ }
+}
+
void DerivationGoal::buildDone()
{
trace("build done");
@@ -1372,9 +1400,15 @@ void DerivationGoal::buildDone()
build failures. */
if (useChroot && buildMode == bmNormal)
foreach (PathSet::iterator, i, missingPaths)
- if (pathExists(chrootRootDir + *i))
+ if (pathExists(chrootRootDir + *i)) {
+ try {
+ secureFilePerms(chrootRootDir + *i);
rename((chrootRootDir + *i).c_str(), i->c_str());
+ } catch(Error & e) {
+ printMsg(lvlError, e.msg());
+ }
+ }
if (diskFull)
printMsg(lvlError, "note: build failure may have been caused by lack of free disk space");
--
2.45.2
From d096d653cc69118e05f49247ab312d0096b16656 Mon Sep 17 00:00:00 2001
Message-ID: <d096d653cc69118e05f49247ab312d0096b16656.1729457080.git.reepca@russelstein.xyz>
In-Reply-To: <e936861263d9bafdfbe395c12526f2dc48ac17d7.1729457080.git.reepca@russelstein.xyz>
References: <e936861263d9bafdfbe395c12526f2dc48ac17d7.1729457080.git.reepca@russelstein.xyz>
From: Reepca Russelstein <reepca@russelstein.xyz>
Date: Sun, 20 Oct 2024 15:39:02 -0500
Subject: [PATCH 2/2] nix: build: sanitize successful build outputs prior to
exposing them.
There is currently a window of time between when the build outputs are exposed
and when their metadata is canonicalized.
* nix/libstore/build.cc (DerivationGoal::registerOutputs): wait until after
metadata canonicalization to move successful build outputs to the store.
Change-Id: Ia995136f3f965eaf7b0e1d92af964b816f3fb276
---
nix/libstore/build.cc | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
Toggle diff (43 lines)
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 67ebfe2f14..43a8a37184 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2369,15 +2369,6 @@ void DerivationGoal::registerOutputs()
Path actualPath = path;
if (useChroot) {
actualPath = chrootRootDir + path;
- if (pathExists(actualPath)) {
- /* Move output paths from the chroot to the store. */
- if (buildMode == bmRepair)
- replaceValidPath(path, actualPath);
- else
- if (buildMode != bmCheck && rename(actualPath.c_str(), path.c_str()) == -1)
- throw SysError(format("moving build output `%1%' from the chroot to the store") % path);
- }
- if (buildMode != bmCheck) actualPath = path;
} else {
Path redirected = redirectedOutputs[path];
if (buildMode == bmRepair
@@ -2463,6 +2454,20 @@ void DerivationGoal::registerOutputs()
canonicalisePathMetaData(actualPath,
buildUser.enabled() && !rewritten ? buildUser.getUID() : -1, inodesSeen);
+ if (useChroot) {
+ if (pathExists(actualPath)) {
+ /* Now that output paths have been canonicalized (in particular
+ there are no setuid files left), move them outside of the
+ chroot and to the store. */
+ if (buildMode == bmRepair)
+ replaceValidPath(path, actualPath);
+ else
+ if (buildMode != bmCheck && rename(actualPath.c_str(), path.c_str()) == -1)
+ throw SysError(format("moving build output `%1%' from the chroot to the store") % path);
+ }
+ if (buildMode != bmCheck) actualPath = path;
+ }
+
/* For this output path, find the references to other paths
contained in it. Compute the SHA-256 NAR hash at the same
time. The hash is stored in the database so that we can
--
2.45.2