Package: guix-patches;
Reported by: Ludovic Courtès <ludo <at> gnu.org>
Date: Tue, 8 Apr 2025 13:31:02 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludovic.courtes <at> inria.fr>
Bug is archived. No further changes may be made.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: guix-patches <at> gnu.org Cc: keinflue <keinflue <at> posteo.net>, Ludovic Courtès <ludo <at> gnu.org>, Reepca Russelstein <reepca <at> russelstein.xyz>, Ada Stevenson <adanskana <at> gmail.com> Subject: [PATCH] daemon: Do not make chroot root directory read-only. Date: Tue, 8 Apr 2025 15:29:44 +0200
Fixes <https://issues.guix.gnu.org/77570>.
Commit 40f69b586a440d0397fa3dfe03b95a0f44e4d242 made chroot root
directory read-only; as a consequence, build processes attempting to
write to the root directory would now get EROFS instead of EACCES.
It turns out that a number of test suites (Go, Ruby, SCons, Shepherd)
would fail because of this observable difference.
To restore previous behavior in build environments while still
preventing build processes from exposing their root directory to outside
processes, this patch (1) keeps the root writable but #o555 by default,
thereby restoring the EACCES behavior, and (2) ensures that the parent
of the chroot root directory is itself user-accessible only.
* nix/libstore/build.cc (class DerivationGoal)[chrootRootTop]: New
field.
(DerivationGoal::startBuilder): Initialize ‘chrootRootTop’ and make it
‘AutoDelete’. Replace ‘mount’ call that made the root directory
read-only by a mere ‘chmod_’ call.
* tests/store.scm ("build root cannot be made world-readable"): Remove.
("writing to build root leads to EACCES"): New test.
Reported-by: Ada Stevenson <adanskana <at> gmail.com>
Reported-by: keinflue <keinflue <at> posteo.net>
Suggested-by: Reepca Russelstein <reepca <at> russelstein.xyz>
Change-Id: I5912e8b3b293f8242a010cfc79255fc981314445
---
nix/libstore/build.cc | 35 ++++++++++++++++++++++++-----------
tests/store.scm | 35 +++++++++++++----------------------
2 files changed, 37 insertions(+), 33 deletions(-)
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index d0fcc99854..1a38e85816 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -657,7 +657,9 @@ private:
/* Whether we're currently doing a chroot build. */
bool useChroot;
- Path chrootRootDir;
+ /* The directory containing the chroot root directory, and the chroot root
+ directory itself--the latter is a sub-directory of the former. */
+ Path chrootRootTop, chrootRootDir;
/* RAII object to delete the chroot directory. */
std::shared_ptr<AutoDelete> autoDelChroot;
@@ -1810,19 +1812,21 @@ void DerivationGoal::startBuilder()
if (useChroot) {
#if CHROOT_ENABLED
/* Create a temporary directory in which we set up the chroot
- environment using bind-mounts. We put it in the store
- to ensure that we can create hard-links to non-directory
- inputs in the fake store in the chroot (see below). */
- chrootRootDir = drvPath + ".chroot";
- if (pathExists(chrootRootDir)) deletePath(chrootRootDir);
+ environment using bind-mounts. Put it in the store to ensure it
+ can be atomically moved to the store. */
+ chrootRootTop = drvPath + ".chroot";
+ chrootRootDir = chrootRootTop + "/top";
+ if (pathExists(chrootRootTop)) deletePath(chrootRootTop);
/* Clean up the chroot directory automatically. */
- autoDelChroot = std::shared_ptr<AutoDelete>(new AutoDelete(chrootRootDir));
+ autoDelChroot = std::shared_ptr<AutoDelete>(new AutoDelete(chrootRootTop));
printMsg(lvlChatty, format("setting up chroot environment in `%1%'") % chrootRootDir);
+ if (mkdir(chrootRootTop.c_str(), 0750) == -1)
+ throw SysError(format("cannot create build root container '%1%'") % chrootRootTop);
if (mkdir(chrootRootDir.c_str(), 0750) == -1)
- throw SysError(format("cannot create ‘%1%’") % chrootRootDir);
+ throw SysError(format("cannot create build root '%1%'") % chrootRootDir);
if (buildUser.enabled() && chown(chrootRootDir.c_str(), 0, buildUser.getGID()) == -1)
throw SysError(format("cannot change ownership of ‘%1%’") % chrootRootDir);
@@ -2245,9 +2249,18 @@ void DerivationGoal::runChild()
if (rmdir("real-root") == -1)
throw SysError("cannot remove real-root directory");
- /* Remount root as read-only. */
- if (mount("/", "/", 0, MS_BIND | MS_REMOUNT | MS_RDONLY, 0) == -1)
- throw SysError(format("read-only remount of build root '%1%' failed") % chrootRootDir);
+ /* Make the root read-only.
+
+ The build process could make it world-accessible, but that's
+ OK: since 'chrootRootTop' is *not* world-accessible, a
+ world-accessible 'chrootRootDir' cannot be used to grant access
+ to the store to external processes.
+
+ Remounting the root as read-only was rejected because it makes
+ write access fail with EROFS instead of EACCES, which goes
+ against what some test suites expect (Go, Ruby, SCons,
+ Shepherd, to name a few). */
+ chmod_("/", 0555);
if (getuid() != 0) {
/* Create a new mount namespace to "lock" previous mounts.
diff --git a/tests/store.scm b/tests/store.scm
index b1ddff2082..b467314bdc 100644
--- a/tests/store.scm
+++ b/tests/store.scm
@@ -498,32 +498,23 @@ (define %shell
(unless (unprivileged-user-namespace-supported?)
(test-skip 1))
-(test-assert "build root cannot be made world-readable"
+(test-assert "writing to build root leads to EACCES"
(let ((drv
(run-with-store %store
(gexp->derivation
- "attempt-to-make-root-world-readable"
- (with-imported-modules (source-module-closure
- '((guix build syscalls)))
- #~(begin
- (use-modules (guix build syscalls))
+ "write-to-root"
+ #~(begin
+ (catch 'system-error
+ (lambda ()
+ (mkdir "/whatever"))
+ (lambda args
+ (format #t "mkdir failed, which is good: ~a~%"
+ (strerror (system-error-errno args)))
+ (when (= EACCES (system-error-errno args))
+ (exit 1))))
- (catch 'system-error
- (lambda ()
- (chmod "/" #o777))
- (lambda args
- (format #t "failed to make root writable: ~a~%"
- (strerror (system-error-errno args)))
- (format #t "attempting read-write remount~%")
- (mount "none" "/" "/" (logior MS_BIND MS_REMOUNT))
- (chmod "/" #o777)))
-
- ;; At this point, the build process could create a
- ;; world-readable setuid binary under its root (so in the
- ;; store) that would remain visible until the build
- ;; completes.
- (mkdir #$output)))))))
- (guard (c ((store-protocol-error? c) #t))
+ (mkdir #$output))))))
+ (guard (c ((store-protocol-error? c) c))
(build-derivations %store (list drv))
#f)))
base-commit: 1dab24555a494beb3db5a335c675f07043e77f1c
--
2.49.0
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.