GNU bug report logs - #75810
[PATCH 0/6] Rootless guix-daemon

Previous Next

Full log

Message #35 received at 75810 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Reepca Russelstein <reepca <at> russelstein.xyz>
Cc: 75810 <at> debbugs.gnu.org
Subject: Re: [bug#75810] [PATCH 0/6] Rootless guix-daemon
Date: Mon, 27 Jan 2025 22:31:43 +0100

[Message part 1 (text/plain, inline)]

Hello Reepca,

Reepca Russelstein <reepca <at> russelstein.xyz> skribis:

> user <at> debian:~$ /gnu/store/8bjy9g0cssjrw9ljz2r8ww1sma95isfj-hello-2.12.1/bin/hello
> GOOOOOD BYYEEEEEE

This particular issue is fixed with read-only mounts:

[Message part 2 (text/x-patch, inline)]

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index c95bd2821f..e8e4a56e2d 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2175,7 +2175,7 @@ void DerivationGoal::runChild()
                     createDirs(dirOf(target));
                     writeFile(target, "");
                 }
-                if (mount(source.c_str(), target.c_str(), "", MS_BIND, 0) == -1)
+                if (mount(source.c_str(), target.c_str(), "", MS_BIND | MS_RDONLY, 0) == -1)
                     throw SysError(format("bind mount from `%1%' to `%2%' failed") % source % target);
             }
 

[Message part 3 (text/plain, inline)]

(I checked that it does the right thing.)

The fix is trivial, but I’m glad you found the bug in the first place;
it does stress that we have to be careful here.

> I suppose we could try to perform these bind-mounts with the MS_RDONLY
> flag, but we would need some way to ensure that the builder can't just
> remount them read-write

The example below tests that; ‘mount’ fails with EPERM when using the
unprivileged daemon (‘./test-env guix build -f …’):

--8<---------------cut here---------------start------------->8---
(use-modules (guix)
             (guix modules)
             (gnu packages bootstrap))

