bug#37744: Per-user profile directory hijack (CVE-2019-17365 for Nix)

[Ludovic Courtès]

Hello!

Here’s a patch that fixes the issue, partly based on what the Nix folks
did.

For the client-connecting-over-TCP case, I added special handling:
‘set-build-options’ now passes a “user-name” property, potentially
allowing to create ‘per-user/$USER’ at that point (like you suggested,
Tobias.)

In a cluster setup, it means that the machine that runs ‘guix-daemon’
must see the same users as the machines where its clients run, but
that’s basically already what we expect:
<https://hpc.guix.info/blog/2017/11/installing-guix-on-a-cluster/>.

There’s one case that won’t be correctly handled: in a cluster setup, an
old client talking to a new daemon won’t provide info to create
‘per-user/$USER’, and thus ‘guix package’ & co. won’t be able to create
the user’s profile it it doesn’t already exist.  I think that’s hard to
avoid though.

Thoughts?

Thanks,
Ludo’.

>From 7c43fdeb2f9283d86d849007e8fbc138ca2912c4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <address@hidden>
Date: Wed, 16 Oct 2019 11:51:42 +0200
Subject: [PATCH 1/2] daemon: Make 'profiles/per-user' non-world-writable.

Fixes <https://bugs.gnu.org/37744>.
Reported at <https://www.openwall.com/lists/oss-security/2019/10/09/4>.

Based on Nix commit 5a303093dcae1e5ce9212616ef18f2ca51020b0d
by Eelco Dolstra <address@hidden>.

* nix/libstore/local-store.cc (LocalStore::LocalStore): Set 'perUserDir'
to #o755 instead of #o1777.
(LocalStore::createUser): New function.
* nix/libstore/local-store.hh (LocalStore): Add it.
* nix/libstore/store-api.hh (StoreAPI): Add it.
* nix/nix-daemon/nix-daemon.cc (performOp): In 'wopSetOptions', add
condition to handle "user-name" property and honor it.
(processConnection): Add 'userId' parameter.  Call 'store->createUser'
when userId is not -1.
* guix/profiles.scm (ensure-profile-directory): Note that this is now
handled by the daemon.
* guix/store.scm (current-user-name): New procedure.
(set-build-options): Add #:user-name parameter and pass it to the daemon.
* tests/guix-daemon.sh: Test the creation of 'profiles/per-user' when
listening on a TCP socket.
* tests/store.scm ("profiles/per-user exists and is not writable")
("profiles/per-user/$USER exists"): New tests.
---
 guix/profiles.scm            |  3 ++-
 guix/store.scm               | 12 ++++++++++++
 nix/libstore/local-store.cc  | 17 +++++++++++++++--
 nix/libstore/local-store.hh  |  2 ++
 nix/libstore/store-api.hh    |  4 ++++
 nix/nix-daemon/nix-daemon.cc | 24 ++++++++++++++++++++++--
 tests/guix-daemon.sh         | 21 +++++++++++++++++++++
 tests/store.scm              | 13 ++++++++++++-
 8 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/guix/profiles.scm b/guix/profiles.scm
