GNU bug report logs - #77642
[PATCH] daemon: Do not make chroot root directory read-only.

Previous Next

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.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 77642 in the body.
You can then email your comments to 77642 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#77642; Package guix-patches. (Tue, 08 Apr 2025 13:31:03 GMT) Full text and rfc822 format available.

Acknowledgement sent to Ludovic Courtès <ludo <at> gnu.org>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Tue, 08 Apr 2025 13:31:03 GMT) Full text and rfc822 format available.

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





Information forwarded to guix-patches <at> gnu.org:
bug#77642; Package guix-patches. (Tue, 08 Apr 2025 15:25:01 GMT) Full text and rfc822 format available.

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Reepca Russelstein <reepca <at> russelstein.xyz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: keinflue <keinflue <at> posteo.net>, Ada Stevenson <adanskana <at> gmail.com>,
 guix-patches <at> gnu.org
Subject: Re: [PATCH] daemon: Do not make chroot root directory read-only.
Date: Tue, 08 Apr 2025 10:23:27 -0500
[Message part 1 (text/plain, inline)]
Ludovic Courtès <ludo <at> 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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#77642; Package guix-patches. (Thu, 10 Apr 2025 08:27:03 GMT) Full text and rfc822 format available.

Message #11 received at 77642 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludovic.courtes <at> inria.fr>
To: Reepca Russelstein <reepca <at> russelstein.xyz>
Cc: keinflue <at> posteo.net, adanskana <at> gmail.com, 77642 <at> debbugs.gnu.org
Subject: Re: [bug#77642] [PATCH] daemon: Do not make chroot root directory
 read-only.
Date: Thu, 10 Apr 2025 09:55:46 +0200
Hi,

Reepca Russelstein <reepca <at> 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’.




Reply sent to Ludovic Courtès <ludovic.courtes <at> inria.fr>:
You have taken responsibility. (Fri, 11 Apr 2025 10:21:01 GMT) Full text and rfc822 format available.

Notification sent to Ludovic Courtès <ludo <at> gnu.org>:
bug acknowledged by developer. (Fri, 11 Apr 2025 10:21:02 GMT) Full text and rfc822 format available.

Message #16 received at 77642-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludovic.courtes <at> inria.fr>
To: Reepca Russelstein <reepca <at> russelstein.xyz>
Cc: keinflue <at> posteo.net, adanskana <at> gmail.com, 77642-done <at> debbugs.gnu.org
Subject: Re: [bug#77642] [PATCH] daemon: Do not make chroot root directory
 read-only.
Date: Fri, 11 Apr 2025 12:20:38 +0200
Hi,

I amended the comments as you suggested and pushed it as
ff5181e27e79c88a82dd429b382e0764af489957.

I’ll update the ‘guix’ package soonish.

Ludo’.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 09 May 2025 11:24:10 GMT) Full text and rfc822 format available.

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.