Subject: bug#77862: guix-daemon run as non-root sets up /etc/group incorrectly in build container

From: Ludovic Courtès <[email-masked]>
Date: Mon, 19 May 2025 15:37:20 +0200

Hello,

(Cc: Reepca.)

keinflue <[email-masked]> writes:

> It seems that the "chown to overflowgid" issue is somewhat
> widespread. I also see the testsuite for go (bootstrap) failing in the
> same way. I'd guess most implementations of "chown" system call
> wrappers in various languages will have test cases like this that fail
> to anticipate user namespaces. I will let my system build keep running
> a bit longer and will then post the list of packages I found with log
> excerpts here.

I think it would be best to support chown-to-supplementary-group even
with the unprivileged daemon (specifically for the case where
guix-daemon runs as a dedicated user, and that this user has one
supplementary group, kvm).

The attached patch tries to do that, by calling out to ‘newuidmap’, and
under the assumption that /etc/subgid allows mapping the ‘kvm’ group.

It does the job (a build process can chown to ‘kvm’), but I couldn’t get
the GID mapping preserved across the ‘unshare’ call (the call that is
made to “lock” mounts), hence the “#if 0” there.

The problem is that when we call ‘unshare’, the ‘newgidmap’ setuid
binary is not longer accessible because we’re already in a chroot, so it
seems that we cannot preserve the GID map.

Thoughts?

Ludo’.

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index a1f39d9a8b..83754b06bd 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -13,6 +13,7 @@
 #include <map>
 #include <sstream>
 #include <algorithm>
+#include <iostream>
 
 #include <limits.h>
 #include <time.h>
@@ -23,6 +24,7 @@
 #include <sys/utsname.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <grp.h>
 #include <errno.h>
 #include <stdio.h>
 #include <cstring>
@@ -1635,15 +1637,77 @@ static const gid_t guestGID = 30000;
 /* Initialize the user namespace of CHILD.  */
 static void initializeUserNamespace(pid_t child,
 				    uid_t hostUID = getuid(),
-				    gid_t hostGID = getgid())
+				    gid_t hostGID = getgid(),
+				    const std::vector<std::pair<gid_t, gid_t>> extraGIDs = {},
+				    bool haveCapSetGID = false)
 {
     writeFile("/proc/" + std::to_string(child) + "/uid_map",
 	      (format("%d %d 1") % guestUID % hostUID).str());
 
-    writeFile("/proc/" + std::to_string(child) + "/setgroups", "deny");
+    if (!haveCapSetGID && !extraGIDs.empty()) {
+	try {
+	    Strings args = {
+		std::to_string(child),
+		std::to_string(guestGID), std::to_string(hostGID), "1"
+	    };
+	    for (auto &pair: extraGIDs) {
+		args.push_back(std::to_string(pair.second));
+		args.push_back(std::to_string(pair.first));
+		args.push_back("1");
+	    }
+	    runProgram("newgidmap", true, args);
+	    printMsg(lvlChatty,
+		     format("mapped %1% extra GIDs into namespace of PID %2%")
+		     % extraGIDs.size() % child);
+	    return;
+	} catch (const ExecError &e) {
+	    ignoreException();
+	}
+	std::cerr << "here i am\n";
+    }
 
-    writeFile("/proc/" + std::to_string(child) + "/gid_map",
-	      (format("%d %d 1") % guestGID % hostGID).str());
+    if (!haveCapSetGID)
+	writeFile("/proc/" + std::to_string(child) + "/setgroups", "deny");
+
+    auto content = (format("%d %d 1\n") % guestGID % hostGID).str();
+    if (haveCapSetGID) {
+	for (auto &mapping: extraGIDs) {
+	    content += (format("%d %d 1\n") % mapping.second % mapping.first).str();
+	}
+    }
+    writeFile("/proc/" + std::to_string(child) + "/gid_map", content);
+}
+
+/* Return the ID of the "kvm" group or -1 if it does not exist or is not part
+   of the current users supplementary groups.  */
+static gid_t kvmGID()
+{
+    struct group *kvm = getgrnam("kvm");
+    if (kvm == NULL) return -1;
+
+    size_t max = 64;
+    gid_t groups[max];
+    int count = getgroups(max, groups);
+    if (count < 0) return -1;
+
+    for (int i = 0; i < count; i++) {
+	if (groups[i] == kvm->gr_gid) return kvm->gr_gid;
+    }
+
+    return -1;
+}
+
+static gid_t guestKVMGID = 40000;
+
+static std::vector<std::pair<gid_t, gid_t>> kvmGIDMapping()
+{
+    gid_t kvm = kvmGID();
+    if (kvm == -1)
+	return {};
+    else {
+	std::pair<gid_t, gid_t> mapping(kvm, guestKVMGID);
+	return { mapping };
+    }
 }
 
 void DerivationGoal::startBuilder()
@@ -2016,8 +2080,9 @@ void DerivationGoal::startBuilder()
 	readiness.readSide.close();
 	if ((flags & CLONE_NEWUSER) != 0) {
 	     /* Initialize the UID/GID mapping of the child process.  */
-	     initializeUserNamespace(pid);
-	     writeFull(readiness.writeSide, (unsigned char*)"go\n", 3);
+	    auto extraGIDs = kvmGIDMapping();
+	    initializeUserNamespace(pid, getuid(), getgid(), extraGIDs);
+	    writeFull(readiness.writeSide, (unsigned char*)"go\n", 3);
 	}
 	readiness.writeSide.close();
     } else
@@ -2269,10 +2334,12 @@ void DerivationGoal::runChild()
 		auto uid = getuid();
 		auto gid = getgid();
 
+#if 0						  // FIXME!
 		if (unshare(CLONE_NEWNS | CLONE_NEWUSER) == -1)
 		    throw SysError(format("creating new user and mount namespaces"));
 
-		initializeUserNamespace(getpid(), uid, gid);
+		auto extraGIDs = { std::pair<gid_t, gid_t>(guestKVMGID, guestKVMGID) };
+		initializeUserNamespace(getpid(), uid, gid, extraGIDs, true);
 
 		/* Check that mounts within the build environment are "locked"
 		   together and cannot be separated from within the build
@@ -2282,6 +2349,7 @@ void DerivationGoal::runChild()
 		   check that this is what we get.  */
 		int ret = umount(tmpDirInSandbox.c_str());
 		assert(ret == -1 && errno == EINVAL);
+#endif
 	    }
         }
 #endif
diff --git a/nix/libutil/util.cc b/nix/libutil/util.cc
index 56f116046c..b2c9b9f639 100644
--- a/nix/libutil/util.cc
+++ b/nix/libutil/util.cc
@@ -1081,9 +1081,11 @@ string runProgram(Path program, bool searchPath, const Strings & args)
 
     /* Wait for the child to finish. */
     int status = pid.wait(true);
-    if (!statusOk(status))
+    std::cerr << "status not ok: " << status << "\n";
+    if (!statusOk(status)) {
         throw ExecError(format("program `%1%' %2%")
             % program % statusToString(status));
+    }
 
     return result;
 }