From 6e5ec1029ad279c1ac69e14730afb4d2d9964b5d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 6 Dec 2006 20:00:15 +0000 Subject: [PATCH] * 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. --- nix.conf.example | 58 +++++++++++------------ src/libstore/build.cc | 86 +++++++++++++++++++++-------------- src/nix-setuid-helper/main.cc | 10 ++-- 3 files changed, 88 insertions(+), 66 deletions(-) diff --git a/nix.conf.example b/nix.conf.example index a75045b14..92e114dc5 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.) +# +# 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 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 empty. # # Example: -# build-users = nix-builder-1 nix-builder-2 nix-builder-3 -#build-users = - - -### Option `build-users-group' -# -# 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. -# -# The default is `nix'. -# -# 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 0ecd8bb10..ec3353cf3 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 != ""); - for (Strings::iterator i = buildUsers.begin(); i != buildUsers.end(); ++i) { + /* 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); + + /* 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); + if (setgid(buildUser.getGID()) == -1 || + getgid() != buildUser.getGID() || + getegid() != buildUser.getGID()) + throw SysError("setgid failed"); - setuid(buildUser.getUID()); - assert(getuid() == buildUser.getUID()); - assert(geteuid() == buildUser.getUID()); + 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 50a059f50..d278d5677 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 args;