diff mbox series

[bug#73923] Vulnerability allowing takeover of build users

Message ID 871q0albbb.fsf@russelstein.xyz
State New
Headers show
Series [bug#73923] Vulnerability allowing takeover of build users | expand

Commit Message

Reepca Russelstein Oct. 20, 2024, 9:22 p.m. UTC 
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
https://git.savannah.gnu.org/cgit/guix.git/tree/nix/libstore/build.cc?id=e951a375a01262dfd470ee343baf7c41dbc6ff58#n1371).
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
https://git.savannah.gnu.org/cgit/guix.git/tree/nix/libstore/build.cc?id=e951a375a01262dfd470ee343baf7c41dbc6ff58#n2343).

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

Comments

Andreas Enge Oct. 22, 2024, 9:07 a.m. UTC | #1 
Hello,

as I understand it, the patch has been applied? Then the bug could be closed.

Andreas
diff mbox series

Patch

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

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