* Get rid of `build-users'. We'll just take all the members of

  `build-users-group'.  This makes configuration easier: you can just
  add users in /etc/group.

3 files changed, 86 insertions, 64 deletions

diff --git a/nix.conf.example b/nix.conf.example
index a75045b148..92e114dc53 100644
--- a/nix.conf.example
+++ b/nix.conf.example
@@ -78,44 +78,44 @@
 #build-max-jobs = 1
 
 
-### Option `build-users'
+### Option `build-users-group'
 #
-# This option contains a list of user names under which Nix can
-# execute builds.  In multi-user Nix installations, builds should not
+# This options specifies the Unix group containing the Nix build user
+# accounts.  In multi-user Nix installations, builds should not
 # be performed by the Nix account since that would allow users to
 # arbitrarily modify the Nix store and database by supplying specially
 # crafted builders; and they cannot be performed by the calling user
 # since that would allow him/her to influence the build result.
 #
-# Thus this list should contain a number of `special' user accounts
-# created specifically for Nix, e.g., `nix-builder-1',
-# `nix-builder-2', and so on.  The more users the better, since at
-# most a number of builds equal to the number of build users can be
-# running simultaneously.
+# Therefore, if this option is non-empty and specifies a valid group,
+# builds will be performed under the user accounts that are a member
+# of the group specified here (as listed in /etc/group).  Those user
+# accounts should not be used for any other purpose!
 #
-# If this list is empty, builds will be performed under the Nix
-# account (that is, the uid under which the Nix daemon runs, or that
-# owns the setuid nix-worker program).
+# Nix will never run two builds under the same user account at the
+# same time.  This is to prevent an obvious security hole: a malicious
+# user writing a Nix expression that modifies the build result of a
+# legitimate Nix expression being built by another user.  Therefore it
+# is good to have as many Nix build user accounts as you can spare.
+# (Remember: uids are cheap.)
 #
-# Example:
-#   build-users = nix-builder-1 nix-builder-2 nix-builder-3
-#build-users =
-
-
-### Option `build-users-group'
+# The build users should have permission to create files in the Nix
+# store, but not delete them.  Therefore, /nix/store should be owned
+# by the Nix account, its group should be the group specified here,
+# and its mode should be 1775.
 #
-# If `build-users' is used, then this option specifies the group ID
-# (gid) under which each build is to be performed.  This group should
-# have permission to create files in the Nix store, but not delete
-# them.  I.e., /nix/store should be owned by the Nix account, its
-# group should be the group specified here, and its mode should be
-# 1775.
+# If the build users group is empty, builds will be performed under
+# the uid of the Nix process (that is, the uid of the caller if
+# $NIX_REMOTE is empty, the uid under which the Nix daemon runs if
+# $NIX_REMOTE is `daemon', or the uid that owns the setuid nix-worker
+# program if $NIX_REMOTE is `slave').  Obviously, this should not be
+# used in multi-user settings with untrusted users.
 #
-# The default is `nix'.
+# The default is empty.
 #
 # Example:
-#   build-users-group = nix
-#build-users-group =
+#   build-users-group = nix-builders
+build-users-group = nix-builders
 
 
 ### Option `system'
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 0ecd8bb107..ec3353cf3b 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -341,6 +341,7 @@ private:
     AutoCloseFD fdUserLock;
 
     uid_t uid;
+    gid_t gid;
     
 public:
     UserLock();
@@ -350,6 +351,7 @@ public:
     void release();
 
     uid_t getUID();
+    uid_t getGID();
 };
 
 
@@ -358,7 +360,7 @@ PathSet UserLock::lockedPaths;
 
 UserLock::UserLock()
 {
-    uid = 0;
+    uid = gid = 0;
 }
 
 
@@ -371,21 +373,37 @@ UserLock::~UserLock()
 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");
+    string buildUsersGroup = querySetting("build-users-group", "");
+    assert(buildUsersGroup != "");
+
+    /* Get the members of the build-users-group. */
+    struct group * gr = getgrnam(buildUsersGroup.c_str());
+    if (!gr)
+        throw Error(format("the group `%1%' specified in `build-users-group' does not exist")
+            % buildUsersGroup);
+    gid = gr->gr_gid;
+
+    /* Copy the result of getgrnam. */
+    Strings users;
+    for (char * * p = gr->gr_mem; *p; ++p) {
+        debug(format("found build user `%1%'") % *p);
+        users.push_back(*p);
+    }
+
+    if (users.empty())
+        throw Error(format("the build users group `%1%' has no members")
+            % buildUsersGroup);
 
