GNU bug report logs -
#77642
[PATCH] daemon: Do not make chroot root directory read-only.
Previous Next
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.
Full log
View this message in rfc822 format
[Message part 1 (text/plain, inline)]
Your message dated Fri, 11 Apr 2025 12:20:38 +0200
with message-id <87bjt3m22x.fsf <at> inria.fr>
and subject line Re: [bug#77642] [PATCH] daemon: Do not make chroot root directory read-only.
has caused the debbugs.gnu.org bug report #77642,
regarding [PATCH] daemon: Do not make chroot root directory read-only.
to be marked as done.
(If you believe you have received this mail in error, please contact
help-debbugs <at> gnu.org.)
--
77642: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=77642
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
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
[Message part 3 (message/rfc822, inline)]
Hi,
I amended the comments as you suggested and pushed it as
ff5181e27e79c88a82dd429b382e0764af489957.
I’ll update the ‘guix’ package soonish.
Ludo’.
This bug report was last modified 91 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.