[bug#78256] daemon: Use the actual overflow UID and GID in /etc/passwd.
Commit Message
Partly fixes <https://issues.guix.gnu.org/77862>.
* nix/libstore/build.cc (fileContent, overflowUID, overflowGID): New
functions.
(DerivationGoal::startBuilder): Use them to populate /etc/passwd when
‘buildUser.enabled()’ is false.
Reported-by: keinflue <keinflue@posteo.net>
Change-Id: I695c697629c739d096933274c1c8a70d08468d4a
---
nix/libstore/build.cc | 37 +++++++++++++++++++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)
base-commit: c2c4bc8758616ebc0148e1bce9311a80658ace88
Comments
On 05.05.2025 10:59, Ludovic Courtès wrote:
> Partly fixes <https://issues.guix.gnu.org/77862>.
>
> * nix/libstore/build.cc (fileContent, overflowUID, overflowGID): New
> functions.
> (DerivationGoal::startBuilder): Use them to populate /etc/passwd when
> ‘buildUser.enabled()’ is false.
>
> Reported-by: keinflue <keinflue@posteo.net>
> Change-Id: I695c697629c739d096933274c1c8a70d08468d4a
> ---
> nix/libstore/build.cc | 37 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index a1f39d9a8b..773dcf1a01 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -13,6 +13,7 @@
> #include <map>
> #include <sstream>
> #include <algorithm>
> +#include <iostream>
>
> #include <limits.h>
> #include <time.h>
> @@ -1646,6 +1647,36 @@ static void initializeUserNamespace(pid_t child,
> (format("%d %d 1") % guestGID % hostGID).str());
> }
>
> +/* Return the content of FILE as an integer, or DFLT if FILE could not
> be
> + opened or parsed. */
> +static unsigned int fileContent(const std::string &file, int dflt)
I think dflt should also be unsigned here? (I don't think POSIX
specifies signdness of the ids, but they are unsigned on Linux.)
> +{
> + AutoCloseFD fd;
> + fd = open(file.c_str(), O_RDONLY|O_CLOEXEC);
> + if (fd == -1)
> + return dflt;
> + else {
> + char buf[64];
> + ssize_t count = read (fd, buf, sizeof buf);
I am not sure it can happen in the /proc file system, but generally
there is no guarantee that this will read the whole file even if it is
smaller than the buffer size. The read may return with partial result on
a signal and EINTR may also occur.
> + if (count <= 0) return dflt;
> +
> + unsigned int result = dflt;
> + std::string str = buf;
buf is not null-terminated, but this constructor of std::string requires
a null-terminated byte string as argument. std::string has another
constructor that takes a count:
std::string str(buf, count);
> + try { result = std::stoi(str); } catch (...) {};
std::stoi converts to signed int. It will throw for the upper half of
valid uids/gids and it will accept negative values. I'd recommend to use
std::stoll instead and to make result have type signed long long. Then
at the end of the function it is possible to check the values range if
desired:
if(result < 0 || result > std::numeric_limits<unsigned int>::max())
return dlft;
else
return result;
> + return result;
> + }
> +}
> +
> +static uid_t overflowUID()
> +{
> + return fileContent("/proc/sys/kernel/overflowuid", 65534);
> +}
> +
> +static gid_t overflowGID()
> +{
> + return fileContent("/proc/sys/kernel/overflowgid", 65534);
> +}
> +
> void DerivationGoal::startBuilder()
> {
> auto f = format(
> @@ -1846,9 +1877,11 @@ void DerivationGoal::startBuilder()
> writeFile(chrootRootDir + "/etc/passwd",
> (format(
> "nixbld:x:%1%:%2%:Nix build user:/:/noshell\n"
> - "nobody:x:65534:65534:Nobody:/:/noshell\n")
> + "nobody:x:%3%:%4%:Nobody:/:/noshell\n")
> % (buildUser.enabled() ? buildUser.getUID() :
> guestUID)
> - % (buildUser.enabled() ? buildUser.getGID() :
> guestGID)).str());
> + % (buildUser.enabled() ? buildUser.getGID() :
> guestGID)
> + % (buildUser.enabled() ? 65534 : overflowUID())
> + % (buildUser.enabled() ? 65534 : overflowGID())).str());
>
> /* Declare the build user's group so that programs get a
> consistent
> view of the system (e.g., "id -gn"). */
>
> base-commit: c2c4bc8758616ebc0148e1bce9311a80658ace88
In general, after some more thoughts about it, I am not really sure that
the ids of "nobody" must reflect the overflowids. It seems that this
user/group name has/had multiple different purposes and it is not clear
to me which one exactly is intended for the build environment.
Best,
keinflue
@@ -13,6 +13,7 @@
#include <map>
#include <sstream>
#include <algorithm>
+#include <iostream>
#include <limits.h>
#include <time.h>
@@ -1646,6 +1647,36 @@ static void initializeUserNamespace(pid_t child,
(format("%d %d 1") % guestGID % hostGID).str());
}
+/* Return the content of FILE as an integer, or DFLT if FILE could not be
+ opened or parsed. */
+static unsigned int fileContent(const std::string &file, int dflt)
+{
+ AutoCloseFD fd;
+ fd = open(file.c_str(), O_RDONLY|O_CLOEXEC);
+ if (fd == -1)
+ return dflt;
+ else {
+ char buf[64];
+ ssize_t count = read (fd, buf, sizeof buf);
+ if (count <= 0) return dflt;
+
+ unsigned int result = dflt;
+ std::string str = buf;
+ try { result = std::stoi(str); } catch (...) {};
+ return result;
+ }
+}
+
+static uid_t overflowUID()
+{
+ return fileContent("/proc/sys/kernel/overflowuid", 65534);
+}
+
+static gid_t overflowGID()
+{
+ return fileContent("/proc/sys/kernel/overflowgid", 65534);
+}
+
void DerivationGoal::startBuilder()
{
auto f = format(
@@ -1846,9 +1877,11 @@ void DerivationGoal::startBuilder()
writeFile(chrootRootDir + "/etc/passwd",
(format(
"nixbld:x:%1%:%2%:Nix build user:/:/noshell\n"
- "nobody:x:65534:65534:Nobody:/:/noshell\n")
+ "nobody:x:%3%:%4%:Nobody:/:/noshell\n")
% (buildUser.enabled() ? buildUser.getUID() : guestUID)
- % (buildUser.enabled() ? buildUser.getGID() : guestGID)).str());
+ % (buildUser.enabled() ? buildUser.getGID() : guestGID)
+ % (buildUser.enabled() ? 65534 : overflowUID())
+ % (buildUser.enabled() ? 65534 : overflowGID())).str());
/* Declare the build user's group so that programs get a consistent
view of the system (e.g., "id -gn"). */