[bug#75810] [PATCH v4 06/14] daemon: Allow running as non-root with unprivileged user namespaces.

[Maxim Cournoyer]

Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

> From: Ludovic Courtès <ludovic.courtes@inria.fr>
>
> * 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.

That's a nice improvement!

[...]

> +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 option only appeared recently.
> +@end enumerate

Similarly to what Simon pointed in their comments, I'd drop time-related
'recently' wording, as it won't age well, and is already made obvious by
the above being mentioned as the 'historical' approach.

> +
> +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 +994,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 option, which is new, is to run @command{guix-daemon}

s/, which is new,// as Simon pointed.

[...]

>  void DerivationGoal::startBuilder()
>  {
>      auto f = format(
> @@ -1682,7 +1705,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 +1822,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 +1841,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 +1877,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);

I think adding the new check for buildUser.enabled() in the above ifs
should be split into a distinct commit since it's not relevant to this
specific new feature.

[...]

>  #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();
> +         }

That hunk above is also orthogonal to this feature AFAICS, should be
split into a different commit to keep its diff focused.

The rest LGTM.  C++ is not that hard to parse after all; it seems the
daemon is written in a style close to that of C.

Reviewed-by: Maxim Cournoyer <maxim.cournoyer@gmail>

-- 
Thanks,
Maxim