From patchwork Sun Mar 23 14:25:00 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ludovic_Court=C3=A8s?= X-Patchwork-Id: 40736 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 4A9EC27BBE9; Sun, 23 Mar 2025 14:26:47 +0000 (GMT) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,MAILING_LIST_MULTI,RCVD_IN_DNSWL_BLOCKED, RCVD_IN_VALIDITY_CERTIFIED,RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE, SPF_HELO_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id C6BC027BBE2 for ; Sun, 23 Mar 2025 14:26:45 +0000 (GMT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1twMHR-000344-IY; Sun, 23 Mar 2025 10:26:25 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1twMHQ-00033u-LR for guix-patches@gnu.org; Sun, 23 Mar 2025 10:26:24 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1twMHQ-0005WO-C8; Sun, 23 Mar 2025 10:26:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-Version:References:In-Reply-To:Date:From:To:Subject; bh=0Kt8D+10MDGNCXLZxl8AYQypvAcokGyIGcmk6YcdbVc=; b=ph9SYUuTUeg0kuKWZYk9IlMx1ZR2uo/6dxQbUSzwsOZ3RSCFjeCH4TY7Y4LtSU6myV9lmWmnFjWLQ0xTZDAn6FrDtQOWHTefoRneu52RSYVDpgSHo7qtKA4i/SIxjBNtCTSMMaUahkwJxgCxRThDmAB4GkIOWWdbeoeZhFjYuM5WmCd1wdLe3e30/kdGhJB1BGiBfsFCYURnxeDTVKkYSh0KABUzyPgYXan+CyJWf7oSxvwQwArVvYB2K98e4vPWcVfN2QEwpnvpekLvlxxkKuH9fLdVgb2rGHymre0t29JSfyA0u7JHNTSBtnSgcr/k/qEH3O4U+74uW5P6OU1DhA==; Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1twMHJ-0005ZF-Vq; Sun, 23 Mar 2025 10:26:18 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#75810] [PATCH v8 07/16] daemon: Allow running as non-root with unprivileged user namespaces. Resent-From: Ludovic =?utf-8?q?Court=C3=A8s?= Original-Sender: "Debbugs-submit" Resent-CC: guix@cbaines.net, dev@jpoiret.xyz, ludo@gnu.org, othacehe@gnu.org, maxim.cournoyer@gmail.com, zimon.toutoune@gmail.com, me@tobias.gr, guix-patches@gnu.org Resent-Date: Sun, 23 Mar 2025 14:26:17 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 75810 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 75810@debbugs.gnu.org Cc: Ludovic =?utf-8?q?Court=C3=A8s?= , Reepca Russelstein , Christopher Baines , Josselin Poiret , Ludovic =?utf-8?q?Court=C3=A8s?= , Mathieu Othacehe , Maxim Cournoyer , Simon Tournier , Tobias Geerinckx-Rice X-Debbugs-Original-Xcc: Christopher Baines , Josselin Poiret , Ludovic =?utf-8?q?Court=C3=A8s?= , Mathieu Othacehe , Maxim Cournoyer , Simon Tournier , Tobias Geerinckx-Rice Received: via spool by 75810-submit@debbugs.gnu.org id=B75810.174273997621386 (code B ref 75810); Sun, 23 Mar 2025 14:26:17 +0000 Received: (at 75810) by debbugs.gnu.org; 23 Mar 2025 14:26:16 +0000 Received: from localhost ([127.0.0.1]:51894 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1twMHG-0005YG-0o for submit@debbugs.gnu.org; Sun, 23 Mar 2025 10:26:16 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:43770) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1twMGm-0005Qz-DZ for 75810@debbugs.gnu.org; Sun, 23 Mar 2025 10:25:52 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1twMGf-0005SR-N1; Sun, 23 Mar 2025 10:25:37 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-Version:References:In-Reply-To:Date:Subject:To: From; bh=0Kt8D+10MDGNCXLZxl8AYQypvAcokGyIGcmk6YcdbVc=; b=GoKfTTjcW44msuLWIZCq npeVnEC0FhBJdhIdU5UkNCV8CblMtnj1561rss6zmUreFh8T7acheiaOnhgbcCtj90If8eqYJ5rS9 eOwwI0KSO+WsVvd+af0eE3pwifsdcoB5FGQgxejRn/UGeOMZjlwbv1zH+RjjzRDkMyhZG23ydGJJA Gf13JNM7kjtEuQ1XFNP0LMF7TkHAV8iz3PMazkd818uSQQXigDEMVSL54MvCf/VTaf65jKbS7cROk 1EWsR0wZYjfJgiYaZGSq7nfgb2ObSuGJLlmk4ey7gWi2rew9pRgDoMjMKldw4/VwuvOyDl8u5RNjY 4rgASvOXs0iQ0g==; From: Ludovic =?utf-8?q?Court=C3=A8s?= Date: Sun, 23 Mar 2025 15:25:00 +0100 Message-ID: <9ad0da0cdfc5b581d74a5afd4078a4317eff71ed.1742739609.git.ludo@gnu.org> X-Mailer: git-send-email 2.48.1 In-Reply-To: References: MIME-Version: 1.0 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org X-getmail-retrieved-from-mailbox: Patches From: Ludovic Courtès Many thanks to Reepca Russelstein for their review and guidance on these changes. * nix/libstore/build.cc (guestUID, guestGID): New variables. (DerivationGoal)[readiness]: New field. (initializeUserNamespace): New function. (DerivationGoal::runChild): When ‘readiness.readSide’ is positive, read from it. (DerivationGoal::startBuilder): Call ‘chown’ only when ‘buildUser.enabled()’ is true. Pass CLONE_NEWUSER to ‘clone’ when ‘buildUser.enabled()’ is false or not running as root. Retry ‘clone’ without CLONE_NEWUSER upon EPERM. (DerivationGoal::registerOutputs): Make ‘actualPath’ writable before ‘rename’. (DerivationGoal::deleteTmpDir): Catch ‘SysError’ around ‘_chown’ call. * nix/libstore/local-store.cc (LocalStore::createUser): Do nothing if ‘dirs’ already exists. Warn instead of failing when failing to chown ‘dir’. * guix/substitutes.scm (%narinfo-cache-directory): Check for ‘_NIX_OPTIONS’ rather than getuid() == 0 to determine the cache location. * doc/guix.texi (Build Environment Setup): Reorganize a bit. Add section headings “Daemon Running as Root” and “The Isolated Build Environment”. Add “Daemon Running Without Privileges” subsection. Remove paragraph about ‘--disable-chroot’. (Invoking guix-daemon): Warn against ‘--disable-chroot’ and explain why. * tests/derivations.scm ("builder is outside the store"): New test. Reviewed-by: Reepca Russelstein --- doc/guix.texi | 102 +++++++++++++++----- guix/substitutes.scm | 2 +- nix/libstore/build.cc | 183 +++++++++++++++++++++++++++++++----- nix/libstore/local-store.cc | 18 +++- tests/derivations.scm | 14 +++ 5 files changed, 264 insertions(+), 55 deletions(-) diff --git a/doc/guix.texi b/doc/guix.texi index 0adc6f8c65..ec5d41bbf0 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -877,6 +877,7 @@ Setting Up the Daemon @section Setting Up the Daemon @cindex daemon +@cindex build daemon During installation, the @dfn{build daemon} that must be running to use Guix has already been set up and you can run @command{guix} commands in your terminal program, @pxref{Getting Started}: @@ -921,20 +922,38 @@ Build Environment Setup @cindex build environment In a standard multi-user setup, Guix and its daemon---the @command{guix-daemon} program---are installed by the system -administrator; @file{/gnu/store} is owned by @code{root} and -@command{guix-daemon} runs as @code{root}. Unprivileged users may use -Guix tools to build packages or otherwise access the store, and the -daemon will do it on their behalf, ensuring that the store is kept in a -consistent state, and allowing built packages to be shared among users. +administrator. Unprivileged users may use Guix tools to build packages +or otherwise access the store, and the daemon will do it on their +behalf, ensuring that the store is kept in a consistent state, and +allowing built packages to be shared among users. + +There are currently two ways to set up and run the build daemon: + +@enumerate +@item +running @command{guix-daemon} as ``root'', letting it run build +processes as unprivileged users taken from a pool of build users---this +is the historical approach; + +@item +running @command{guix-daemon} as a separate unprivileged user, relying +on Linux's @dfn{unprivileged user namespace} functionality to set up +isolated environments---this is the option chosen when installing Guix +on a systemd-based distribution with the installation script +(@pxref{Binary Installation}). +@end enumerate + +The sections below describe each of these two configurations in more +detail and summarize the kind of build isolation they provide. + +@unnumberedsubsubsec Daemon Running as Root @cindex build users When @command{guix-daemon} runs as @code{root}, you may not want package build processes themselves to run as @code{root} too, for obvious security reasons. To avoid that, a special pool of @dfn{build users} should be created for use by build processes started by the daemon. -These build users need not have a shell and a home directory: they will -just be used when the daemon drops @code{root} privileges in build -processes. Having several such users allows the daemon to launch +Having several such users allows the daemon to launch distinct build processes under separate UIDs, which guarantees that they do not interfere with each other---an essential feature since builds are regarded as pure functions (@pxref{Introduction}). @@ -977,11 +996,45 @@ Build Environment Setup # guix-daemon --build-users-group=guixbuild @end example +In this setup, @file{/gnu/store} is owned by @code{root}. + +@unnumberedsubsubsec Daemon Running Without Privileges + +@cindex rootless build daemon +@cindex unprivileged build daemon +@cindex build daemon, unprivileged +The second and preferred option is to run @command{guix-daemon} +@emph{as an unprivileged user}. It has the advantage of reducing the +harm that can be done should a build process manage to exploit a +vulnerability in the daemon. This option requires the use of Linux's +unprivileged user namespace mechanism; today it is available and enabled +by most GNU/Linux distributions but can still be disabled. The +installation script automatically determines whether this option is +available on your system (@pxref{Binary Installation}). + +When using this option, you only need to create one user account, and +@command{guix-daemon} will run with the authority of that account: + +@example +# groupadd --system guix-daemon +# useradd -g guix-daemon -G guix-daemon \ + -d /var/empty -s $(which nologin) \ + -c "Guix daemon privilege separation user" \ + --system guix-daemon +@end example + +In this configuration, @file{/gnu/store} is owned by the +@code{guix-daemon} user. + +@unnumberedsubsubsec The Isolated Build Environment + @cindex chroot -@noindent -This way, the daemon starts build processes in a chroot, under one of -the @code{guixbuilder} users. On GNU/Linux, by default, the chroot -environment contains nothing but: +@cindex build environment isolation +@cindex isolated build environment +@cindex hermetic build environment +In both cases, the daemon starts build processes without privileges in +an @emph{isolated} or @emph{hermetic} build environment---a ``chroot''. +On GNU/Linux, by default, the build environment contains nothing but: @c Keep this list in sync with libstore/build.cc! ----------------------- @itemize @@ -1015,7 +1068,7 @@ Build Environment Setup @file{/homeless-shelter}. This helps to highlight inappropriate uses of @env{HOME} in the build scripts of packages. -All this usually enough to ensure details of the environment do not +All this is usually enough to ensure details of the environment do not influence build processes. In some exceptional cases where more control is needed---typically over the date, kernel, or CPU---you can resort to a virtual build machine (@pxref{build-vm, virtual build machines}). @@ -1035,14 +1088,6 @@ Build Environment Setup for fixed-output derivations (@pxref{Derivations}) or for substitutes (@pxref{Substitutes}). -If you are installing Guix as an unprivileged user, it is still possible -to run @command{guix-daemon} provided you pass @option{--disable-chroot}. -However, build processes will not be isolated from one another, and not -from the rest of the system. Thus, build processes may interfere with -each other, and may access programs, libraries, and other files -available on the system---making it much harder to view them as -@emph{pure} functions. - @node Daemon Offload Setup @subsection Using the Offload Facility @@ -1567,10 +1612,17 @@ Invoking guix-daemon @item --disable-chroot Disable chroot builds. -Using this option is not recommended since, again, it would allow build -processes to gain access to undeclared dependencies. It is necessary, -though, when @command{guix-daemon} is running under an unprivileged user -account. +@quotation Warning +Using this option is not recommended since it allows build processes to +gain access to undeclared dependencies, to interfere with one another, +and more generally to do anything that can be done with the authority of +build users or that of the daemon---which includes at least the ability +to tamper with any file in the store! + +You may find it necessary, though, when support for Linux unprivileged +user namespaces is missing (@pxref{Build Environment Setup}). Use at +your own risk! +@end quotation @item --log-compression=@var{type} Compress build logs according to @var{type}, one of @code{gzip}, diff --git a/guix/substitutes.scm b/guix/substitutes.scm index 7ca55788d5..86b9f5472a 100644 --- a/guix/substitutes.scm +++ b/guix/substitutes.scm @@ -79,7 +79,7 @@ (define %narinfo-cache-directory ;; time, 'guix substitute' is called by guix-daemon as root and stores its ;; cached data in /var/guix/…. However, when invoked from 'guix challenge' ;; as a user, it stores its cache in ~/.cache. - (if (zero? (getuid)) + (if (getenv "_NIX_OPTIONS") ;invoked by guix-daemon (or (and=> (getenv "XDG_CACHE_HOME") (cut string-append <> "/guix/substitute")) (string-append %state-directory "/substitute/cache")) diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index c8b778362a..2261ef3c2a 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -744,6 +744,10 @@ private: friend int childEntry(void *); + /* Pipe to notify readiness to the child process when using unprivileged + user namespaces. */ + Pipe readiness; + /* Check that the derivation outputs all exist and register them as valid. */ void registerOutputs(); @@ -1619,6 +1623,24 @@ int childEntry(void * arg) } +/* UID and GID of the build user inside its own user namespace. */ +static const uid_t guestUID = 30001; +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()) +{ + writeFile("/proc/" + std::to_string(child) + "/uid_map", + (format("%d %d 1") % guestUID % hostUID).str()); + + writeFile("/proc/" + std::to_string(child) + "/setgroups", "deny"); + + writeFile("/proc/" + std::to_string(child) + "/gid_map", + (format("%d %d 1") % guestGID % hostGID).str()); +} + void DerivationGoal::startBuilder() { auto f = format( @@ -1682,7 +1704,7 @@ void DerivationGoal::startBuilder() then an attacker could create in it a hardlink to a root-owned file such as /etc/shadow. If 'keepFailed' is true, the daemon would then chown that hardlink to the user, giving them write access to - that file. */ + that file. See CVE-2021-27851. */ tmpDir += "/top"; if (mkdir(tmpDir.c_str(), 0700) == 1) throw SysError("creating top-level build directory"); @@ -1799,7 +1821,7 @@ void DerivationGoal::startBuilder() if (mkdir(chrootRootDir.c_str(), 0750) == -1) throw SysError(format("cannot create ‘%1%’") % chrootRootDir); - if (chown(chrootRootDir.c_str(), 0, buildUser.getGID()) == -1) + if (buildUser.enabled() && chown(chrootRootDir.c_str(), 0, buildUser.getGID()) == -1) throw SysError(format("cannot change ownership of ‘%1%’") % chrootRootDir); /* Create a writable /tmp in the chroot. Many builders need @@ -1818,8 +1840,8 @@ void DerivationGoal::startBuilder() (format( "nixbld:x:%1%:%2%:Nix build user:/:/noshell\n" "nobody:x:65534:65534:Nobody:/:/noshell\n") - % (buildUser.enabled() ? buildUser.getUID() : getuid()) - % (buildUser.enabled() ? buildUser.getGID() : getgid())).str()); + % (buildUser.enabled() ? buildUser.getUID() : guestUID) + % (buildUser.enabled() ? buildUser.getGID() : guestGID)).str()); /* Declare the build user's group so that programs get a consistent view of the system (e.g., "id -gn"). */ @@ -1854,7 +1876,7 @@ void DerivationGoal::startBuilder() createDirs(chrootStoreDir); chmod_(chrootStoreDir, 01775); - if (chown(chrootStoreDir.c_str(), 0, buildUser.getGID()) == -1) + if (buildUser.enabled() && chown(chrootStoreDir.c_str(), 0, buildUser.getGID()) == -1) throw SysError(format("cannot change ownership of ‘%1%’") % chrootStoreDir); foreach (PathSet::iterator, i, inputPaths) { @@ -1960,14 +1982,36 @@ void DerivationGoal::startBuilder() if (useChroot) { char stack[32 * 1024]; int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD; - if (!fixedOutput) flags |= CLONE_NEWNET; + if (!fixedOutput) { + flags |= CLONE_NEWNET; + } + if (!buildUser.enabled() || getuid() != 0) { + flags |= CLONE_NEWUSER; + readiness.create(); + } + /* Ensure proper alignment on the stack. On aarch64, it has to be 16 bytes. */ - pid = clone(childEntry, + pid = clone(childEntry, (char *)(((uintptr_t)stack + sizeof(stack) - 8) & ~(uintptr_t)0xf), flags, this); - if (pid == -1) - throw SysError("cloning builder process"); + if (pid == -1) { + if ((flags & CLONE_NEWUSER) != 0 && getuid() != 0) + /* 'clone' fails with EPERM on distros where unprivileged user + namespaces are disabled. Error out instead of giving up on + isolation. */ + throw SysError("cannot create process in unprivileged user namespace"); + else + throw SysError("cloning builder process"); + } + + 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); + } + readiness.writeSide.close(); } else #endif { @@ -2013,23 +2057,37 @@ void DerivationGoal::runChild() _writeToStderr = 0; + if (readiness.writeSide >= 0) readiness.writeSide.close(); + + if (readiness.readSide >= 0) { + /* Wait for the parent process to initialize the UID/GID mapping + of our user namespace. */ + char str[20] = { '\0' }; + readFull(readiness.readSide, (unsigned char*)str, 3); + readiness.readSide.close(); + if (strcmp(str, "go\n") != 0) + throw Error("failed to initialize process in unprivileged user namespace"); + } + restoreAffinity(); commonChildInit(builderOut); #if CHROOT_ENABLED if (useChroot) { - /* Initialise the loopback interface. */ - AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP)); - if (fd == -1) throw SysError("cannot open IP socket"); + if (!fixedOutput) { + /* Initialise the loopback interface. */ + AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP)); + if (fd == -1) throw SysError("cannot open IP socket"); - struct ifreq ifr; - strcpy(ifr.ifr_name, "lo"); - ifr.ifr_flags = IFF_UP | IFF_LOOPBACK | IFF_RUNNING; - if (ioctl(fd, SIOCSIFFLAGS, &ifr) == -1) - throw SysError("cannot set loopback interface flags"); + struct ifreq ifr; + strcpy(ifr.ifr_name, "lo"); + ifr.ifr_flags = IFF_UP | IFF_LOOPBACK | IFF_RUNNING; + if (ioctl(fd, SIOCSIFFLAGS, &ifr) == -1) + throw SysError("cannot set loopback interface flags"); - fd.close(); + fd.close(); + } /* Set the hostname etc. to fixed values. */ char hostname[] = "localhost"; @@ -2180,6 +2238,27 @@ void DerivationGoal::runChild() /* Remount root as read-only. */ if (mount("/", "/", 0, MS_BIND | MS_REMOUNT | MS_RDONLY, 0) == -1) throw SysError(format("read-only remount of build root '%1%' failed") % chrootRootDir); + + if (getuid() != 0) { + /* Create a new mount namespace to "lock" previous mounts. + See mount_namespaces(7). */ + auto uid = getuid(); + auto gid = getgid(); + + if (unshare(CLONE_NEWNS | CLONE_NEWUSER) == -1) + throw SysError(format("creating new user and mount namespaces")); + + initializeUserNamespace(getpid(), uid, gid); + + /* Check that mounts within the build environment are "locked" + together and cannot be separated from within the build + environment namespace. Since + umount(2) is documented to fail with EINVAL when attempting + to unmount one of the mounts that are locked together, + check that this is what we get. */ + int ret = umount(tmpDirInSandbox.c_str()); + assert(ret == -1 && errno == EINVAL); + } } #endif @@ -2262,6 +2341,7 @@ void DerivationGoal::runChild() writeFull(STDERR_FILENO, "\n"); /* Execute the program. This should not return. */ + string builderBasename; if (isBuiltin(drv)) { try { logType = ltFlat; @@ -2285,11 +2365,28 @@ 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). + + This attack is possible even if the target of /proc/self/exe is + outside the chroot (it's as if it were a hard link), though it + requires that its ELF interpreter and dependencies be in the + chroot. + + Note: 'canonPath' throws if 'drv.builder' cannot be resolved + within the chroot. */ + builderBasename = baseNameOf(drv.builder); + drv.builder = canonPath(drv.builder, true); + + if (!isInStore(drv.builder)) + throw Error(format("derivation builder '%1%' is outside the store") % drv.builder); + } /* Fill in the arguments. */ Strings args; - string builderBasename = baseNameOf(drv.builder); args.push_back(builderBasename); foreach (Strings::iterator, i, drv.args) args.push_back(rewriteHashes(*i, rewritesToTmp)); @@ -2476,8 +2573,16 @@ void DerivationGoal::registerOutputs() 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) { + if (S_ISDIR(st.st_mode)) + /* Change mode on the directory to allow for + rename(2). */ + chmod(actualPath.c_str(), st.st_mode | 0700); + if (rename(actualPath.c_str(), path.c_str()) == -1) + throw SysError(format("moving build output `%1%' from the chroot to the store") % path); + if (S_ISDIR(st.st_mode) && chmod(path.c_str(), st.st_mode) == -1) + throw SysError(format("restoring permissions on directory `%1%'") % actualPath); + } } if (buildMode != bmCheck) actualPath = path; } @@ -2736,16 +2841,46 @@ void DerivationGoal::deleteTmpDir(bool force) // Change the ownership if clientUid is set. Never change the // ownership or the group to "root" for security reasons. if (settings.clientUid != (uid_t) -1 && settings.clientUid != 0) { - _chown(tmpDir, settings.clientUid, - settings.clientGid != 0 ? settings.clientGid : -1); + uid_t uid = settings.clientUid; + gid_t gid = settings.clientGid != 0 ? settings.clientGid : -1; + bool reown = false; + + /* First remove setuid/setgid bits. */ + secureFilePerms(tmpDir); + + try { + _chown(tmpDir, uid, gid); + + if (getuid() != 0) { + /* If, without being root, the '_chown' call above + succeeded, then it means we have CAP_CHOWN. Retake + ownership of tmpDir itself so it can be renamed + below. */ + reown = true; + } + + } catch (SysError & e) { + /* When running as an unprivileged user and without + CAP_CHOWN, we cannot chown the build tree. Print a + message and keep going. */ + printMsg(lvlInfo, format("cannot change ownership of build directory '%1%': %2%") + % tmpDir % strerror(e.errNo)); + } if (top != tmpDir) { + if (reown) chown(tmpDir.c_str(), getuid(), getgid()); + // Rename tmpDir to its parent, with an intermediate step. string pivot = top + ".pivot"; if (rename(top.c_str(), pivot.c_str()) == -1) throw SysError("pivoting failed build tree"); if (rename((pivot + "/top").c_str(), top.c_str()) == -1) throw SysError("renaming failed build tree"); + + if (reown) + /* Running unprivileged but with CAP_CHOWN. */ + chown(top.c_str(), uid, gid); + rmdir(pivot.c_str()); } } diff --git a/nix/libstore/local-store.cc b/nix/libstore/local-store.cc index 0883a4bbce..83e6c3e16e 100644 --- a/nix/libstore/local-store.cc +++ b/nix/libstore/local-store.cc @@ -1614,11 +1614,19 @@ void LocalStore::createUser(const std::string & userName, uid_t userId) { auto dir = settings.nixStateDir + "/profiles/per-user/" + userName; - createDirs(dir); - if (chmod(dir.c_str(), 0755) == -1) - throw SysError(format("changing permissions of directory '%s'") % dir); - if (chown(dir.c_str(), userId, -1) == -1) - throw SysError(format("changing owner of directory '%s'") % dir); + auto created = createDirs(dir); + if (!created.empty()) { + if (chmod(dir.c_str(), 0755) == -1) + throw SysError(format("changing permissions of directory '%s'") % dir); + + /* The following operation requires CAP_CHOWN or can be handled + manually by a user with CAP_CHOWN. */ + if (chown(dir.c_str(), userId, -1) == -1) { + rmdir(dir.c_str()); + string message = strerror(errno); + printMsg(lvlInfo, format("failed to change owner of directory '%1%' to %2%: %3%") % dir % userId % message); + } + } } diff --git a/tests/derivations.scm b/tests/derivations.scm index 72ea9aa9cc..b0b151dd33 100644 --- a/tests/derivations.scm +++ b/tests/derivations.scm @@ -858,6 +858,20 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all)) (call-with-input-file (derivation->output-path drv) get-string-all)))) +(test-assert "builder is outside the store" + ;; Ensure that attempts to build derivations whose builder is outside the + ;; store are rejected. This is a protection against attacks similar to + ;; CVE-2019-5736, which abuse the fact that /proc/self/exe can be opened + ;; even when it presents itself as a symlink to a file not in the chroot. + (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))) + (define %coreutils (false-if-exception