index f5c863945c..cd3b21e390 100644
--- a/guix/profiles.scm
+++ b/guix/profiles.scm
@@ -1732,7 +1732,8 @@ because the NUMBER is zero.)"
   (string-append %profile-directory "/guix-profile"))
 
 (define (ensure-profile-directory)
-  "Attempt to create /…/profiles/per-user/$USER if needed."
+  "Attempt to create /…/profiles/per-user/$USER if needed.  Nowadays this is
+taken care of by the daemon."
   (let ((s (stat %profile-directory #f)))
     (unless (and s (eq? 'directory (stat:type s)))
       (catch 'system-error
diff --git a/guix/store.scm b/guix/store.scm
index d7c603898c..382aad29d9 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -748,6 +748,14 @@ encoding conversion errors."
            (cut string-append "http://"; <>))
        '("ci.guix.gnu.org")))
 
+(define (current-user-name)
+  "Return the name of the calling user."
+  (catch #t
+    (lambda ()
+      (passwd:name (getpwuid (getuid))))
+    (lambda _
+      (getenv "USER"))))
+
 (define* (set-build-options server
                             #:key keep-failed? keep-going? fallback?
                             (verbosity 0)
@@ -759,6 +767,7 @@ encoding conversion errors."
                             (build-verbosity 0)
                             (log-type 0)
                             (print-build-trace #t)
+                            (user-name (current-user-name))
 
                             ;; When true, provide machine-readable "build
                             ;; traces" for use by (guix status).  Old clients
@@ -849,6 +858,9 @@ encoding conversion errors."
                            `(("build-repeat"
                               . ,(number->string (max 0 (1- rounds)))))
                            '())
+                     ,@(if user-name
+                           `(("user-name" . ,user-name))
+                           '())
                      ,@(if terminal-columns
                            `(("terminal-columns"
                               . ,(number->string terminal-columns)))
diff --git a/nix/libstore/local-store.cc b/nix/libstore/local-store.cc
index 3b08492c64..3793382361 100644
--- a/nix/libstore/local-store.cc
+++ b/nix/libstore/local-store.cc
@@ -88,8 +88,9 @@ LocalStore::LocalStore(bool reserveSpace)
 
         Path perUserDir = profilesDir + "/per-user";
         createDirs(perUserDir);
-        if (chmod(perUserDir.c_str(), 01777) == -1)
-            throw SysError(format("could not set permissions on '%1%' to 
1777") % perUserDir);
+        if (chmod(perUserDir.c_str(), 0755) == -1)
+            throw SysError(format("could not set permissions on '%1%' to 755")
+                           % perUserDir);
 
         mode_t perm = 01775;
 
@@ -1642,4 +1643,16 @@ void LocalStore::vacuumDB()
 }
 
 
+void LocalStore::createUser(const std::string & userName, uid_t userId)
+{
+    auto dir = settings.nixStateDir + "/profiles/per-user/" + userName;
+
+    createDirs(dir);
+    if (chmod(dir.c_str(), 0755) == -1)
+       throw SysError(format("changing permissions of directory '%s'") % dir);
+    if (chown(dir.c_str(), userId, -1) == -1)
+       throw SysError(format("changing owner of directory '%s'") % dir);
+}
+
+
 }
diff --git a/nix/libstore/local-store.hh b/nix/libstore/local-store.hh
index 4113fafcb5..2e48cf03e6 100644
--- a/nix/libstore/local-store.hh
+++ b/nix/libstore/local-store.hh
@@ -180,6 +180,8 @@ public:
 
     void setSubstituterEnv();
 
+    void createUser(const std::string & userName, uid_t userId);
+
 private:
 
     Path schemaPath;
diff --git a/nix/libstore/store-api.hh b/nix/libstore/store-api.hh
index 2d9dcbd573..7d2ad2270d 100644
--- a/nix/libstore/store-api.hh
+++ b/nix/libstore/store-api.hh
@@ -289,6 +289,10 @@ public:
     /* Check the integrity of the Nix store.  Returns true if errors
        remain. */
     virtual bool verifyStore(bool checkContents, bool repair) = 0;
+
+    /* Create a profile for the given user.  This is done by the daemon
+       because the 'profiles/per-user' directory is not writable by users.  */
+    virtual void createUser(const std::string & userName, uid_t userId) = 0;
 };
 
 
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 1163a249d1..3dd156ba77 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -613,6 +613,17 @@ static void performOp(bool trusted, unsigned int 
clientVersion,
                     || name == "build-repeat"
                     || name == "multiplexed-build-output")
                     settings.set(name, value);
+               else if (name == "user-name"
+                         && settings.clientUid == (uid_t) -1) {
+                    /* Create the user profile.  This is necessary if
+                       clientUid = -1, for instance because the client
+                       connected over TCP.  */
+                    struct passwd *pw = getpwnam(value.c_str());
+                    if (pw != NULL)
+                        store->createUser(value, pw->pw_uid);
+                    else
+                        printMsg(lvlInfo, format("user name %1% not found") % 
value);
+               }
                 else
                     settings.set(trusted ? name : "untrusted-" + name, value);
             }
@@ -731,7 +742,7 @@ static void performOp(bool trusted, unsigned int 
clientVersion,
 }
 
 
-static void processConnection(bool trusted)
+static void processConnection(bool trusted, uid_t userId)
 {
     canSendStderr = false;
     _writeToStderr = tunnelStderr;
@@ -778,6 +789,15 @@ static void processConnection(bool trusted)
         /* Open the store. */
         store = std::shared_ptr<StoreAPI>(new LocalStore(reserveSpace));
 
+       if (userId != (uid_t) -1) {
+            /* Create the user profile.  */
+            struct passwd *pw = getpwuid(userId);
+            if (pw != NULL && pw->pw_name != NULL)
+                store->createUser(pw->pw_name, userId);
+            else
+                printMsg(lvlInfo, format("user with UID %1% not found") % 
userId);
+       }
+
         stopWork();
         to.flush();
 
@@ -963,7 +983,7 @@ static void acceptConnection(int fdSocket)
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-                processConnection(trusted);
+                processConnection(trusted, clientUid);
 
                 exit(0);
             }, false, "unexpected build daemon error: ", true);
diff --git a/tests/guix-daemon.sh b/tests/guix-daemon.sh
index 758f18cc36..b58500966b 100644
--- a/tests/guix-daemon.sh
+++ b/tests/guix-daemon.sh
@@ -94,6 +94,27 @@ done
 
 kill "$daemon_pid"
 
+# Make sure 'profiles/per-user' is created when connecting over TCP.
+
+orig_GUIX_STATE_DIRECTORY="$GUIX_STATE_DIRECTORY"
+GUIX_STATE_DIRECTORY="$GUIX_STATE_DIRECTORY-2"
+
+guix-daemon --disable-chroot --listen="localhost:9877" &
+daemon_pid=$!
+
+GUIX_DAEMON_SOCKET="guix://localhost:9877"
+export GUIX_DAEMON_SOCKET
+
+test ! -d "$GUIX_STATE_DIRECTORY/profiles/per-user"
+
+guix build guile-bootstrap -d
+
+test -d "$GUIX_STATE_DIRECTORY/profiles/per-user/$USER"
+
+kill "$daemon_pid"
+unset GUIX_DAEMON_SOCKET
+GUIX_STATE_DIRECTORY="$orig_GUIX_STATE_DIRECTORY"
+
 # Check the failed build cache.
 
 guix-daemon --no-substitutes --listen="$socket" --disable-chroot       \
diff --git a/tests/store.scm b/tests/store.scm
index 518750d26a..2b14a4af0a 100644
--- a/tests/store.scm
+++ b/tests/store.scm
@@ -18,6 +18,7 @@
 
 (define-module (test-store)
   #:use-module (guix tests)
+  #:use-module (guix config)
   #:use-module (guix store)
   #:use-module (guix utils)
   #:use-module (guix monads)
@@ -102,7 +103,17 @@
               "/283gqy39v3g9dxjy26rynl0zls82fmcg-guile-2.0.7/bin/guile")))
        (not (direct-store-path? (%store-prefix)))))
 
-(test-skip (if %store 0 13))
+(test-skip (if %store 0 15))
+
+(test-equal "profiles/per-user exists and is not writable"
+  #o755
+  (stat:perms (stat (string-append %state-directory "/profiles/per-user"))))
+
+(test-equal "profiles/per-user/$USER exists"
+  (list (getuid) #o755)
+  (let ((s (stat (string-append %state-directory "/profiles/per-user/"
+                                (passwd:name (getpwuid (getuid)))))))
+    (list (stat:uid s) (stat:perms s))))
 
 (test-equal "add-data-to-store"
   #vu8(1 2 3 4 5)
-- 
2.23.0

>From 07126db581f1854a2235c271fcdaecfb36705d5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <address@hidden>
Date: Wed, 16 Oct 2019 12:16:20 +0200
Subject: [PATCH 2/2] DRAFT news: Add entry for security issue with
 /var/guix/profiles/per-user.

DRAFT: Update commit before pushing.

* etc/news.scm: Add entry for security issue in multi-user setups.
---
 etc/news.scm | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/etc/news.scm b/etc/news.scm
index e19dec38dd..afcf5fadaa 100644
--- a/etc/news.scm
+++ b/etc/news.scm
@@ -9,6 +9,27 @@
 (channel-news
  (version 0)
 
+ (entry (commit "FIXME")
+        (title (en "Security issue with profiles in multi-user setups"))
+        (body
+         (en "The default user profile, @file{~/.guix-profile}, points to
+@file{/var/guix/profiles/per-user/$USER}.  Until now,
+@file{/var/guix/profiles/per-user} was world-writable, allowing the
+@command{guix} command to create the @code{$USER} sub-directory.
+
+On a multi-user system, this allowed a malicious user to create and populate
+that @code{$USER} sub-directory for another user that had not yet logged in.
+Since @code{$USER} is in @code{$PATH}, the target user could end up running
+attacker-provided code.  See @uref{https://issues.guix.gnu.org/issue/37744}
+for more information.
+
+This is now fixed by letting @command{guix-daemon} create these directories on
+behalf of users and removing the world-writable permissions on
+@code{per-user}.  On multi-user systems, we recommend updating the daemon now.
+To do that, run @code{sudo guix pull} if you're on a foreign distro, or run
+@code{sudo guix pull && sudo guix system reconfigure @dots{}} on Guix
+System.")))
+
  (entry (commit "5f3f70391809f8791c55c05bd1646bc58508fa2c")
         (title (en "GNU C Library upgraded")
                (de "GNU-C-Bibliothek aktualisiert")
-- 
2.23.0