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

> Hello!
>
> Here’s an updated version, addressing most issues brought up
> by Reepca, also available from
> <https://codeberg.org/civodul/guix/src/branch/wip-rootless-daemon>.
> Main changes compared to v2:
>
>   • Derivation inputs and / are mounted read-only; additional
>     tests check the ability to write to these, to /tmp, to
>     /dev/{full,null}, and to remount any of these as read-write.
>
>   • Unit files for systemd tweaked so that (1) guix-daemon sees
>     a private read-write mount of the store, and (2) gnu-store.mount
>     actually remounts the store read-only after guix-daemon has
>     started.

I'm not familiar with how systemd does service dependencies, but does
this mean that the store becomes writable when the daemon is stopped?

>
>   • ‘DerivationGoal::deleteTmpDir’ bails out when it fails to
>     chown ‘tmpDir’ (i.e., it does not try to “pivot” the /top
>     sub-directory).
>
> Did I forget anything, Reepca?

I believe that if you try a "--keep-failed" build that fails in the
CAP_CHOWN case, you'll find that only root or the guix-daemon user can
delete the kept build directory, though the user that started the build
can delete everything inside it.  This is because in that case the build
directory was chown'ed back to guix-daemon so that it could be moved,
but wasn't chown'ed to the client user afterward.  If I recall correctly
there was code included to perform this extra chown in the (getuid() !=
0) case in the v2 series - was it accidentally forgotten?

Also, there are potential issues with how wide the scope of the try
block in DerivationGoal::deleteTmpDir is - _chown isn't the only place
within it that can raise a SysError, and there are failure modes present
that may merit more user attention than lvlInfo.  For example, if

rename((pivot + "/top").c_str(), top.c_str())

fails (which can be rather easily arranged by a local attacker), then
the build directory path reported in the "note: keeping build directory"
message remains up for grabs by anyone.  If the user doesn't go out of
their way to verify that the build directory isn't attacker-controlled,
they could be rather easily tricked into executing malicious code.  But
currently the exception from this rename failing will be turned into a
lvlInfo message, and I'm not sure how that interacts with the verbosity
defaults in the various CLI programs.

This does somewhat raise the question of why we're even doing the
pivoting in a way that creates a window during which failure can be
induced.  For example, we could move the inner build directory to the
pivot path, at which point the outer build directory should become
empty, so it should work to then rename the pivot path to the outer
build directory path, thereby atomically replacing it.

Also, in the unprivileged case (non-root, no CAP_CHOWN), the build
directory never gets pivoted out.  This is better for security than the
previous situation (which allowed setuid programs to be exposed), but it
should be quite doable to simply secure the file permissions first and
then carry on with the pivot.  I believe I previously mentioned perhaps
using secureFilePerms to do this?

It may work well to use the v2 patch for this with a call to
secureFilePerms added right before the try block and a have_cap_chown
boolean flag being saved for later recall after the pivot instead of the
(getuid() != 0) check.  That way in the fully-unprivileged case it
doesn't successfully pivot the now-sanitized build directory only to
immediately fail to chown it.  Actually, because that chown call doesn't
result in an exception on failure, it would also work to only add the
secureFilePerms call.


Also, I've discovered that while mount(2) uses EPERM for both a locked
mount point and insufficient privileges, umount(2) uses EINVAL for the
former and EPERM for the latter.  This may be a good way to test that
we're triggering the mount-locking behavior as intended.

> The one observable difference compared to current guix-daemon
> operational mode is that, in the build environment, writing to
> the root file system results in EROFS instead of EPERM, as you
> pointed out earlier.  That’s not great but probably acceptable.
> We’ll only know whether this is a problem in practice once we’ve
> run the test suites of tens of thousands of packages.

Strictly speaking, it's also observable that the root file system,
store, /tmp, etc is not owned by uid 0, and that the input store items
are all mounted read-only.

- reepca