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

Previous Next

Full log

Message #337 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 v6 00/16] Rootless guix-daemon
Date: Tue, 18 Mar 2025 15:00:16 +0100

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

Hi!

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

> (in patch 7/16, in nix/libstore/build.cc)
>
> Strictly speaking we should check whether the fds are >= 0, not > 0,
> since 0 is technically a valid file descriptor, and we use -1 to
> indicate the absence of a file descriptor.
>
> Also, readiness.readSide isn't explicitly closed in the child after
> we're done with it.

Right, fixed.

>> +	# The unprivileged daemon cannot create the log directory by itself.
>> +	mkdir /var/log/guix
>> +	chown guix-daemon:guix-daemon /var/log/guix
>> +	chmod 755 /var/log/guix
>
> (in patch 15/16, in etc/guix-install.sh)
>
> Should this be 'mkdir -p' or some other conditional creation?  If I
> understand correctly this will fail when overwriting an existing install
> using GUIX_ALLOW_OVERWRITE.

Correct, fixed as well.

(I’ve updated the branch on Codeberg.)

> Concerning guix-install.sh, to be clear, is the intent to specifically
> not support installing the rootful daemon on systemd systems?
>
> For my two cents, I do think that it's still a tradeoff - not just
> because of the reliance on different kernel mechanisms for security, but
> also because the rootless daemon currently causes visible changes to the
> build environment (EROFS on /, and nothing owned by root, for example).
> Which one should we consider the "canonical" build environment going
> forward?

The way I see it, we would gradually move to the non-root daemon:

  • First step here is to enable it by default on systemd distros.

  • Second step would be to allow Guix System users to migrate to
    non-root daemon, keeping the default unchanged.

  • Third step (a year later maybe?) would be to default to non-root
    daemon on Guix System and on remaining distros (though for these it
    might be trickier because we probably cannot rely on CAP_SYS_CHOWN,
    not as easily as with systemd at least).

The visible changes in the build environment are unfortunate; I’m hoping
they won’t have any practical impact, not any more than the other
parameters that may change currently (build UID, binfmt_misc, file
system, etc.)  We could test this hypothesis by rebuilding at least the
entire set of packages up to ‘hello’.  (I tried doing it just now in a
Debian VM but failed since the main partition cannot easily be extended;
it’ll be easier to do with Guix System.)

> The demo, modified for guix circumstances, would go something like this:
>
> 1. A derivation is created whose builder is /proc/self/exe, and whose
>    LD_PRELOAD environment variable points into a malicious store item
>    for one of its shared libraries - for example, libc.
> 2. The daemon reads this in, and, to my knowledge, does no verification
>    of the builder string.  Note that this aspect isn't actually
>    necessary, as the builder could also be a symlink to /proc/self/exe
>    from the store.
> 3. The daemon sets up the build environment, and execs /proc/self/exe.

Then there are 3 possibilities:

  1. If /proc/self/exe points to (say) /usr/bin/guix-daemon, outside the
     store, execve fails with ENOENT because that file is not mounted in
     the chroot.

  2. If /proc/self/exe points to a guix-daemon file inside the store:

     2a. If the store item containing guix-daemon is not an input of the
         derivation, execv fails with ENOENT.

     2b. If the store item containing guix-daemon is an input of the
         derivation, then it’s been remounted read-only and attempts to
         write to it fail with EROFS.

Here’s a test that fails both “rootfull” and “rootless”:

--8<---------------cut here---------------start------------->8---
(let* ((builder (add-file-tree-to-store %store
                                        `("builder" symlink "/proc/self/exe")))
       (drv (derivation %store "attempt-to-run-guix-daemon" builder '()
                        #:env-vars
                        '(("LD_PRELOAD" . "attacker-controlled.so")))))
  (guard (c ((store-protocol-error? c) c))
    (build-derivations %store (list drv))
    #f))
--8<---------------cut here---------------end--------------->8---

> There are several points at which that particular attack could be
> stopped, and I'd like to try to stop it at as many of them as possible.
> A good start would be canonicalizing the builder prior to executing it
> and then checking to make sure it is in the store.  A more general
> solution could look like writing out and then executing a tiny binary,
> something like /tmp/runbuilder, that does nothing but unlink itself and
> then exec the actual program.

If the above is correct, we’re already safe against this particular
attack.

Canonicalizing the builder cannot hurt (it’s useful in the
‘--disable-chroot’ case though mostly to prevent programming errors
rather than from a security perspective since there are many other
issues in that case), apart from adding more code to an already long
function.

For reference, the extra check I tried but that I’m inclined to not
include since it cannot catch anything new:

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

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 7484ea8fcf..970107c265 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2374,6 +2374,15 @@ void DerivationGoal::runChild()
                 writeFull(STDERR_FILENO, "error: " + string(e.what()) + "\n");
                 _exit(1);
             }
+        } else {
+	    /* Ensure that the builder is within the store.  This prevents
+	       users from using /proc/self/exe (or a symlink to it) as their
+	       builder, which could allow them to overwrite the guix-daemon
+	       binary (CVE-2019-5736).  */
+	    drv.builder = canonPath(drv.builder, true);
+	    printMsg(lvlError, format("builder is `%1%'") % drv.builder);
+	    if (!isInStore(drv.builder))
+		throw Error(format("derivation builder '%1%' is outside the store") % drv.builder);
 	}
 
         /* Fill in the arguments. */

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

Let me know what you think and I’ll send v7 accordingly.

Ludo’.

Previous Next

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