[bug#73923] Vulnerability allowing takeover of build users

[bug#73923] Vulnerability allowing takeover of build users

[Reepca Russelstein via Guix-patches via]

[-- Attachment #1.1: Type: text/plain, Size: 1519 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-nix-build-sanitize-failed-build-outputs-prior-to-exp.patch --]
[-- Type: text/x-patch, Size: 3070 bytes --]

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

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-nix-build-sanitize-successful-build-outputs-prior-to.patch --]
[-- Type: text/x-patch, Size: 3052 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 519 bytes --]

[bug#73923] Close?

[Andreas Enge]

Hello,

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

Andreas