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

> Hello Guix!
>
> Changes in v4, hopefully the last revision of this patch set:
>
>   • For ‘deleteTmpDir’, go back to v2, but add ‘secureFilePerms’ call and
>     define ‘reown’ variable to determine whether to re-chown after pivoting
>     (suggested by Reepca).

After re-reading the v4 patch for this I've noticed one minor nitpick:
since it's technically possible (though unlikely) to both have CAP_CHOWN
and have (top == tmpdir), for example if --disable-chroot is given, it
is possible that it will unnecessarily chown tmpDir and then never
re-chown it back.

The diff in question, for clarity:

> @@ -2736,8 +2798,32 @@ 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.  */
> +			chown(tmpDir.c_str(), getuid(), getgid());
> +			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) {
>  		    // Rename tmpDir to its parent, with an intermediate step.
> @@ -2746,6 +2832,11 @@ void DerivationGoal::deleteTmpDir(bool force)
>  			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());
>  		}
>              }

This can be remedied by moving

chown(tmpDir.c_str(), getuid(), getgid());

to inside the

if (top != tmpDir)

block, and adding a test for 'reown', like so:

if (top != tmpDir) {
    if (reown) chown(tmpDir.c_str(), getuid(), getgid());
    // Rename tmpDir to its parent, with an intermediate step.
    ...
}

The extra symmetry should also make this section a bit clearer overall.


> The tests try to MS_REMOUNT the inputs, which is exactly what we want to
> prevent; we could test the low-level semantics you describe, but it’s
> quite obscure and maybe unnecessary given that we test MS_REMOUNT?

My concern is that it may be possible, now or in the future, for the
builder to gain the necessary capability within its user
namespace... somehow.  This concern comes from reading the
capabilities(7) manual page, where it says:

   Per-user-namespace "set-user-ID-root" programs
       A  set-user-ID  program  whose  UID matches the UID that created a user
       namespace will confer capabilities in the process's permitted  and  ef‐
       fective  sets when executed by any process inside that namespace or any
       descendant user namespace.

       The rules about the transformation of the process's capabilities during
       the  execve(2)  are exactly as described in Transformation of capabili‐
       ties during execve() and Capabilities and execution of programs by root
       above,  with  the  difference that, in the latter subsection, "root" is
       the UID of the creator of the user namespace.

Even with no effective capabilities whatsoever, nothing is stopping root
from making a setuid program and executing it, and I don't see what
would stop the builder from doing likewise.  If it works as described
("whose UID matches the UID that created a user namespace"), this should
cause the builder to gain full capabilities within its user namespace.

Now, experimentally, this doesn't /seem/ to work as described, but if
it's in the manual, it may be unwise to bet against it ever happening.
Additionally, even if it never is implemented as described, this text's
presence makes it less clear how security within a user namespace (not
just between user namespaces) is intended to work.  That's why I would
like for the security against remounting to not depend on the
capabilities that the builder has in its user namespace.

- reepca