#47229 Local privilege escalation via guix-daemon and ‘--keep-failed’

Message #5 received at submit@debbugs.gnu.org (full text, mbox, reply):

Received: (at submit) by debbugs.gnu.org; 18 Mar 2021 11:17:25 +0000
From debbugs-submit-bounces@debbugs.gnu.org Thu Mar 18 07:17:24 2021
Received: from localhost ([127.0.0.1]:45234 helo=debbugs.gnu.org)
	by debbugs.gnu.org with esmtp (Exim 4.84_2)
	(envelope-from <debbugs-submit-bounces@debbugs.gnu.org>)
	id 1lMqea-0002Vt-EG
	for submit@debbugs.gnu.org; Thu, 18 Mar 2021 07:17:24 -0400
Received: from lists.gnu.org ([209.51.188.17]:56814)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <ludo@gnu.org>) id 1lMqeV-0002Ve-9c
 for submit@debbugs.gnu.org; Thu, 18 Mar 2021 07:17:23 -0400
Received: from eggs.gnu.org ([2001:470:142:3::10]:41040)
 by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <ludo@gnu.org>) id 1lMqeT-0001tS-Qx
 for bug-guix@gnu.org; Thu, 18 Mar 2021 07:17:18 -0400
Received: from fencepost.gnu.org ([2001:470:142:3::e]:52987)
 by eggs.gnu.org with esmtp (Exim 4.90_1)
 (envelope-from <ludo@gnu.org>) id 1lMqeT-0002vn-Bs
 for bug-guix@gnu.org; Thu, 18 Mar 2021 07:17:17 -0400
Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=51680 helo=ribbon)
 by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256)
 (Exim 4.82) (envelope-from <ludo@gnu.org>) id 1lMqeS-0002A3-PW
 for bug-guix@gnu.org; Thu, 18 Mar 2021 07:17:17 -0400
From: Ludovic Courtès <ludo@gnu.org>
To: <bug-guix@gnu.org>
Subject: Local privilege escalation via guix-daemon and
 ‘--keep-failed’
X-Debbugs-Cc: Leo Famulari <leo@famulari.name>
X-URL: http://www.fdn.fr/~lcourtes/
X-Revolutionary-Date: 28 Ventôse an 229 de la Révolution
X-PGP-Key-ID: 0x090B11993D9AEBB5
X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc
X-PGP-Fingerprint: 3CE4 6455 8A84 FDC6 9DB4  0CFB 090B 1199 3D9A EBB5
X-OS: x86_64-pc-linux-gnu
Date: Thu, 18 Mar 2021 12:17:15 +0100
Message-ID: <87lfaksock.fsf@gnu.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
X-Spam-Score: -2.3 (--)
X-Debbugs-Envelope-To: submit
X-BeenThere: debbugs-submit@debbugs.gnu.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: <debbugs-submit.debbugs.gnu.org>
List-Unsubscribe: <https://debbugs.gnu.org/cgi-bin/mailman/options/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=unsubscribe>
List-Archive: <https://debbugs.gnu.org/cgi-bin/mailman/private/debbugs-submit/>
List-Post: <mailto:debbugs-submit@debbugs.gnu.org>
List-Help: <mailto:debbugs-submit-request@debbugs.gnu.org?subject=help>
List-Subscribe: <https://debbugs.gnu.org/cgi-bin/mailman/listinfo/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=subscribe>
Errors-To: debbugs-submit-bounces@debbugs.gnu.org
Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
X-Spam-Score: -3.3 (---)

[Message part 1 (text/plain, inline)]

A security vulnerability that can lead to local privilege escalation has
been found in ’guix-daemon’.  It affects multi-user setups in which
’guix-daemon’ runs locally.

It does not affect multi-user setups where ‘guix-daemon’ runs on a
separate machine and is accessed over the network, via
‘GUIX_DAEMON_SOCKET’, as is customary on cluster setups.  Machines where
the Linux “protected hardlink”[*] feature is enabled, which is common,
are also unaffected—this is the case when the contents of
/proc/sys/fs/protected_hardlinks are 1.

[*] https://www.kernel.org/doc/Documentation/sysctl/fs.txt


Vulnerability
~~~~~~~~~~~~~

The attack consists in having an unprivileged user spawn a build
process, for instance with ‘guix build’, that makes its build directory
world-writable.  The user then creates a hardlink within the build
directory to a root-owned file from outside of the build directory, such
as ‘/etc/shadow’.  If the user passed the ‘--keep-failed’ option and the
build eventually fails, the daemon changes ownership of the whole build
tree, including the hardlink, to the user.  At that point, the user has
write access to the target file.


