unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
* [bug#73919] Daemon vulnerability allowing takeover of build users
@ 2024-10-20 22:02 Ludovic Courtès
  2024-10-20 22:35 ` [bug#73919] News entry Reepca Russelstein via Guix-patches via
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Ludovic Courtès @ 2024-10-20 22:02 UTC (permalink / raw)
  To: 73919; +Cc: Reepca Russelstein

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

(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
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 #2: 0001-nix-build-sanitize-failed-build-outputs-prior-to-exp.patch --]
[-- Type: text/x-patch, Size: 2987 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 #3: 0002-nix-build-sanitize-successful-build-outputs-prior-to.patch --]
[-- Type: text/x-patch, Size: 2988 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-10-21 15:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-20 22:02 [bug#73919] Daemon vulnerability allowing takeover of build users Ludovic Courtès
2024-10-20 22:35 ` [bug#73919] News entry Reepca Russelstein via Guix-patches via
2024-10-20 23:28 ` [bug#73919] Daemon vulnerability allowing takeover of build users Ludovic Courtès
2024-10-21  5:10 ` [bug#73919] [PATCH guix-artwork] add security advisory post about recent vulnerability Reepca Russelstein via Guix-patches via
2024-10-21 15:36   ` [bug#73919] Daemon vulnerability allowing takeover of build users Ludovic Courtès

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).