[bug#73919] Daemon vulnerability allowing takeover of build users

[bug#73919] Daemon vulnerability allowing takeover of build users

[Ludovic Courtès]

[-- 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

[bug#73919] News entry

[Reepca Russelstein via Guix-patches via]

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

Here's a news entry describing the vulnerability.  The "TBD" commit
should be replaced with the one that updated the guix package.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-etc-news-add-news-entry-for-build-user-takeover-vuln.patch --]
[-- Type: text/x-patch, Size: 2615 bytes --]

From 532996c5908fb14cc8d102865280fb203c075c9c Mon Sep 17 00:00:00 2001
From: Reepca Russelstein <reepca@russelstein.xyz>
Date: Sun, 20 Oct 2024 17:32:23 -0500
Subject: [PATCH] etc: news: add news entry for build user takeover
 vulnerability fix.

* etc/news.scm: add entry about build user takeover vulnerability.
---
 etc/news.scm | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/etc/news.scm b/etc/news.scm
index a90f92a..20cc3d7 100644
--- a/etc/news.scm
+++ b/etc/news.scm
@@ -33,6 +33,38 @@
 (channel-news
  (version 0)
 
+ (entry (commit "TBD")
+        (title
+         (en "Daemon vulnerability allowing takeover of build users fixed"))
+        (body
+         (en "A vulnerability allowing a local user to execute arbitrary code
+as any of the build users has been identified and fixed.  Most notably, this
+allows any local user to alter the result of any local build, even if it
+happens inside a container.  The only requirements to exploit this
+vulnerability are the ability to start a derivation build and the ability to
+run arbitrary code with access to the store in the root PID namespace on the
+machine that build occurs on.  This largely limits the vulnerability to
+multi-user systems.
+
+This vulnerability is caused by the fact that @command{guix-daemon} does not
+change ownership and permissions on the outputs of failed builds when it moves
+them to the store, and is also caused by there being a window of time between
+when it moves outputs of successful builds to the store and when it changes
+their ownership and permissions.  Because of this, a build can create a binary
+with both setuid and setgid bits set and have it become visible to the outside
+world once the build ends.  At that point any process that can access the
+store can execute it and gain the build user's privileges.  From there any
+process owned by that build user can be manipulated via procfs and signals at
+will, allowing the attacker to control the output of its builds.
+
+You are advised to upgrade @command{guix-daemon}.  Run @command{info \"(guix)
+Upgrading Guix\"}, for info on how to do that.  Additionally, if there is any
+risk that a builder may have already created these setuid binaries (for
+example on accident), run @command{guix gc} to remove all failed build
+outputs.
+
+See @uref{https://issues.guix.gnu.org/73919} for more information on this
+vulnerability.")))
  (entry (commit "2fae63df2138b74d30e120364f0f272871595862")
         (title
          (en "Core packages updated")
-- 
2.45.2


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

[bug#73919] Daemon vulnerability allowing takeover of build users

[Ludovic Courtès]

The two patches by Reepca have been pushed, followed by an update of the
‘guix’ package, followed by a news entry:

  c9e51ab38d * news: Add news entry for build user takeover vulnerability fix.
  5966e0fdc7 * gnu: guix: Update to 5ab3c4c.
  5ab3c4c1e4 * daemon: Sanitize successful build outputs prior to exposing them.
  558224140d * daemon: Sanitize failed build outputs prior to exposing them.

Now’s the time to upgrade your system!

Ludo’.

[bug#73919] [PATCH guix-artwork] add security advisory post about recent vulnerability

[Reepca Russelstein via Guix-patches via]

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

Patch attached, let me know if anything needs revision (or make the
change yourself, I'm going to have to sleep soon).

- reepca


[-- Attachment #1.2: 0001-website-Add-2024-10-21-security-advisory-post.patch --]
[-- Type: text/x-patch, Size: 11756 bytes --]

From 22adb50845bf4af7b1d6fd78e2515c3387356ce1 Mon Sep 17 00:00:00 2001
From: Reepca Russelstein <reepca@russelstein.xyz>
Date: Mon, 21 Oct 2024 00:04:32 -0500
Subject: [PATCH] website: Add 2024-10-21 security advisory post

* website/posts/2024-10-21-security-advisory.md: new file.
---
 website/posts/2024-10-21-security-advisory.md | 212 ++++++++++++++++++
 1 file changed, 212 insertions(+)
 create mode 100644 website/posts/2024-10-21-security-advisory.md

diff --git a/website/posts/2024-10-21-security-advisory.md b/website/posts/2024-10-21-security-advisory.md
new file mode 100644
index 0000000..8850da7
--- /dev/null
+++ b/website/posts/2024-10-21-security-advisory.md
@@ -0,0 +1,212 @@
+title: Build User Takeover Vulnerability
+author: Caleb Ristvedt
+tags: Security Advisory
+date: 2024-10-21 06:00
+---
+A security issue has been identified in
+[`guix-daemon`](https://guix.gnu.org/en/manual/devel/en/html_node/Invoking-guix_002ddaemon.html)
+which allows for a local user to gain the privileges of any of the build users
+and subsequently use this to manipulate the output of any build. Specifically,
+this exploit requires the ability to start a derivation build and the ability to
+run arbitrary code with access to the store in the root PID namespace on the
+machine the build occurs on. As such, this represents an increased risk
+primarily to multi-user systems and systems using dedicated privilege-separation
+users for various daemons: without special sandboxing measures, any process of
+theirs can take advantage of this vulnerability.
+
+# Vulnerability
+
+For a very long time, `guix-daemon` [has helpfully made the outputs of failed
+derivation builds
+available](https://git.savannah.gnu.org/cgit/guix.git/tree/nix/libstore/build.cc?id=e951a375a01262dfd470ee343baf7c41dbc6ff58#n1371)
+at the same location they were at in the build container. This has aided greatly
+especially in situations where test suites require the package to already be
+installed in order to run, as it allows one to re-run the test suite
+interactively outside of the container when built with `--keep-failed`. This
+transferral of store items from inside the chroot to the real store was
+implemented with a simple `rename()`, and no modification of the store item or
+any files it may contain.
+
+If an attacker starts a build of a derivation that creates a binary with the
+setuid and/or setgid bit in an output directory, then, and the build fails, that
+binary will be accessible unaltered for anybody on the system. The attacker or a
+cooperating user can then execute the binary, gain the privileges, and from
+there use a combination of signals and procfs to freeze a builder, open any file
+it has open via /proc/$PID/fd, and overwrite it with whatever it wants. This
+manipulation of builds can happen regardless of which user started the build, so
+it can work not only for producing compromised outputs for commonly-used
+programs before anybody else uses them, but also for compromising any builds
+another user happens to start.
+
+A related vulnerability was also discovered concerning the outputs of
+*successful* builds. These were
+[moved](https://git.savannah.gnu.org/cgit/guix.git/tree/nix/libstore/build.cc?id=e951a375a01262dfd470ee343baf7c41dbc6ff58#n2343) -
+also via `rename()` - outside of the container prior to having their
+permissions, ownership, and timestamps
+[canonicalized](https://git.savannah.gnu.org/cgit/guix.git/tree/nix/libstore/build.cc?id=e951a375a01262dfd470ee343baf7c41dbc6ff58#n2429). This
+means that there also exists a window of time for a successful build's outputs
+during which a setuid/setgid binary can be executed.
+
+In general, any time that a build user running a build for some submitter can
+get a setuid/setgid binary to a place the submitter can execute it, it is
+possible for the submitter to use it to take over the build user. This situation
+always occurs when `--disable-chroot` is passed to `guix-daemon`. This holds
+even in the case where there are no dedicated build users, and builds happen
+under the same user the daemon runs as, as happens during `make check` in the
+guix repository. Consequently, if a permissive umask that allows execute
+permission for untrusted users on directories all the way to a user's guix
+checkout is used, an attacker can use that user's test-environment daemon to
+gain control over their user while `make check` is running.
+
+# Mitigation
+
+This security issue (tracked [here](https://issues.guix.gnu.org/73919) for GNU
+Guix) has been fixed by
+[two](https://git.savannah.gnu.org/cgit/guix.git/commit/?id=558224140dab669cabdaebabff18504a066c48d4)
+[commits](https://git.savannah.gnu.org/cgit/guix.git/commit/?id=5ab3c4c1e43ebb637551223791db0ea3519986e1). Users
+should make sure they have updated to the second commit to be protected from
+this vulnerability. Upgrade instructions are in the following section. If there
+is a possibility that a failed build has left a setuid/setgid binary lying
+around in the store by accident, run `guix gc` to remove all failed build
+outputs.
+
+The fix was accomplished by sanitizing the permissions of all files in a failed
+build output prior to moving it to the store, and also by waiting to move
+successful build outputs to the store until after their permissions had been
+canonicalized. The sanitizing was done in such a way as to preserve as many
+non-security-critical properties of failed build outputs as possible to aid in
+debugging. After applying these two commits, the guix package in guix was
+[updated](https://git.savannah.gnu.org/cgit/guix.git/commit/?id=5966e0fdc78771c562e0f484a22f381a77908be0)
+so that guix-daemon deployed using it would use the fixed version.
+
+If you are using `--disable-chroot`, whether with dedicated build users or not,
+make sure that access to your daemon's socket is restricted to trusted
+users. This particularly affects anyone running `make check` and anyone running
+on GNU/Hurd. The former should either manually remove execute permission for
+untrusted users on their guix checkout or apply [this
+patch](https://issues.guix.gnu.org/73924), which restricts access to the
+test-environment daemon to the user running the tests. The latter should adjust
+the ownership and permissions of `/var/guix/daemon-socket`, which can be done
+for Guix System users using the new `socket-directory-{perms,group,user}` fields
+in [this patch](https://issues.guix.gnu.org/73925).
+
+A proof of concept is available at the end of this post. One can run this code
+with 
+
+```sh
+guix repl -- setuid-exposure-vuln-check.scm
+```
+
+This will output whether the current `guix-daemon` being used is vulnerable or
+not. If it is vulnerable, the last line will contain `your system is not
+vulnerable`, otherwise the last line will contain `YOUR SYSTEM IS VULNERABLE`.
+
+# Upgrading
+
+Due to the severity of this security advisory, we strongly recommend
+all users to upgrade their `guix-daemon` immediately.
+
+For a Guix System the procedure is just reconfiguring the system after
+a `guix pull`, either restarting `guix-daemon` or rebooting. For
+example,
+
+```sh
+guix pull
+sudo guix system reconfigure /run/current-system/configuration.scm
+sudo herd restart guix-daemon
+```
+
+where `/run/current-system/configuration.scm` is the current system
+configuration but could, of course, be replaced by a system
+configuration file of a user's choice.
+
+For Guix running as a package manager on other distributions, one
+needs to `guix pull` with `sudo`, as the `guix-daemon` runs as root,
+and restart the `guix-daemon` service. For example, on a system using
+systemd to manage services,
+
+```sh
+sudo --login guix pull
+sudo systemctl restart guix-daemon.service
+```
+
+Note that for users with their distro's package of Guix (as opposed to
+having used the [install
+script](https://guix.gnu.org/en/manual/devel/en/html_node/Binary-Installation.html))
+you may need to take other steps or upgrade the Guix package as per
+other packages on your distro. Please consult the relevant
+documentation from your distro or contact the package maintainer for
+additional information or questions.
+
+# Conclusion
+
+Even with the sandboxing features of modern kernels, it can be quite challenging
+to synthesize a situation in which two users on the same system who are
+determined to cooperate nevertheless cannot. Guix has an especially difficult
+job because it needs to not only realize such a situation, but also maintain the
+ability to interact with both users itself, while not allowing them to cooperate
+through itself in unintended ways. Keeping failed build outputs around for
+debugging introduced a vulnerability, but finding that vulnerability because of
+it enabled the discovery of an additional vulnerability that would have existed
+anyway, and prompted the use of mechanisms for securing access to the guix
+daemon.
+
+I would like to thank Ludovic Courtès for giving feedback on these
+vulnerabilities and their fixes - discussion of which led to discovering the
+vulnerable time window with successful build outputs - and also for helping me
+to discover that my email server was broken.
+
+## Proof of Concept
+Below is code to check if a `guix-daemon` is vulnerable to this exploit. Save
+this file as `setuid-exposure-vuln-check.scm` and run following the instructions
+above, in "Mitigation."
+
+```scheme
+(use-modules (guix)
+             (srfi srfi-34))
+
+(define maybe-setuid-file
+  ;; Attempt to create a setuid file in the store, with one of the build
+  ;; users as its owner.
+  (computed-file "maybe-setuid-file"
+                 #~(begin
+                     (call-with-output-file #$output (const #t))
+                     (chmod #$output #o6000)
+
+                     ;; Failing causes guix-daemon to copy the output from
+                     ;; its temporary location back to the store.
+                     (exit 1))))
+
+(with-store store
+  (let* ((drv (run-with-store store
+                (lower-object maybe-setuid-file)))
+         (out (derivation->output-path drv)))
+    (guard (c (#t
+               (if (zero? (logand #o6000 (stat:perms (stat out))))
+                   (format #t "~a is not setuid: your system is not \
+vulnerable.~%"
+                           out)
+                   (format #t "~a is setuid: YOUR SYSTEM IS VULNERABLE.
+
+Run 'guix gc' to remove that file and upgrade.~%"
+                           out))))
+      (build-things store (list (derivation-file-name drv))))))
+```
+
+### About GNU Guix
+
+[GNU Guix](https://guix.gnu.org) is a transactional package manager
+and an advanced distribution of the GNU system that [respects user
+freedom](https://www.gnu.org/distros/free-system-distribution-guidelines.html).
+Guix can be used on top of any system running the Hurd or the Linux
+kernel, or it can be used as a standalone operating system
+distribution for i686, x86_64, ARMv7, AArch64, and POWER9 machines.
+
+In addition to standard package management features, Guix supports
+transactional upgrades and roll-backs, unprivileged package
+management, per-user profiles, and garbage collection. When used as a
+standalone GNU/Linux distribution, Guix offers a declarative,
+stateless approach to operating system configuration management. Guix
+is highly customizable and hackable through
+[Guile](https://www.gnu.org/software/guile) programming interfaces and
+extensions to the [Scheme](http://schemers.org) language.
-- 
2.45.2


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

[bug#73919] Daemon vulnerability allowing takeover of build users

[Ludovic Courtès]

Hello,

Reepca Russelstein <reepca@russelstein.xyz> skribis:

> From 22adb50845bf4af7b1d6fd78e2515c3387356ce1 Mon Sep 17 00:00:00 2001
> From: Reepca Russelstein <reepca@russelstein.xyz>
> Date: Mon, 21 Oct 2024 00:04:32 -0500
> Subject: [PATCH] website: Add 2024-10-21 security advisory post
>
> * website/posts/2024-10-21-security-advisory.md: new file.

I pushed this patch a few hours ago and followed up with minor edits:

  https://guix.gnu.org/blog/2024/build-user-takeover-vulnerability/

Ludo’.