bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1.1: Type: text/plain, Size: 84 bytes --]

Patch is attached.
The committer will need to change the commit id appropriately.

[-- Attachment #1.2: 0001-activation-Do-not-dereference-symlinks-in-home-direc.patch --]
[-- Type: text/x-patch, Size: 2549 bytes --]

From 9672bd37bf50db1e0989d0b84035c4788422bd31 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Tue, 30 Mar 2021 22:36:14 +0200
Subject: [PATCH 1/2] activation: Do not dereference symlinks in home directory
 creation.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes <https://bugs.gnu.org/47584>.

* gnu/build/activation.scm
  (copy-account-skeletons): Do not chown the home directory; leave this
  to 'activate-user-home'.
  (activate-user-home): Only chown the home directory after the account
  skeletons have been copied.

Co-authored-by: Ludovic Courtès <ludo@gnu.org>.
---
 gnu/build/activation.scm | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index 6cb6f8819b..43d973d3da 100644
--- a/gnu/build/activation.scm
+++ b/gnu/build/activation.scm
@@ -107,7 +107,8 @@ Warning: this is currently suspect to a TOCTTOU race!"
                                  (directory %skeleton-directory)
                                  uid gid)
   "Copy the account skeletons from DIRECTORY to HOME.  When UID is an integer,
-make it the owner of all the files created; likewise for GID."
+make it the owner of all the files created except the home directory; likewise
+for GID."
   (define (set-owner file)
     (when (or uid gid)
       (chown file (or uid -1) (or gid -1))))
@@ -115,7 +116,6 @@ make it the owner of all the files created; likewise for GID."
   (let ((files (scandir directory (negate dot-or-dot-dot?)
                         string<?)))
     (mkdir-p home)
-    (set-owner home)
     (for-each (lambda (file)
                 (let ((target (string-append home "/" file)))
                   (copy-recursively (string-append directory "/" file)
@@ -215,10 +215,14 @@ they already exist."
                  (uid (passwd:uid pw))
                  (gid (passwd:gid pw)))
             (mkdir-p home)
-            (chown home uid gid)
             (chmod home #o700)
             (copy-account-skeletons home
-                                    #:uid uid #:gid gid))))))
+                                    #:uid uid #:gid gid)
+            ;; It is important 'chown' is called after 'copy-account-skeletons'
+            ;; Otherwise, a malicious user with good timing could
+            ;; create a symlink in HOME that would be dereferenced by
+            ;; 'copy-account-skeletons'.
+            (chown home uid gid))))))
 
   (for-each ensure-user-home users))
 
-- 
2.31.1


[-- Attachment #1.3: 0002-news-Add-entry-for-user-account-activation-vulnerabi.patch --]
[-- Type: text/x-patch, Size: 2045 bytes --]

From d071ee3aff5be1a6d7876d7411e70f7283dce1fb Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sat, 3 Apr 2021 12:19:10 +0200
Subject: [PATCH 2/2] news: Add entry for user account activation
 vulnerability.

TODO for guix committer: correct the commit id appropriately.

* etc/news.scm: Add entry.
---
 etc/news.scm | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/etc/news.scm b/etc/news.scm
index deedc69f6e..0cc9c183a0 100644
--- a/etc/news.scm
+++ b/etc/news.scm
@@ -12,6 +12,7 @@
 ;; Copyright © 2020, 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;; Copyright © 2021 Leo Famulari <leo@famulari.name>
 ;; Copyright © 2021 Zhu Zihao <all_but_last@163.com>
+;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
 ;;
 ;; Copying and distribution of this file, with or without modification, are
 ;; permitted in any medium without royalty provided the copyright notice and
@@ -20,6 +21,22 @@
 (channel-news
  (version 0)
 
+ ;; XXX to guix committers: this commit likely needs to be changed.
+ (entry (commit "9672bd37bf50db1e0989d0b84035c4788422bd31")
+        (title
+         (en "Risk of local privilege escalation by creation of new user accounts"))
+        (body
+         (en "A security vulnerability that can lead to local privilege
+escalation has been found in the activation code of user accounts.  The
+system is only vulnerable during the activation of user accounts (including
+system accounts) that do not already exist.
+
+The attack consists of the user logging in after the user's home directory
+has been created, but before the activation of the user has been completed,
+by creating an appropriately named symbolic link in the home directory
+pointing to a sensitive file, such as @file{/etc/shadow}.
+
+See @uref{https://issues.guix.gnu.org/47584} for more information on this bug.")))
  (entry (commit "9ade2b720af91acecf76278b4d9b99ace406781e")
         (title
          (en "Update on previous @command{guix-daemon} local privilege escalation")
-- 
2.31.1


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]