diff --git a/.gitignore b/.gitignore index 8a7e99d06..5dc9af7fc 100644 --- a/.gitignore +++ b/.gitignore @@ -100,9 +100,6 @@ Makefile.in /src/nix-log2xml/*.xml /src/nix-log2xml/*.html -# /src/nix-setuid-helper/ -/src/nix-setuid-helper/nix-setuid-helper - # /src/nix-store/ /src/nix-store/nix-store diff --git a/configure.ac b/configure.ac index abf4136a9..7acd57557 100644 --- a/configure.ac +++ b/configure.ac @@ -375,7 +375,6 @@ AC_CONFIG_FILES([Makefile src/nix-instantiate/Makefile src/nix-env/Makefile src/nix-daemon/Makefile - src/nix-setuid-helper/Makefile src/nix-log2xml/Makefile src/bsdiff-4.3/Makefile perl/Makefile diff --git a/doc/manual/installation.xml b/doc/manual/installation.xml index 9d1a7e755..a136d3b11 100644 --- a/doc/manual/installation.xml +++ b/doc/manual/installation.xml @@ -380,7 +380,7 @@ group should be the build users group, and it should have the sticky bit turned on (like /tmp): -$ chgrp nixbld /nix/store +$ chown root.nixbld /nix/store $ chmod 1775 /nix/store @@ -401,15 +401,7 @@ build-users-group = nixbld -
Nix store/database owned by root - -The simplest setup is to let root own the Nix -store and database. I.e., - - -$ chown -R root /nix/store /nix/var/nix - - +
Running the daemon The Nix daemon should be started as follows (as root): @@ -433,72 +425,6 @@ into the users’ login scripts.
-
Nix store/database not owned by root - -It is also possible to let the Nix store and database be owned -by a non-root user, which should be more secureNote -however that even when the Nix daemon runs as root, not -that much code is executed as root: Nix -expression evaluation is performed by the calling (unprivileged) user, -and builds are performed under the special build user accounts. So -only the code that accesses the database and starts builds is executed -as root.. Typically, this user -is a special account called nix, but it can be -named anything. It should own the Nix store and database: - - -$ chown -R nix /nix/store /nix/var/nix - -and of course nix-daemon should be started under -that user, e.g., - - -$ su - nix -c "exec /nix/bin/nix-daemon" - - - -There is a catch, though: non-root users -cannot start builds under the build user accounts, since the -setuid system call is obviously privileged. To -allow a non-root Nix daemon to use the build user -feature, it calls a setuid-root helper program, -nix-setuid-helper. This program is installed in -prefix/libexec/nix-setuid-helper. -To set the permissions properly (Nix’s make install -doesn’t do this, since we don’t want to ship setuid-root programs -out-of-the-box): - - -$ chown root.root /nix/libexec/nix-setuid-helper -$ chmod 4755 /nix/libexec/nix-setuid-helper - - -(This example assumes that the Nix binaries are installed in -/nix.) - -Of course, the nix-setuid-helper command -should not be usable by just anybody, since then anybody could run -commands under the Nix build user accounts. For that reason there is -a configuration file /etc/nix-setuid.conf that -restricts the use of the helper. This file should be a text file -containing precisely two lines, the first being the Nix daemon user -and the second being the build users group, e.g., - - -nix -nixbld - - -The setuid-helper barfs if it is called by a user other than the one -specified on the first line, or if it is asked to execute a build -under a user who is not a member of the group specified on the second -line. The file /etc/nix-setuid.conf must be -owned by root, and must not be group- or world-writable. The -setuid-helper barfs if this is not the case. - -
- -
Restricting access To limit which users can perform Nix operations, you can use the diff --git a/doc/manual/release-notes.xml b/doc/manual/release-notes.xml index 5d057881d..3db083870 100644 --- a/doc/manual/release-notes.xml +++ b/doc/manual/release-notes.xml @@ -5,6 +5,22 @@ Nix Release Notes + + +
Release 1.7 (TBA) + +This release has the following changes: + + + + nix-setuid-helper is + gone. + + + +
+ +
Release 1.6.1 (October 28, 2013) diff --git a/src/Makefile.am b/src/Makefile.am index 0ae407c57..a5e411b9e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,3 +1,3 @@ SUBDIRS = boost libutil libstore libmain nix-store nix-hash \ - libexpr nix-instantiate nix-env nix-daemon nix-setuid-helper \ + libexpr nix-instantiate nix-env nix-daemon \ nix-log2xml bsdiff-4.3 diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 51314f736..63e34d256 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -549,93 +549,10 @@ void UserLock::release() } -static void runSetuidHelper(const string & command, - const string & arg) -{ - Path program = getEnv("NIX_SETUID_HELPER", - settings.nixLibexecDir + "/nix-setuid-helper"); - - /* Fork. */ - Pid pid; - pid = fork(); - switch (pid) { - - case -1: - throw SysError("unable to fork"); - - case 0: /* child */ - try { - std::vector args; /* careful with c_str()! */ - args.push_back(program.c_str()); - args.push_back(command.c_str()); - args.push_back(arg.c_str()); - args.push_back(0); - - restoreSIGPIPE(); - restoreAffinity(); - - execve(program.c_str(), (char * *) &args[0], 0); - throw SysError(format("executing `%1%'") % program); - } - catch (std::exception & e) { - writeToStderr("error: " + string(e.what()) + "\n"); - } - _exit(1); - } - - /* Parent. */ - - /* Wait for the child to finish. */ - int status = pid.wait(true); - if (!statusOk(status)) - throw Error(format("program `%1%' %2%") - % program % statusToString(status)); -} - - void UserLock::kill() { assert(enabled()); - if (amPrivileged()) - killUser(uid); - else - runSetuidHelper("kill", user); -} - - -bool amPrivileged() -{ - return geteuid() == 0; -} - - -void getOwnership(const Path & path) -{ - runSetuidHelper("get-ownership", path); -} - - -void deletePathWrapped(const Path & path, unsigned long long & bytesFreed) -{ - try { - /* First try to delete it ourselves. */ - deletePath(path, bytesFreed); - } catch (SysError & e) { - /* If this failed due to a permission error, then try it with - the setuid helper. */ - if (settings.buildUsersGroup != "" && !amPrivileged()) { - getOwnership(path); - deletePath(path, bytesFreed); - } else - throw; - } -} - - -void deletePathWrapped(const Path & path) -{ - unsigned long long dummy1; - deletePathWrapped(path, dummy1); + killUser(uid); } @@ -971,15 +888,11 @@ void DerivationGoal::killChild() worker.childTerminated(pid); if (buildUser.enabled()) { - /* We can't use pid.kill(), since we may not have the - appropriate privilege. I.e., if we're not root, then - setuid helper should do it). - - Also, if we're using a build user, then there is a - tricky race condition: if we kill the build user before - the child has done its setuid() to the build user uid, - then it won't be killed, and we'll potentially lock up - in pid.wait(). So also send a conventional kill to the + /* If we're using a build user, then there is a tricky + race condition: if we kill the build user before the + child has done its setuid() to the build user uid, then + it won't be killed, and we'll potentially lock up in + pid.wait(). So also send a conventional kill to the child. */ ::kill(-pid, SIGKILL); /* ignore the result */ buildUser.kill(); @@ -1349,7 +1262,7 @@ void DerivationGoal::tryToBuild() if (worker.store.isValidPath(path)) continue; if (!pathExists(path)) continue; debug(format("removing unregistered path `%1%'") % path); - deletePathWrapped(path); + deletePath(path); } /* Check again whether any output previously failed to build, @@ -1427,7 +1340,7 @@ void replaceValidPath(const Path & storePath, const Path tmpPath) if (rename(tmpPath.c_str(), storePath.c_str()) == -1) throw SysError(format("moving `%1%' to `%2%'") % tmpPath % storePath); if (pathExists(oldPath)) - deletePathWrapped(oldPath); + deletePath(oldPath); } @@ -1532,13 +1445,6 @@ void DerivationGoal::buildDone() rewrittenPaths.insert(path); } - - /* Gain ownership of the build result using the setuid - wrapper if we're not root. If we *are* root, then - canonicalisePathMetaData() will take care of this later - on. */ - if (buildUser.enabled() && !amPrivileged()) - getOwnership(path); } /* Check the exit status. */ @@ -1846,13 +1752,9 @@ void DerivationGoal::startBuilder() uid. */ buildUser.kill(); - /* Change ownership of the temporary build directory, if we're - root. If we're not root, then the setuid helper will do it - just before it starts the builder. */ - if (amPrivileged()) { - if (chown(tmpDir.c_str(), buildUser.getUID(), buildUser.getGID()) == -1) - throw SysError(format("cannot change ownership of `%1%'") % tmpDir); - } + /* Change ownership of the temporary build directory. */ + if (chown(tmpDir.c_str(), buildUser.getUID(), buildUser.getGID()) == -1) + throw SysError(format("cannot change ownership of `%1%'") % tmpDir); /* Check that the Nix store has the appropriate permissions, i.e., owned by root and mode 1775 (sticky bit on so that @@ -2212,30 +2114,18 @@ void DerivationGoal::initChild() if (buildUser.enabled()) { printMsg(lvlChatty, format("switching to user `%1%'") % buildUser.getUser()); - if (amPrivileged()) { + if (setgroups(0, 0) == -1) + throw SysError("cannot clear the set of supplementary groups"); - if (setgroups(0, 0) == -1) - throw SysError("cannot clear the set of supplementary groups"); + if (setgid(buildUser.getGID()) == -1 || + getgid() != buildUser.getGID() || + getegid() != buildUser.getGID()) + throw SysError("setgid failed"); - 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"); - - } else { - /* Let the setuid helper take care of it. */ - program = settings.nixLibexecDir + "/nix-setuid-helper"; - args.push_back(program.c_str()); - args.push_back("run-builder"); - user = buildUser.getUser().c_str(); - args.push_back(user.c_str()); - args.push_back(drv.builder.c_str()); - } + if (setuid(buildUser.getUID()) == -1 || + getuid() != buildUser.getUID() || + geteuid() != buildUser.getUID()) + throw SysError("setuid failed"); } /* Fill in the arguments. */ @@ -2466,12 +2356,10 @@ void DerivationGoal::deleteTmpDir(bool force) printMsg(lvlError, format("note: keeping build directory `%2%'") % drvPath % tmpDir); - if (buildUser.enabled() && !amPrivileged()) - getOwnership(tmpDir); chmod(tmpDir.c_str(), 0755); } else - deletePathWrapped(tmpDir); + deletePath(tmpDir); tmpDir = ""; } } @@ -2548,7 +2436,7 @@ Path DerivationGoal::addHashRewrite(const Path & path) string h1 = string(path, settings.nixStore.size() + 1, 32); string h2 = string(printHash32(hashString(htSHA256, "rewrite:" + drvPath + ":" + path)), 0, 32); Path p = settings.nixStore + "/" + h2 + string(path, settings.nixStore.size() + 33); - if (pathExists(p)) deletePathWrapped(p); + if (pathExists(p)) deletePath(p); assert(path.size() == p.size()); rewritesToTmp[h1] = h2; rewritesFromTmp[h2] = h1; @@ -2639,9 +2527,6 @@ SubstitutionGoal::SubstitutionGoal(const Path & storePath, Worker & worker, bool SubstitutionGoal::~SubstitutionGoal() { - /* !!! Once we let substitution goals run under a build user, we - need to use the setuid helper just as in ~DerivationGoal(). - Idem for cancel. */ if (pid != -1) worker.childTerminated(pid); } @@ -2792,7 +2677,7 @@ void SubstitutionGoal::tryToRun() /* Remove the (stale) output path if it exists. */ if (pathExists(destPath)) - deletePathWrapped(destPath); + deletePath(destPath); worker.store.setSubstituterEnv(); diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index f32329960..ce9b10f05 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -415,7 +415,7 @@ bool LocalStore::isActiveTempFile(const GCState & state, void LocalStore::deleteGarbage(GCState & state, const Path & path) { unsigned long long bytesFreed; - deletePathWrapped(path, bytesFreed); + deletePath(path, bytesFreed); state.results.bytesFreed += bytesFreed; } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 24d6acc2f..8aa113796 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1328,7 +1328,7 @@ Path LocalStore::addToStoreFromDump(const string & dump, const string & name, if (repair || !isValidPath(dstPath)) { - if (pathExists(dstPath)) deletePathWrapped(dstPath); + if (pathExists(dstPath)) deletePath(dstPath); if (recursive) { StringSource source(dump); @@ -1397,7 +1397,7 @@ Path LocalStore::addTextToStore(const string & name, const string & s, if (repair || !isValidPath(dstPath)) { - if (pathExists(dstPath)) deletePathWrapped(dstPath); + if (pathExists(dstPath)) deletePath(dstPath); writeFile(dstPath, s); @@ -1630,7 +1630,7 @@ Path LocalStore::importPath(bool requireSignature, Source & source) if (!isValidPath(dstPath)) { - if (pathExists(dstPath)) deletePathWrapped(dstPath); + if (pathExists(dstPath)) deletePath(dstPath); if (rename(unpacked.c_str(), dstPath.c_str()) == -1) throw SysError(format("cannot move `%1%' to `%2%'") diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index adba52fd4..1ace7cec9 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -322,7 +322,7 @@ typedef set InodesSeen; - the permissions are set of 444 or 555 (i.e., read-only with or without execute permission; setuid bits etc. are cleared) - the owner and group are set to the Nix user and group, if we're - in a setuid Nix installation. */ + running as root. */ void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & inodesSeen); void canonicalisePathMetaData(const Path & path, uid_t fromUid); @@ -330,16 +330,4 @@ void canonicaliseTimestampAndPermissions(const Path & path); MakeError(PathInUse, Error); -/* Whether we are root. */ -bool amPrivileged(); - -/* Recursively change the ownership of `path' to the current uid. */ -void getOwnership(const Path & path); - -/* Like deletePath(), but changes the ownership of `path' using the - setuid wrapper if necessary (and possible). */ -void deletePathWrapped(const Path & path, unsigned long long & bytesFreed); - -void deletePathWrapped(const Path & path); - } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 8fd61826f..9437a2f99 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -968,19 +968,6 @@ void closeOnExec(int fd) } -void setuidCleanup() -{ - /* Don't trust the environment. */ - environ = 0; - - /* Make sure that file descriptors 0, 1, 2 are open. */ - for (int fd = 0; fd <= 2; ++fd) { - struct stat st; - if (fstat(fd, &st) == -1) abort(); - } -} - - #if HAVE_VFORK pid_t (*maybeVfork)() = vfork; #else diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 86c65b763..2335cbf98 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -268,10 +268,6 @@ void closeMostFDs(const set & exceptions); /* Set the close-on-exec flag for the given file descriptor. */ void closeOnExec(int fd); -/* Common initialisation for setuid programs: clear the environment, - sanitize file handles 0, 1 and 2. */ -void setuidCleanup(); - /* Call vfork() if available, otherwise fork(). */ extern pid_t (*maybeVfork)(); diff --git a/src/nix-setuid-helper/Makefile.am b/src/nix-setuid-helper/Makefile.am deleted file mode 100644 index a04701636..000000000 --- a/src/nix-setuid-helper/Makefile.am +++ /dev/null @@ -1,7 +0,0 @@ -libexec_PROGRAMS = nix-setuid-helper - -nix_setuid_helper_SOURCES = nix-setuid-helper.cc -nix_setuid_helper_LDADD = ../libutil/libutil.la \ - ../boost/format/libformat.la - -AM_CXXFLAGS = -I$(srcdir)/.. -I$(srcdir)/../libutil diff --git a/src/nix-setuid-helper/nix-setuid-helper.cc b/src/nix-setuid-helper/nix-setuid-helper.cc deleted file mode 100644 index 697964088..000000000 --- a/src/nix-setuid-helper/nix-setuid-helper.cc +++ /dev/null @@ -1,263 +0,0 @@ -#include "config.h" - -#include -#include -#include -#include -#include - -#include -#include - -#include -#include - -#include "util.hh" - -using namespace nix; - - -extern char * * environ; - - -/* Recursively change the ownership of `path' to user `uidTo' and - group `gidTo'. `path' must currently be owned by user `uidFrom', - or, if `uidFrom' is -1, by group `gidFrom'. */ -static void secureChown(uid_t uidFrom, gid_t gidFrom, - uid_t uidTo, gid_t gidTo, const Path & path) -{ - format error = format("cannot change ownership of `%1%'") % path; - - struct stat st; - if (lstat(path.c_str(), &st) == -1) - /* Important: don't give any detailed error messages here. - Otherwise, the Nix account can discover information about - the existence of paths that it doesn't normally have access - to. */ - throw Error(error); - - if (uidFrom != (uid_t) -1) { - assert(uidFrom != 0); - if (st.st_uid != uidFrom) - throw Error(error); - } else { - assert(gidFrom != 0); - if (st.st_gid != gidFrom) - throw Error(error); - } - - assert(uidTo != 0 && gidTo != 0); - -#if HAVE_LCHOWN - if (lchown(path.c_str(), uidTo, gidTo) == -1) - throw Error(error); -#else - if (!S_ISLNK(st.st_mode) && - chown(path.c_str(), uidTo, gidTo) == -1) - throw Error(error); -#endif - - if (S_ISDIR(st.st_mode)) { - Strings names = readDirectory(path); - for (Strings::iterator i = names.begin(); i != names.end(); ++i) - /* !!! recursion; check stack depth */ - secureChown(uidFrom, gidFrom, uidTo, gidTo, path + "/" + *i); - } -} - - -static uid_t nameToUid(const string & userName) -{ - struct passwd * pw = getpwnam(userName.c_str()); - if (!pw) - throw Error(format("user `%1%' does not exist") % userName); - return pw->pw_uid; -} - - -static void checkIfBuildUser(const StringSet & buildUsers, - const string & userName) -{ - if (buildUsers.find(userName) == buildUsers.end()) - throw Error(format("user `%1%' is not a member of the build users group") - % userName); -} - - -/* Run `program' under user account `targetUser'. `targetUser' should - be a member of `buildUsersGroup'. The ownership of the current - directory is changed from the Nix user (uidNix) to the target - user. */ -static void runBuilder(uid_t uidNix, gid_t gidBuildUsers, - const StringSet & buildUsers, const string & targetUser, - string program, int argc, char * * argv, char * * env) -{ - uid_t uidTargetUser = nameToUid(targetUser); - - /* Sanity check. */ - if (uidTargetUser == 0) - throw Error("won't setuid to root"); - - /* Verify that the target user is a member of the build users - group. */ - checkIfBuildUser(buildUsers, targetUser); - - /* Chown the current directory, *if* it is owned by the Nix - account. The idea is that the current directory is the - temporary build directory in /tmp or somewhere else, and we - don't want to create that directory here. */ - secureChown(uidNix, (gid_t) -1, uidTargetUser, gidBuildUsers, "."); - - /* Set the real, effective and saved gid. Must be done before - setuid(), otherwise it won't set the real and saved gids. */ - if (setgroups(0, 0) == -1) - throw SysError("cannot clear the set of supplementary groups"); - - if (setgid(gidBuildUsers) == -1 || - getgid() != gidBuildUsers || - getegid() != gidBuildUsers) - throw SysError("setgid failed"); - - /* Set the real, effective and saved uid. */ - if (setuid(uidTargetUser) == -1 || - getuid() != uidTargetUser || - geteuid() != uidTargetUser) - throw SysError("setuid failed"); - - /* Execute the program. */ - std::vector args; - for (int i = 0; i < argc; ++i) - args.push_back(argv[i]); - args.push_back(0); - - environ = env; - - /* Glibc clears TMPDIR in setuid programs (see - sysdeps/generic/unsecvars.h in the Glibc sources), so bring it - back. */ - setenv("TMPDIR", getenv("NIX_BUILD_TOP"), 1); - - if (execv(program.c_str(), (char * *) &args[0]) == -1) - throw SysError(format("cannot execute `%1%'") % program); -} - - -void killBuildUser(gid_t gidBuildUsers, - const StringSet & buildUsers, const string & userName) -{ - uid_t uid = nameToUid(userName); - - /* Verify that the user whose processes we are to kill is a member - of the build users group. */ - checkIfBuildUser(buildUsers, userName); - - assert(uid != 0); - - killUser(uid); -} - - -#ifndef NIX_SETUID_CONFIG_FILE -#define NIX_SETUID_CONFIG_FILE "/etc/nix-setuid.conf" -#endif - - -static void run(int argc, char * * argv) -{ - char * * oldEnviron = environ; - - setuidCleanup(); - - if (geteuid() != 0) - throw Error("nix-setuid-wrapper must be setuid root"); - - - /* Read the configuration file. It should consist of two words: - - - - The first is the privileged account under which the main Nix - processes run (i.e., the supposed caller). It should match our - real uid. The second is the Unix group to which the Nix - builders belong (and nothing else!). */ - string configFile = NIX_SETUID_CONFIG_FILE; - AutoCloseFD fdConfig = open(configFile.c_str(), O_RDONLY); - if (fdConfig == -1) - throw SysError(format("opening `%1%'") % configFile); - - /* Config file should be owned by root. */ - struct stat st; - if (fstat(fdConfig, &st) == -1) throw SysError("statting file"); - if (st.st_uid != 0) - throw Error(format("`%1%' not owned by root") % configFile); - if (st.st_mode & (S_IWGRP | S_IWOTH)) - throw Error(format("`%1%' should not be group or world-writable") % configFile); - - vector tokens = tokenizeString >(readFile(fdConfig)); - - fdConfig.close(); - - if (tokens.size() != 2) - throw Error(format("parse error in `%1%'") % configFile); - - string nixUser = tokens[0]; - string buildUsersGroup = tokens[1]; - - - /* Check that the caller (real uid) is the one allowed to call - this program. */ - uid_t uidNix = nameToUid(nixUser); - if (uidNix != getuid()) - throw Error("you are not allowed to call this program, go away"); - - - /* Get the gid and members of buildUsersGroup. */ - struct group * gr = getgrnam(buildUsersGroup.c_str()); - if (!gr) - throw Error(format("group `%1%' does not exist") % buildUsersGroup); - gid_t gidBuildUsers = gr->gr_gid; - - StringSet buildUsers; - for (char * * p = gr->gr_mem; *p; ++p) - buildUsers.insert(*p); - - - /* Perform the desired command. */ - if (argc < 2) - throw Error("invalid arguments"); - - string command(argv[1]); - - if (command == "run-builder") { - /* Syntax: nix-setuid-helper run-builder - */ - if (argc < 4) throw Error("missing user name / program name"); - runBuilder(uidNix, gidBuildUsers, buildUsers, - argv[2], argv[3], argc - 4, argv + 4, oldEnviron); - } - - else if (command == "get-ownership") { - /* Syntax: nix-setuid-helper get-ownership */ - if (argc != 3) throw Error("missing path"); - secureChown((uid_t) -1, gidBuildUsers, uidNix, gidBuildUsers, argv[2]); - } - - else if (command == "kill") { - /* Syntax: nix-setuid-helper kill */ - if (argc != 3) throw Error("missing user name"); - killBuildUser(gidBuildUsers, buildUsers, argv[2]); - } - - else throw Error ("invalid command"); -} - - -int main(int argc, char * * argv) -{ - try { - run(argc, argv); - } catch (Error & e) { - std::cerr << e.msg() << std::endl; - return 1; - } -}