-    for (Strings::iterator i = buildUsers.begin(); i != buildUsers.end(); ++i) {
+    /* Find a user account that isn't currently in use for another
+       build. */
+    for (Strings::iterator i = users.begin(); i != users.end(); ++i) {
         debug(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);
+            throw Error(format("the user `%1%' in the group `%2%' does not exist")
+                % *i % buildUsersGroup);
 
         fnUserLock = (format("%1%/userpool/%2%") % nixStateDir % pw->pw_uid).str();
 
@@ -405,9 +423,9 @@ void UserLock::acquire()
         }
     }
 
-    throw Error("all build users are currently in use; "
-        "consider expanding the `build-users' field "
-        "in the Nix configuration file");
+    throw Error(format("all build users are currently in use; "
+        "consider creating additional users and adding them to the `%1%' group")
+        % buildUsersGroup);
 }
 
 
@@ -428,6 +446,12 @@ uid_t UserLock::getUID()
 }
 
 
+uid_t UserLock::getGID()
+{
+    return gid;
+}
+
+
 static void killUser(uid_t uid)
 {
     debug(format("killing all processes running under uid `%1%'") % uid);
@@ -1275,12 +1299,12 @@ void DerivationGoal::startBuilder()
     }
 
     
-    /* If `build-users' is not empty, then we have to build as one of
-       the users listed in `build-users'. */
-    gid_t gidBuildGroup = -1;    
-    if (querySetting("build-users", Strings()).size() > 0) {
+    /* If `build-users-group' is not empty, then we have to build as
+       one of the members of that group. */
+    if (querySetting("build-users-group", "") != "") {
         buildUser.acquire();
         assert(buildUser.getUID() != 0);
+        assert(buildUser.getGID() != 0);
 
         /* Make sure that no other processes are executing under this
            uid. */
@@ -1290,16 +1314,8 @@ void DerivationGoal::startBuilder()
         if (chown(tmpDir.c_str(), buildUser.getUID(), (gid_t) -1) == -1)
             throw SysError(format("cannot change ownership of `%1%'") % tmpDir);
 
-        /* What group to execute the builder in? */
-        string buildGroup = querySetting("build-users-group", "nix");
-        struct group * gr = getgrnam(buildGroup.c_str());
-        if (!gr) throw Error(
-            format("the group `%1%' specified in `build-users-group' does not exist")
-            % buildGroup);
-        gidBuildGroup = gr->gr_gid;
-        
         /* Check that the Nix store has the appropriate permissions,
-           i.e., owned by root and mode 1777 (sticky bit on so that
+           i.e., owned by root and mode 1775 (sticky bit on so that
            the builder can create its output but not mess with the
            outputs of other processes). */
         struct stat st;
@@ -1307,11 +1323,11 @@ void DerivationGoal::startBuilder()
             throw SysError(format("cannot stat `%1%'") % nixStore);
         if (!(st.st_mode & S_ISVTX) ||
             ((st.st_mode & S_IRWXG) != S_IRWXG) ||
-            (st.st_gid != gidBuildGroup))
+            (st.st_gid != buildUser.getGID()))
             throw Error(format(
                 "builder does not have write permission to `%1%'; "
                 "try `chgrp %1% %2%; chmod 1775 %2%'")
-                % buildGroup % nixStore);
+                % buildUser.getGID() % nixStore);
     }
 
     
@@ -1365,13 +1381,15 @@ void DerivationGoal::startBuilder()
                 if (setgroups(0, 0) == -1)
                     throw SysError("cannot clear the set of supplementary groups");
                 
-                setgid(gidBuildGroup);
-                assert(getgid() == gidBuildGroup);
-                assert(getegid() == gidBuildGroup);
-
-                setuid(buildUser.getUID());
-                assert(getuid() == buildUser.getUID());
-                assert(geteuid() == buildUser.getUID());
+                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");
             }
             
             /* Execute the program.  This should not return. */
diff --git a/src/nix-setuid-helper/main.cc b/src/nix-setuid-helper/main.cc
index 50a059f504..d278d56773 100644
--- a/src/nix-setuid-helper/main.cc
+++ b/src/nix-setuid-helper/main.cc
@@ -40,14 +40,18 @@ static void runBuilder(string userName,
        don't want to create that directory here. */
     secureChown(pw->pw_uid, gidBuilders, ".");
 
+                
     /* 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");
     //setgid(gidBuilders);
 
     /* Set the real, effective and saved uid. */
-    setuid(pw->pw_uid);
-    if (getuid() != pw->pw_uid || geteuid() != pw->pw_uid)
-        throw Error("cannot setuid");
+    if (setuid(pw->pw_uid) == -1 ||
+        getuid() != pw->pw_uid ||
+        geteuid() != pw->pw_uid)
+        throw SysError("setuid failed");
 
     /* Execute the program. */
     std::vector<const char *> args;