From 8ea3a911aa81d41efdff231f4b42b11d8861a000 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 16 Jul 2022 14:39:22 -0700 Subject: [PATCH 1/9] local-derivation-goal.cc: improve error messages when sandboxing fails The failure modes for nix's sandboxing setup are pretty complicated. When nix is unable to set up the sandbox, let's provide more detail about what went wrong. Specifically: * Make sure the error message includes the word "sandbox" so the user knows that the failure was related to sandboxing. * If `--option sandbox-fallback false` was provided, and removing it would have allowed further attempts to make progress, let the user know. --- src/libstore/build/local-derivation-goal.cc | 24 +++++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index d1ec91ed5..86a59e427 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -850,13 +850,23 @@ void LocalDerivationGoal::startBuilder() flags &= ~CLONE_NEWUSER; child = clone(childEntry, stack + stackSize, flags, this); } - /* Otherwise exit with EPERM so we can handle this in the - parent. This is only done when sandbox-fallback is set - to true (the default). */ - if (child == -1 && (errno == EPERM || errno == EINVAL) && settings.sandboxFallback) - _exit(1); - if (child == -1) throw SysError("cloning builder process"); - + if (child == -1) + switch(errno) { + case EPERM: + case EINVAL: { + /* Otherwise exit with EPERM so we can handle this in the + parent. This is only done when sandbox-fallback is set + to true (the default). */ + if (settings.sandboxFallback) + _exit(1); + /* Mention sandbox-fallback in the error message so the user + knows that having it disabled contributed to the + unrecoverability of this failure */ + throw SysError("creating sandboxed builder process using clone(), without sandbox-fallback"); + } + default: + throw SysError("creating sandboxed builder process using clone()"); + } writeFull(builderOut.writeSide.get(), fmt("%d %d\n", usingUserNamespace, child)); _exit(0); From 90830b1074cd09b58adde859fb1741a33390412f Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 16 Jul 2022 19:28:13 -0700 Subject: [PATCH 2/9] local-derivation-goal.cc: warn if failing due to max_user_namespaces==0 This commit uses `warn()` to notify the user if sandbox setup fails with errno==EPERM and /proc/sys/user/max_user_namespaces is missing or zero, since that is at least part of the reason why sandbox setup failed. Note that `echo -n 0 > /proc/sys/user/max_user_namespaces` or equivalent at boot time has been the recommended mitigation for several Linux LPE vulnerabilities over the past few years. Many users have applied this mitigation and then forgotten that they have done so. --- src/libstore/build/local-derivation-goal.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 86a59e427..674b2eaa3 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -859,6 +859,8 @@ void LocalDerivationGoal::startBuilder() to true (the default). */ if (settings.sandboxFallback) _exit(1); + if (!userNamespacesEnabled && errno==EPERM) + warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From 8d35f387dcfa61c59f898de88ef45f3f97817267 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 16 Jul 2022 19:37:27 -0700 Subject: [PATCH 3/9] local-derivation-goal.cc: warn if failing and /proc/self/ns/user missing This commit causes nix to `warn()` if sandbox setup has failed and `/proc/self/ns/user` does not exist. This is usually a sign that the kernel was compiled without `CONFIG_USER_NS=y`, which is required for sandboxing. --- src/libstore/build/local-derivation-goal.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 674b2eaa3..3aa85e264 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -861,6 +861,9 @@ void LocalDerivationGoal::startBuilder() _exit(1); if (!userNamespacesEnabled && errno==EPERM) warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); + Path procSelfNsUser = "/proc/self/ns/user"; + if (!pathExists(procSelfNsUser)) + warn("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From 6fc56318bf32f715de8634c199c0fb812f813a8c Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sun, 17 Jul 2022 01:23:27 -0700 Subject: [PATCH 4/9] local-derivation-goal.cc: add comment re: CLONE_NEWUSER local-derivation-goal.cc contains a comment stating that "Some distros patch Linux to not allow unprivileged user namespaces." Let's give a pointer to a common version of this patch for those who want more details about this failure mode. --- src/libstore/build/local-derivation-goal.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 3aa85e264..1c7618045 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -845,6 +845,7 @@ void LocalDerivationGoal::startBuilder() /* Some distros patch Linux to not allow unprivileged * user namespaces. If we get EPERM or EINVAL, try * without CLONE_NEWUSER and see if that works. + * Details: https://salsa.debian.org/kernel-team/linux/-/commit/d98e00eda6bea437e39b9e80444eee84a32438a6 */ usingUserNamespace = false; flags &= ~CLONE_NEWUSER; From c8c6203c2c6912a52c5f08881c453a04e7fc3f58 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sun, 17 Jul 2022 01:27:22 -0700 Subject: [PATCH 5/9] local-derivation-goal.cc: detect unprivileged_userns_clone failure mode The workaround for "Some distros patch Linux" mentioned in local-derivation-goal.cc will not help in the `--option sandbox-fallback false` case. To provide the user more helpful guidance on how to get the sandbox working, let's check to see if the `/proc` node created by the aforementioned patch is present and configured in a way that will cause us problems. If so, give the user a suggestion for how to troubleshoot the problem. --- src/libstore/build/local-derivation-goal.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 1c7618045..047c5c8ea 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -862,6 +862,13 @@ void LocalDerivationGoal::startBuilder() _exit(1); if (!userNamespacesEnabled && errno==EPERM) warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); + if (userNamespacesEnabled) { + Path procSysKernelUnprivilegedUsernsClone = "/proc/sys/kernel/unprivileged_userns_clone"; + if (pathExists(procSysKernelUnprivilegedUsernsClone) + && trim(readFile(procSysKernelUnprivilegedUsernsClone)) == "0") { + warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/kernel/unprivileged_userns_clone"); + } + } Path procSelfNsUser = "/proc/self/ns/user"; if (!pathExists(procSelfNsUser)) warn("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); From 5f51539f88227285866843f1383fd47d80fd5918 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:30:52 -0700 Subject: [PATCH 6/9] change warn() to notice() --- src/libstore/build/local-derivation-goal.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 047c5c8ea..595149f0a 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -861,17 +861,17 @@ void LocalDerivationGoal::startBuilder() if (settings.sandboxFallback) _exit(1); if (!userNamespacesEnabled && errno==EPERM) - warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); + notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); if (userNamespacesEnabled) { Path procSysKernelUnprivilegedUsernsClone = "/proc/sys/kernel/unprivileged_userns_clone"; if (pathExists(procSysKernelUnprivilegedUsernsClone) && trim(readFile(procSysKernelUnprivilegedUsernsClone)) == "0") { - warn("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/kernel/unprivileged_userns_clone"); + notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/kernel/unprivileged_userns_clone"); } } Path procSelfNsUser = "/proc/self/ns/user"; if (!pathExists(procSelfNsUser)) - warn("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); + notice("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From 99fcc91f67ece5a9646065665395f496d6a0cb84 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:33:12 -0700 Subject: [PATCH 7/9] as requested by @thufschmitt https://github.com/NixOS/nix/pull/6814#discussion_r924275777 --- src/libstore/build/local-derivation-goal.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 595149f0a..43df41e34 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -855,11 +855,6 @@ void LocalDerivationGoal::startBuilder() switch(errno) { case EPERM: case EINVAL: { - /* Otherwise exit with EPERM so we can handle this in the - parent. This is only done when sandbox-fallback is set - to true (the default). */ - if (settings.sandboxFallback) - _exit(1); if (!userNamespacesEnabled && errno==EPERM) notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); if (userNamespacesEnabled) { @@ -872,6 +867,11 @@ void LocalDerivationGoal::startBuilder() Path procSelfNsUser = "/proc/self/ns/user"; if (!pathExists(procSelfNsUser)) notice("/proc/self/ns/user does not exist; your kernel was likely built without CONFIG_USER_NS=y, which is required for sandboxing"); + /* Otherwise exit with EPERM so we can handle this in the + parent. This is only done when sandbox-fallback is set + to true (the default). */ + if (settings.sandboxFallback) + _exit(1); /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ From a9e75eca00f7efe361545c6e77ecb65244230dc3 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:49:33 -0700 Subject: [PATCH 8/9] error.hh: add additional constructor with explicit errno argument --- src/libutil/error.hh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libutil/error.hh b/src/libutil/error.hh index a53e9802e..3d1479c54 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -204,13 +204,19 @@ public: int errNo; template - SysError(const Args & ... args) + SysError(int errNo_, const Args & ... args) : Error("") { - errNo = errno; + errNo = errNo_; auto hf = hintfmt(args...); err.msg = hintfmt("%1%: %2%", normaltxt(hf.str()), strerror(errNo)); } + + template + SysError(const Args & ... args) + : SysError(errno, args ...) + { + } }; } From 36e1383b6b32abd414c8402dd66f7ef78f93f4e1 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Tue, 19 Jul 2022 03:49:52 -0700 Subject: [PATCH 9/9] local-derivation-goal.cc: save global errno to the stack before performing tests which might clobber it --- src/libstore/build/local-derivation-goal.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 43df41e34..79a241ae0 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -851,10 +851,11 @@ void LocalDerivationGoal::startBuilder() flags &= ~CLONE_NEWUSER; child = clone(childEntry, stack + stackSize, flags, this); } - if (child == -1) + if (child == -1) { switch(errno) { case EPERM: case EINVAL: { + int errno_ = errno; if (!userNamespacesEnabled && errno==EPERM) notice("user namespaces appear to be disabled; they are required for sandboxing; check /proc/sys/user/max_user_namespaces"); if (userNamespacesEnabled) { @@ -875,11 +876,12 @@ void LocalDerivationGoal::startBuilder() /* Mention sandbox-fallback in the error message so the user knows that having it disabled contributed to the unrecoverability of this failure */ - throw SysError("creating sandboxed builder process using clone(), without sandbox-fallback"); + throw SysError(errno_, "creating sandboxed builder process using clone(), without sandbox-fallback"); } default: throw SysError("creating sandboxed builder process using clone()"); } + } writeFull(builderOut.writeSide.get(), fmt("%d %d\n", usingUserNamespace, child)); _exit(0);