Fix
~~~

The fix (patch attached) consists in adding a root-owned “wrapper”
directory in which the build directory itself is located.  If the user
passed the ‘--keep-failed’ option and the build fails, the ‘guix-daemon’
first changes ownership of the build directory, and then, in two stages,
moves the build directory into the location where users expect to find
failed builds, roughly like this:

  1. chown -R USER /tmp/guix-build-foo.drv-0/top
  2. mv /tmp/guix-build-foo.drv-0{,.pivot}
  3. mv /tmp/guix-build-foo.drv-0.pivot/top /tmp/guix-build-foo.drv-0

In step #1, /tmp/guix-build-foo.drv-0 remains root-owned, with
permissions of #o700.  Thus, only root can change directory into it or
into ‘top’.  Likewise in step #2.

The build tree becomes accessible to the user once step #3 has
succeeded, not before.  These steps are performed after the package
build scripts have stopped running.


Additionally, the patch at <https://issues.guix.gnu.org/47013> enables
protected hardlinks and symlinks by default on Guix System, which will
protect against this class of vulnerability from now on.


Credit
~~~~~~

We are grateful to Nathan Nye of WhiteBeam Security for reporting this
bug and discussing fixes with us!


Timeline
~~~~~~~~

We learned about this bug on the private guix-security@gnu.org list on
February 7th, and discussed and prepared fixes in the interim.

Ludo’ & Leo Famulari.

[Message part 2 (text/x-patch, inline)]

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 20d83fea4a..4f486f0822 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -1621,6 +1621,24 @@ void DerivationGoal::startBuilder()
     auto drvName = storePathToName(drvPath);
     tmpDir = createTempDir("", "guix-build-" + drvName, false, false, 0700);
 
+    if (useChroot) {
+	/* Make the build directory seen by the build process a sub-directory.
+	   That way, "/tmp/guix-build-foo.drv-0" is root-owned, and thus its
+	   permissions cannot be changed by the build process, while
+	   "/tmp/guix-build-foo.drv-0/top" is owned by the build user.  This
+	   cannot be done when !useChroot because then $NIX_BUILD_TOP would
+	   be inaccessible to the build user by its full file name.
+
+	   If the build user could make the build directory world-writable,
+	   then an attacker could create in it a hardlink to a root-owned file
+	   such as /etc/shadow.  If 'keepFailed' is true, the daemon would
+	   then chown that hardlink to the user, giving them write access to
+	   that file.  */
+	tmpDir += "/top";
+	if (mkdir(tmpDir.c_str(), 0700) == 1)
+	    throw SysError("creating top-level build directory");
+    }
+
     /* In a sandbox, for determinism, always use the same temporary
        directory. */
     tmpDirInSandbox = useChroot ? canonPath("/tmp", true) + "/guix-build-" + drvName + "-0" : tmpDir;
@@ -2626,20 +2644,41 @@ static void _chown(const Path & path, uid_t uid, gid_t gid)
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
+	// When useChroot is true, tmpDir looks like
+	// "/tmp/guix-build-foo.drv-0/top".  Its parent is root-owned.
+	string top;
+	if (useChroot) {
+	    if (baseNameOf(tmpDir) != "top") abort();
+	    top = dirOf(tmpDir);
+	} else top = tmpDir;
+
         if (settings.keepFailed && !force) {
             printMsg(lvlError,
                 format("note: keeping build directory `%2%'")
-                % drvPath % tmpDir);
+                % drvPath % top);
             chmod(tmpDir.c_str(), 0755);
+
             // Change the ownership if clientUid is set. Never change the
             // ownership or the group to "root" for security reasons.
             if (settings.clientUid != (uid_t) -1 && settings.clientUid != 0) {
                 _chown(tmpDir, settings.clientUid,
                        settings.clientGid != 0 ? settings.clientGid : -1);
+
+		if (top != tmpDir) {
+		    // Rename tmpDir to its parent, with an intermediate step.
+		    string pivot = top + ".pivot";
+		    if (rename(top.c_str(), pivot.c_str()) == -1)
+			throw SysError("pivoting failed build tree");
+		    if (rename((pivot + "/top").c_str(), top.c_str()) == -1)
+			throw SysError("renaming failed build tree");
+		    rmdir(pivot.c_str());
+		}
             }
         }
-        else
+        else {
             deletePath(tmpDir);
+	    if (top != tmpDir) rmdir(dirOf(tmpDir).c_str());
+	}
         tmpDir = "";
     }
 }

Send a report that this bug log contains spam.