From 92d599c6a7e7d197fa41167967860628b0f51e60 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 20 Oct 2005 16:58:34 +0000 Subject: [PATCH] * Prevent uids from being used for more than one build simultaneously. We do this using exclusive locks on uid files in /nix/var/nix/userpool, e.g., /nix/var/nix/userpool/123 for uid 123. --- Makefile.am | 3 +- src/libstore/build.cc | 156 ++++++++++++++++++++++++++++++++---------- 2 files changed, 122 insertions(+), 37 deletions(-) diff --git a/Makefile.am b/Makefile.am index f62b2d20f3..d2f483f114 100644 --- a/Makefile.am +++ b/Makefile.am @@ -36,7 +36,8 @@ init-state: $(INSTALL) $(INIT_FLAGS) $(GROUP_WRITABLE) -d $(DESTDIR)$(localstatedir)/nix/gcroots/channels rm -f $(DESTDIR)$(localstatedir)/nix/gcroots/profiles ln -s $(localstatedir)/nix/profiles $(DESTDIR)$(localstatedir)/nix/gcroots/profiles - $(INSTALL) $(INIT_FLAGS) -d $(DESTDIR)$(prefix)/store + $(INSTALL) $(INIT_FLAGS) -d $(DESTDIR)$(localstatedir)/nix/userpool + $(INSTALL) $(INIT_FLAGS) -m 1777 -d $(DESTDIR)$(prefix)/store $(INSTALL) $(INIT_FLAGS) $(GROUP_WRITABLE) -d $(DESTDIR)$(localstatedir)/nix/manifests # $(bindir)/nix-store --init else diff --git a/src/libstore/build.cc b/src/libstore/build.cc index fb3b51c4fc..e4ff1efd3e 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -312,6 +312,110 @@ const char * * strings2CharPtrs(const Strings & ss) } + +////////////////////////////////////////////////////////////////////// + + +class UserLock +{ +private: + /* POSIX locks suck. If we have a lock on a file, and we open and + close that file again (without closing the original file + descriptor), we lose the lock. So we have to be *very* careful + not to open a lock file on which we are holding a lock. */ + static PathSet lockedPaths; /* !!! not thread-safe */ + + Path fnUserLock; + AutoCloseFD fdUserLock; + + uid_t uid; + +public: + UserLock(); + ~UserLock(); + + void acquire(); + void release(); + + uid_t getUID(); +}; + + +PathSet UserLock::lockedPaths; + + +UserLock::UserLock() +{ + uid = 0; +} + + +UserLock::~UserLock() +{ + release(); +} + + +void UserLock::acquire() +{ + assert(uid == 0); + + Strings buildUsers = querySetting("build-users", Strings()); + + if (buildUsers.empty()) + throw Error( + "cannot build as `root'; you must define " + "one or more users for building in `build-users' " + "in the Nix configuration file"); + + for (Strings::iterator i = buildUsers.begin(); i != buildUsers.end(); ++i) { + printMsg(lvlError, format("trying user `%1%'") % *i); + + struct passwd * pw = getpwnam(i->c_str()); + if (!pw) + throw Error(format("the user `%1%' listed in `build-users' does not exist") % *i); + + fnUserLock = (format("%1%/userpool/%2%") % nixStateDir % pw->pw_uid).str(); + + if (lockedPaths.find(fnUserLock) != lockedPaths.end()) + /* We already have a lock on this one. */ + continue; + + AutoCloseFD fd = open(fnUserLock.c_str(), O_RDWR | O_CREAT, 0600); + if (fd == -1) + throw SysError(format("opening user lock `%1%'") % fnUserLock); + + if (lockFile(fd, ltWrite, false)) { + fdUserLock = fd.borrow(); + lockedPaths.insert(fnUserLock); + uid = pw->pw_uid; + return; + } + } + + throw Error("all build users are currently in use; " + "consider expanding the `build-users' field " + "in the Nix configuration file"); +} + + +void UserLock::release() +{ + if (uid == 0) return; + fdUserLock.close(); /* releases lock */ + assert(lockedPaths.find(fnUserLock) != lockedPaths.end()); + lockedPaths.erase(fnUserLock); + fnUserLock = ""; + uid = 0; +} + + +uid_t UserLock::getUID() +{ + return uid; +} + + static void killUser(uid_t uid) { debug(format("killing all processes running under uid `%1%'") % uid); @@ -381,7 +485,7 @@ private: PathSet allPaths; /* User selected for running the builder. */ - uid_t buildUser; + UserLock buildUser; /* The process ID of the builder. */ Pid pid; @@ -468,7 +572,6 @@ DerivationGoal::DerivationGoal(const Path & drvPath, Worker & worker) { this->drvPath = drvPath; state = &DerivationGoal::init; - buildUser = 0; name = (format("building of `%1%'") % drvPath).str(); trace("created"); } @@ -681,8 +784,8 @@ void DerivationGoal::buildDone() malicious user from leaving behind a process that keeps files open and modifies them after they have been chown'ed to root. */ - if (buildUser != 0) - killUser(buildUser); + if (buildUser.getUID() != 0) + killUser(buildUser.getUID()); /* Close the read side of the logger pipe. */ logPipe.readSide.close(); @@ -713,6 +816,9 @@ void DerivationGoal::buildDone() return; } + /* Release the build user, if applicable. */ + buildUser.release(); + amDone(); } @@ -1023,29 +1129,6 @@ bool DerivationGoal::prepareBuild() } -static uid_t allocBuildUser() -{ - Strings buildUsers = querySetting("build-users", Strings()); - - if (buildUsers.empty()) - throw Error( - "cannot build as `root'; you must define " - "one or more users for building in `build-users' " - "in the Nix configuration file"); - - for (Strings::iterator i = buildUsers.begin(); i != buildUsers.end(); ++i) { - printMsg(lvlError, format("trying user `%1%'") % *i); - - struct passwd * pw = getpwnam(i->c_str()); - if (!pw) - throw Error(format("the user `%1%' listed in `build-users' does not exist") % *i); - - return pw->pw_uid; - } - -} - - void DerivationGoal::startBuilder() { startNest(nest, lvlInfo, @@ -1126,14 +1209,15 @@ void DerivationGoal::startBuilder() if (!queryBoolSetting("build-allow-root", true) && getuid() == rootUserId) { - buildUser = allocBuildUser(); + buildUser.acquire(); + assert(buildUser.getUID() != 0); /* Make sure that no other processes are executing under this uid. */ - killUser(buildUser); + killUser(buildUser.getUID()); /* Change ownership of the temporary build directory. !!! gid */ - if (chown(tmpDir.c_str(), buildUser, (gid_t) -1) == -1) + if (chown(tmpDir.c_str(), buildUser.getUID(), (gid_t) -1) == -1) throw SysError(format("cannot change ownership of `%1%'") % tmpDir); /* Check that the Nix store has the appropriate permissions, @@ -1198,15 +1282,15 @@ void DerivationGoal::startBuilder() except std*, so that's safe. Also note that setuid() when run as root sets the real, effective and saved UIDs. */ - if (buildUser != 0) { - printMsg(lvlError, format("switching to uid `%1%'") % buildUser); + if (buildUser.getUID() != 0) { + printMsg(lvlError, format("switching to uid `%1%'") % buildUser.getUID()); /* !!! setgid also */ if (setgroups(0, 0) == -1) throw SysError("cannot clear the set of supplementary groups"); - setuid(buildUser); - assert(getuid() == buildUser); - assert(geteuid() == buildUser); + setuid(buildUser.getUID()); + assert(getuid() == buildUser.getUID()); + assert(geteuid() == buildUser.getUID()); } @@ -1296,7 +1380,7 @@ void DerivationGoal::computeClosure() build. Also, the output should be owned by the build user. */ if ((st.st_mode & (S_IWGRP | S_IWOTH)) || - (buildUser != 0 && st.st_uid != buildUser)) + (buildUser.getUID() != 0 && st.st_uid != buildUser.getUID())) throw Error(format("suspicious ownership or permission on `%1%'; rejecting this build output") % path); /* Get rid of all weird permissions. */