[bug#77642] daemon: Do not make chroot root directory read-only.
Commit Message
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@gmail.com>
Reported-by: keinflue <keinflue@posteo.net>
Suggested-by: Reepca Russelstein <reepca@russelstein.xyz>
Change-Id: I5912e8b3b293f8242a010cfc79255fc981314445
---
nix/libstore/build.cc | 35 ++++++++++++++++++++++++-----------
tests/store.scm | 35 +++++++++++++----------------------
2 files changed, 37 insertions(+), 33 deletions(-)
base-commit: 1dab24555a494beb3db5a335c675f07043e77f1c
Comments
Ludovic Courtès <ludo@gnu.org> writes:
> @@ -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
Strictly speaking, in the case of --build-users-group, it couldn't even
do that.
> + OK: since 'chrootRootTop' is *not* world-accessible, a
> + world-accessible 'chrootRootDir' cannot be used to grant access
> + to the store to external processes.
It may be more general to write "grant access to the build environment",
unless you're using this as a shorthand for "grant access to the build
environment, and thereby a setuid binary, and thereby (in some
configurations) the store".
Looks good to me, hopefully there aren't any major packages further down
the line that rely on chmod("/", ...) failing.
- reepca
Hi,
Reepca Russelstein <reepca@russelstein.xyz> skribis:
>> + /* Make the root read-only.
>> +
>> + The build process could make it world-accessible, but that's
>
> Strictly speaking, in the case of --build-users-group, it couldn't even
> do that.
True.
>> + OK: since 'chrootRootTop' is *not* world-accessible, a
>> + world-accessible 'chrootRootDir' cannot be used to grant access
>> + to the store to external processes.
>
> It may be more general to write "grant access to the build environment",
> unless you're using this as a shorthand for "grant access to the build
> environment, and thereby a setuid binary, and thereby (in some
> configurations) the store".
Yes, but I’ll change it as you suggest.
> Looks good to me, hopefully there aren't any major packages further down
> the line that rely on chmod("/", ...) failing.
Crossing fingers…
Ludo’.
Hi,
I amended the comments as you suggested and pushed it as
ff5181e27e79c88a82dd429b382e0764af489957.
I’ll update the ‘guix’ package soonish.
Ludo’.
@@ -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.
@@ -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)))