(computed-file "try-to-remount-input-read-write"
               (with-imported-modules (source-module-closure
                                       '((guix build syscalls)))
                 #~(begin
                     (use-modules (guix build syscalls))

                     (let ((input #$(plain-file "input-that-might-be-tampered-with"
                                                "All good!")))
                       (mount "none" input "none" (logior MS_BIND MS_REMOUNT))
                       (call-with-output-file input
                         (lambda (port)
                           (display "BAD!" port)))
                       (mkdir #$output))))
               #:guile %bootstrap-guile)
--8<---------------cut here---------------end--------------->8---

This is similar to:

  guix shell -C guile guix -- \
    guile -c '(use-modules (guix build syscalls)) (mount "none" (getenv "GUIX_ENVIRONMENT") "none" (logior MS_BIND MS_REMOUNT))'

mount(2) has this:

   EPERM  An attempt was made to modify (MS_REMOUNT) the MS_RDONLY, MS_NO‐
          SUID, or MS_NOEXEC flag, or one of the "atime"  flags  (MS_NOAT‐
          IME,  MS_NODIRATIME,  MS_RELATIME) of an existing mount, but the
          mount is locked; see mount_namespaces(7).

I couldn’t find the definite answer in mount_namespaces(7) as to whether
this applies in this case (same namespace but after chroot); I can only
tell empirically that it does apply.

>> This patch changes guix-daemon so it can run as an unprivileged
>> user, using unprivileged user namespaces to still support isolated
>> builds.  There’s a couple of cases where root is/was still necessary:
>> 
>>   1. To create /var/guix/profiles/per-user/$USER and chown it
>>      as $USER (see CVE-2019-18192).
>> 
>>   2. To chown /tmp/guix-build-* when using ‘--keep-failed’.
>> 
>> Both can be addressed by giving CAP_CHOWN to guix-daemon, and this is
>> what this patch series does on distros using systemd.  (For some
>> reason CAP_CHOWN had to be added to the set of “ambient capabilities”,
>> which are inherited by child processes; this is why there’s a patch
>> to drop ambient capabilities in build processes.)
>> 
>> On Guix System (not implemented here), we could address (1) by
>> creating /var/guix/profiles/per-user/$USER upfront for all the
>> user accounts.  We could leave (2) unaddressed (so failed build
>> directories would be owned by guix-daemon:guix-daemon) or we’d
>> have to pass CAP_CHOWN as well.

[...]

> This does raise the question, though, of how these failed build
> directories would get deleted, aside from rebooting the system.

Note that in the early days (and in current Nix actually), build trees
were not chowned.  That’s OK: they’re deleted upon reboot or by the
system administrator.

Current Nix has this:

--8<---------------cut here---------------start------------->8---
void DerivationGoal::deleteTmpDir(bool force)
{
    if (tmpDir != "") {
        /* Don't keep temporary directories for builtins because they
           might have privileged stuff (like a copy of netrc). */
        if (settings.keepFailed && !force && !drv->isBuiltin()) {
            printError("note: keeping build directory '%s'", tmpDir);
            chmod(tmpDir.c_str(), 0755);
        }
        else
            deletePath(tmpDir);
        tmpDir = "";
    }
}
--8<---------------cut here---------------end--------------->8---

We could go back to this.  It’s less convenient, but okay.

In this patch series, it attempts to chown the tree; if it fails to do
so (because it lacks CAP_CHOWN), it prints a warning and keeps going.

> Perhaps the garbage collector could be modified to get rid of them?
> In which case it may be best to make it so that the failed build
> directories are automatically added to the temp roots for a client,
> and the client takes care to copy the failed build directory to a
> fresh path owned by the current user?  Or we could make it so that the
> failed build directory gets sent over the wire in nar form to the
> client.  Not sure what the best approach there is.

Dunno.  Sending it as nar may be too heavyweight and quite a bit of
work.

I’d say it goes beyond the scope of this patch series, though.

> We currently remount /gnu/store read-write at LocalStore-creation-time,
> which happens in the newly-forked guix-daemon process at the start of a
> connection.  I don't think there's any particularly elevated risk from
> instead doing that before the per-connection process is forked.  There
> are a number of ways we could do this: we could make it the
> responsibility of the init system to create the mount namespace and do
> the remounting, or we could have guix-daemon do it immediately on
> startup and subsequently switch its uid and gid to
> guix-daemon:guix-daemon.  These lack the slick appeal of "see, you never
> have to give it root, and you can prove it just by looking at the
> service file", but realistically should be just as secure.  It may be
> useful to provide a small wrapper around guix-daemon that does the
> remount and privilege-dropping, to more succinctly express this to
> anybody wishing to see for themselves.

I think I’d prefer to have a systemd (or other) service make a
read-write bind-mount at /gnu/store/.rw-store, and then we’d run
‘guix-daemon --backing-store=/gnu/store/.rw-store’.

WDYT?

> There are, effectively, 3 platforms that guix currently supports: posix,
> linux, and hurd.

Rather two: Linux and Hurd.  But note: we don’t use any Hurd-specific
features yet, and in practice all the energy and focus is on Linux (on
the Hurd we run ‘guix-daemon --disable-chroot’ anyway).

Adding the privileged/unprivileged setting, we’d have two configurations
really, again setting aside the Hurd.

The way I see it, if everything goes well, we’d default to unprivileged
guix-daemon on Guix System as well and eventually (longer term) drop the
privileged daemon.

> Personally, I think that if a guix-daemon can use privilege separation
> users, it would probably be a good idea to.  We're certainly going to
> need to support them on non-linux systems either way.  Could it be
> possible to have guix-install.sh modify /etc/sudoers on systems that use
> it to allow the guix-daemon user to run processes under guix builder
> users?  I am currently less worried about arbitrary code execution
> vulnerabilities being found in the daemon than about the possibility of
> malicious builders (but it is possible I am underexposed to the ways
> those can happen in C++).

What would you put in /etc/sudoers?  I’m not sure what you had in mind.

Aside, I’d rather avoid relying on external tools like ‘sudo’.

> Additionally, CAP_CHOWN, while not having a direct path to privilege
> escalation due to setuid and setgid bits being reset when chown is
> called, can nevertheless be easily leveraged into privilege escalation
> in most real-world situations where arbitrary code execution is
> possible, so switching to using just that capability would realistically
> only add defense in less-than-arbitrary-code-execution scenarios.

I agree about CAP_CHOWN, which is why I proposed scenarios without it.

Thanks a lot for your feedback!

I’ll send a second version addressing the immediate issue you found
and, if everything goes well, an attempt at restoring the /gnu/store
read-only bind-mount.

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.