diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index 23d368bb1a2..a56cbe3c252 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -183,6 +183,19 @@ struct DerivationBuilder : RestrictionContext virtual bool killChild() = 0; }; +/** + * Run a callback that may change process credentials (setuid, setgid, etc.) + * while preserving the parent-death signal. + * + * The parent-death signal setting is cleared by the Linux kernel upon changes + * to EUID, EGID. + * + * @note Does nothing on non-Linux systems. + * @see man PR_SET_PDEATHSIG + * @see https://github.com/golang/go/issues/9686 + */ +void preserveDeathSignal(std::function fn); + struct ExternalBuilder { StringSet systems; diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 958f06e62d9..222521a8ad8 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -26,6 +26,9 @@ #include #include #include +#ifdef __linux__ +# include +#endif #include "store-config-private.hh" @@ -60,6 +63,37 @@ struct NotDeterministic : BuildError } }; +void preserveDeathSignal(std::function fn) +{ +#ifdef __linux__ + /* Record the old parent pid. This is to avoid a race in case the parent + gets killed after setuid, but before we restored the death signal. It is + zero if the parent isn't visible inside the PID namespace. + See: https://stackoverflow.com/questions/284325/how-to-make-child-process-die-after-parent-exits */ + auto parentPid = getppid(); + + int oldDeathSignal; + if (prctl(PR_GET_PDEATHSIG, &oldDeathSignal) == -1) + throw SysError("getting death signal"); + + fn(); /* Invoke the callback that does setuid etc. */ + + /* Set the old death signal. SIGKILL is set by default in startProcess, + but it gets cleared after setuid. Without this we end up with runaway + build processes if we get killed. */ + if (prctl(PR_SET_PDEATHSIG, oldDeathSignal) == -1) + throw SysError("setting death signal"); + + /* The parent got killed and we got reparented. Commit seppuku. This check + doesn't help much with PID namespaces, but it's still useful without + sandboxing. */ + if (oldDeathSignal && getppid() != parentPid) + raise(oldDeathSignal); +#else + fn(); /* Just call the function on non-Linux. */ +#endif +} + /** * This class represents the state for building locally. * @@ -403,8 +437,8 @@ class DerivationBuilderImpl : public DerivationBuilder, public DerivationBuilder /** * Delete the temporary directory, if we have one. * - * @param force We know the build suceeded, so don't attempt to - * preseve anything for debugging. + * @param force We know the build succeeded, so don't attempt to + * preserve anything for debugging. */ virtual void cleanupBuild(bool force); @@ -1344,17 +1378,21 @@ void DerivationBuilderImpl::setUser() setuid() when run as root sets the real, effective and saved UIDs. */ if (buildUser) { - /* Preserve supplementary groups of the build user, to allow - admins to specify groups such as "kvm". */ - auto gids = buildUser->getSupplementaryGIDs(); - if (setgroups(gids.size(), gids.data()) == -1) - throw SysError("cannot set supplementary groups of build user"); - - if (setgid(buildUser->getGID()) == -1 || getgid() != buildUser->getGID() || getegid() != buildUser->getGID()) - throw SysError("setgid failed"); - - if (setuid(buildUser->getUID()) == -1 || getuid() != buildUser->getUID() || geteuid() != buildUser->getUID()) - throw SysError("setuid failed"); + preserveDeathSignal([this]() { + /* Preserve supplementary groups of the build user, to allow + admins to specify groups such as "kvm". */ + auto gids = buildUser->getSupplementaryGIDs(); + if (setgroups(gids.size(), gids.data()) == -1) + throw SysError("cannot set supplementary groups of build user"); + + if (setgid(buildUser->getGID()) == -1 || getgid() != buildUser->getGID() + || getegid() != buildUser->getGID()) + throw SysError("setgid failed"); + + if (setuid(buildUser->getUID()) == -1 || getuid() != buildUser->getUID() + || geteuid() != buildUser->getUID()) + throw SysError("setuid failed"); + }); } } diff --git a/src/libstore/unix/build/linux-derivation-builder.cc b/src/libstore/unix/build/linux-derivation-builder.cc index 4af88bb1548..5dc61eb57c4 100644 --- a/src/libstore/unix/build/linux-derivation-builder.cc +++ b/src/libstore/unix/build/linux-derivation-builder.cc @@ -690,13 +690,15 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu void setUser() override { - /* Switch to the sandbox uid/gid in the user namespace, - which corresponds to the build user or calling user in - the parent namespace. */ - if (setgid(sandboxGid()) == -1) - throw SysError("setgid failed"); - if (setuid(sandboxUid()) == -1) - throw SysError("setuid failed"); + preserveDeathSignal([this]() { + /* Switch to the sandbox uid/gid in the user namespace, + which corresponds to the build user or calling user in + the parent namespace. */ + if (setgid(sandboxGid()) == -1) + throw SysError("setgid failed"); + if (setuid(sandboxUid()) == -1) + throw SysError("setuid failed"); + }); } SingleDrvOutputs unprepareBuild() override