From patchwork Tue Apr 8 13:29:44 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ludovic_Court=C3=A8s?= X-Patchwork-Id: 41455 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id A941127BC4A; Tue, 8 Apr 2025 14:31:31 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2, RCVD_IN_VALIDITY_CERTIFIED,RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE, SPF_HELO_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id 4470627BC49 for ; Tue, 8 Apr 2025 14:31:30 +0100 (BST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1u292g-00039x-Sk; Tue, 08 Apr 2025 09:31:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1u292d-000398-TU for guix-patches@gnu.org; Tue, 08 Apr 2025 09:31:04 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1u292d-0001Ch-FX for guix-patches@gnu.org; Tue, 08 Apr 2025 09:31:03 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-Version:Date:From:To:Subject; bh=Fsbk82DEQ9dIk2tXlY378vpo1TWoaRffDTT/TcwWeB8=; b=XJuGk4PXMTeEXf2RT8LlICcDSrydf6HJxSsj7HVolkR2ngEeOBuXgqQjQkqPEPR5FtLH7IUH57Uno+yO0TiX8phye0QEFsEunjTHW4Xre3TTt4tBKnkb/ZrXIY9ZrKECGU6lZoGuDiqngC4TXp8u4Mh6aqZFZQyFSNfw8+o8HOKsZtvhuGHImnIJXJzcxaSSZx4zY4IC8d3ZYHjDgZmwXG6A4hGjz/HJJbCrbBGB+EIZ3ipRcZ8gdWKQHjijya+3DRnelCf6dizt/FRGDo6B0fIUvcFtyjtm1UROU+4Tkbw0aImr3tfu/TaYajUDPS3PUphWmVD0Wpx3DEsaTOvMhQ==; Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1u292d-0008Nl-5z for guix-patches@gnu.org; Tue, 08 Apr 2025 09:31:03 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#77642] [PATCH] daemon: Do not make chroot root directory read-only. Resent-From: Ludovic =?utf-8?q?Court=C3=A8s?= Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Tue, 08 Apr 2025 13:31:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: report 77642 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 77642@debbugs.gnu.org Cc: keinflue , Ludovic =?utf-8?q?Court=C3=A8s?= , Reepca Russelstein , Ada Stevenson X-Debbugs-Original-To: guix-patches@gnu.org Received: via spool by submit@debbugs.gnu.org id=B.174411902531939 (code B ref -1); Tue, 08 Apr 2025 13:31:02 +0000 Received: (at submit) by debbugs.gnu.org; 8 Apr 2025 13:30:25 +0000 Received: from localhost ([127.0.0.1]:59989 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1u291y-0008In-0z for submit@debbugs.gnu.org; Tue, 08 Apr 2025 09:30:25 -0400 Received: from lists.gnu.org ([2001:470:142::17]:56186) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1u291p-0008Cu-3O for submit@debbugs.gnu.org; Tue, 08 Apr 2025 09:30:16 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1u291g-0002qi-RK for guix-patches@gnu.org; Tue, 08 Apr 2025 09:30:05 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1u291b-0000v3-0Z; Tue, 08 Apr 2025 09:29:59 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-Version:Date:Subject:To:From:in-reply-to: references; bh=Fsbk82DEQ9dIk2tXlY378vpo1TWoaRffDTT/TcwWeB8=; b=nkAakaYUBNTYkW UQ8ym9OxlM7WeerEzFsEagTBDqpHUC8k8TzPx62QtuKeq5ocvFXkOFU4J1Bj8LtMr/8O9cHsdgtV4 7A7Ibo9J5gkwu/KUaatQ6gZhRkKmgeGjK6K5Inh8/GGFOs7NV9Z0yw6gniB06DMPkdFR2k5HFwUAD lxxVD7PP2ikF6ZoSH91RYrn3jOUCdGVhZvy9tVEdqm74Np9E5Q6aAH4QCq2VpsVk4tCIkFK6HmDtp wqOCBl2mZ1xAezHtNz444tmqMPt/55mc0hA9Tm+kyB9WusMTamkMAKF4q6lC5WRsobuUU+iXT6yZW dCloAbIaAGC35J4crfgQ==; From: Ludovic =?utf-8?q?Court=C3=A8s?= Date: Tue, 8 Apr 2025 15:29:44 +0200 Message-ID: X-Mailer: git-send-email 2.49.0 MIME-Version: 1.0 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org X-getmail-retrieved-from-mailbox: Patches Fixes . 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 Reported-by: keinflue Suggested-by: Reepca Russelstein Change-Id: I5912e8b3b293f8242a010cfc79255fc981314445 --- nix/libstore/build.cc | 35 ++++++++++++++++++++++++----------- tests/store.scm | 35 +++++++++++++---------------------- 2 files changed, 37 insertions(+), 33 deletions(-) base-commit: 1dab24555a494beb3db5a335c675f07043e77f1c 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 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(new AutoDelete(chrootRootDir)); + autoDelChroot = std::shared_ptr(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)))