From debbugs-submit-bounces@debbugs.gnu.org Wed Oct 16 06:22:55 2019 Received: (at 37744) by debbugs.gnu.org; 16 Oct 2019 10:22:55 +0000 Received: from localhost ([127.0.0.1]:45265 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iKgS8-0001uI-M9 for submit@debbugs.gnu.org; Wed, 16 Oct 2019 06:22:54 -0400 Received: from eggs.gnu.org ([209.51.188.92]:54090) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iKgS6-0001u5-6k for 37744@debbugs.gnu.org; Wed, 16 Oct 2019 06:22:47 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:39205) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1iKgRx-0003kS-Tt; Wed, 16 Oct 2019 06:22:38 -0400 Received: from no3.u-bordeaux.fr ([147.210.245.180]:37774 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1iKgRw-0002vc-UR; Wed, 16 Oct 2019 06:22:37 -0400 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: Tobias Geerinckx-Rice Subject: Re: bug#37744: Per-user profile directory hijack (CVE-2019-17365 for Nix) References: <87o8yjsr8o.fsf@gnu.org> <87blujsqq0.fsf@gnu.org> <87y2xno85o.fsf@nckx> <87d0eyuqzd.fsf@gnu.org> <87mue2nkrj.fsf@nckx> <8736fttby6.fsf@gnu.org> Date: Wed, 16 Oct 2019 12:22:33 +0200 In-Reply-To: <8736fttby6.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Wed, 16 Oct 2019 08:57:05 +0200") Message-ID: <87tv89rnva.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 37744 Cc: 37744@debbugs.gnu.org, guix-security@gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hello! Here=E2=80=99s a patch that fixes the issue, partly based on what the Nix f= olks did. For the client-connecting-over-TCP case, I added special handling: =E2=80=98set-build-options=E2=80=99 now passes a =E2=80=9Cuser-name=E2=80= =9D property, potentially allowing to create =E2=80=98per-user/$USER=E2=80=99 at that point (like you= suggested, Tobias.) In a cluster setup, it means that the machine that runs =E2=80=98guix-daemo= n=E2=80=99 must see the same users as the machines where its clients run, but that=E2=80=99s basically already what we expect: . There=E2=80=99s one case that won=E2=80=99t be correctly handled: in a clus= ter setup, an old client talking to a new daemon won=E2=80=99t provide info to create =E2=80=98per-user/$USER=E2=80=99, and thus =E2=80=98guix package=E2=80=99 &= co. won=E2=80=99t be able to create the user=E2=80=99s profile it it doesn=E2=80=99t already exist. I think th= at=E2=80=99s hard to avoid though. Thoughts? Thanks, Ludo=E2=80=99. --=-=-= Content-Type: text/x-patch; charset=utf-8 Content-Disposition: inline; filename=0001-daemon-Make-profiles-per-user-non-world-writable.patch Content-Transfer-Encoding: quoted-printable From 7c43fdeb2f9283d86d849007e8fbc138ca2912c4 Mon Sep 17 00:00:00 2001 From: =3D?UTF-8?q?Ludovic=3D20Court=3DC3=3DA8s?=3D Date: Wed, 16 Oct 2019 11:51:42 +0200 Subject: [PATCH 1/2] daemon: Make 'profiles/per-user' non-world-writable. Fixes . Reported at . Based on Nix commit 5a303093dcae1e5ce9212616ef18f2ca51020b0d by Eelco Dolstra . * 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")) =20 (define (ensure-profile-directory) - "Attempt to create /=E2=80=A6/profiles/per-user/$USER if needed." + "Attempt to create /=E2=80=A6/profiles/per-user/$USER if needed. Nowada= ys 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"))) =20 +(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)) =20 ;; When true, provide machine-readable "build ;; traces" for use by (guix status). Old clie= nts @@ -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) =20 Path perUserDir =3D profilesDir + "/per-user"; createDirs(perUserDir); - if (chmod(perUserDir.c_str(), 01777) =3D=3D -1) - throw SysError(format("could not set permissions on '%1%' to 1= 777") % perUserDir); + if (chmod(perUserDir.c_str(), 0755) =3D=3D -1) + throw SysError(format("could not set permissions on '%1%' to 7= 55") + % perUserDir); =20 mode_t perm =3D 01775; =20 @@ -1642,4 +1643,16 @@ void LocalStore::vacuumDB() } =20 =20 +void LocalStore::createUser(const std::string & userName, uid_t userId) +{ + auto dir =3D settings.nixStateDir + "/profiles/per-user/" + userName; + + createDirs(dir); + if (chmod(dir.c_str(), 0755) =3D=3D -1) + throw SysError(format("changing permissions of directory '%s'") % dir); + if (chown(dir.c_str(), userId, -1) =3D=3D -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: =20 void setSubstituterEnv(); =20 + void createUser(const std::string & userName, uid_t userId); + private: =20 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) =3D 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) = =3D 0; }; =20 =20 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 clien= tVersion, || name =3D=3D "build-repeat" || name =3D=3D "multiplexed-build-output") settings.set(name, value); + else if (name =3D=3D "user-name" + && settings.clientUid =3D=3D (uid_t) -1) { + /* Create the user profile. This is necessary if + clientUid =3D -1, for instance because the client + connected over TCP. */ + struct passwd *pw =3D getpwnam(value.c_str()); + if (pw !=3D NULL) + store->createUser(value, pw->pw_uid); + else + printMsg(lvlInfo, format("user name %1% not found"= ) % value); + } else settings.set(trusted ? name : "untrusted-" + name, val= ue); } @@ -731,7 +742,7 @@ static void performOp(bool trusted, unsigned int client= Version, } =20 =20 -static void processConnection(bool trusted) +static void processConnection(bool trusted, uid_t userId) { canSendStderr =3D false; _writeToStderr =3D tunnelStderr; @@ -778,6 +789,15 @@ static void processConnection(bool trusted) /* Open the store. */ store =3D std::shared_ptr(new LocalStore(reserveSpace)); =20 + if (userId !=3D (uid_t) -1) { + /* Create the user profile. */ + struct passwd *pw =3D getpwuid(userId); + if (pw !=3D NULL && pw->pw_name !=3D NULL) + store->createUser(pw->pw_name, userId); + else + printMsg(lvlInfo, format("user with UID %1% not found") % = userId); + } + stopWork(); to.flush(); =20 @@ -963,7 +983,7 @@ static void acceptConnection(int fdSocket) /* Handle the connection. */ from.fd =3D remote; to.fd =3D remote; - processConnection(trusted); + processConnection(trusted, clientUid); =20 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 =20 kill "$daemon_pid" =20 +# Make sure 'profiles/per-user' is created when connecting over TCP. + +orig_GUIX_STATE_DIRECTORY=3D"$GUIX_STATE_DIRECTORY" +GUIX_STATE_DIRECTORY=3D"$GUIX_STATE_DIRECTORY-2" + +guix-daemon --disable-chroot --listen=3D"localhost:9877" & +daemon_pid=3D$! + +GUIX_DAEMON_SOCKET=3D"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=3D"$orig_GUIX_STATE_DIRECTORY" + # Check the failed build cache. =20 guix-daemon --no-substitutes --listen=3D"$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 @@ =20 (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))))) =20 -(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)))) =20 (test-equal "add-data-to-store" #vu8(1 2 3 4 5) --=20 2.23.0 --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0002-DRAFT-news-Add-entry-for-security-issue-with-var-gui.patch From 07126db581f1854a2235c271fcdaecfb36705d5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 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 --=-=-=--