(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’.