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

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

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

A TOCTTOU (time-of-check to time-of-use) vulnerability has been found
in the activation code of user accounts, more specifically in the
code that copies the account skeletons.

* Vulnerability

The attack consists of the user being logged in after the account
skeletons have been copied to the home directory, but before the
owner of the account skeletons have been set.  The user then deletes
a copied account skeleton (e.g. @file{$HOME/.gdbinit}) and replaces
it with a symbolic link to a file not owned by the user, such as
@file{/etc/shadow}.

The activation code then changes the ownership
of the file the symbolic link points to instead of the symbolic
link itself.  At that point, the user has read-write access
to the target file.

* Where in the code does this happen?

Module: (gnu build activation).
Procedures: 'copy-account-skeletons' and 'activate-user-home'.

'copy-account-skeletons' creates the home directory, sets it
owner, copies the account skeletons, and chowns the copied skeletons,
in that order.   The bug is that it dereferences symbolic links.

It is called from 'activate-user-home' if the home directory does
not already exist.

* Fix

The fix consist of initially creating the home directory root-owned and only
changing the owner of the home directory once all skeletons have been copied
and their owner has been set.

* Extra notes

A blog post, a news entry and a fix have been prepared and will be posted
and hopefully merged soon.  The following tests succeeded:

$ make check-system TESTS='switch-to-system upgrade-services install-bootloader basic'
$ make check

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

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

[Maxime Devos]

[-- 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 --]

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

[Maxime Devos]

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

A suggested blog post is attached.

[-- Attachment #1.2: 0001-website-Add-post-about-vulnerability-in-copy-account.patch --]
[-- Type: text/x-patch, Size: 5137 bytes --]

From 7937b9f18085569e5d7cb8a3c4dc08e1088a94a9 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sat, 3 Apr 2021 18:02:05 +0200
Subject: [PATCH] =?UTF-8?q?website:=20Add=20post=20about=20vulnerability?=
 =?UTF-8?q?=20in=20=E2=80=98copy-account-skeletons=E2=80=99.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* website/posts/home-symlink.md: New post.
---
 website/posts/home-symlink.md | 103 ++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 website/posts/home-symlink.md

diff --git a/website/posts/home-symlink.md b/website/posts/home-symlink.md
new file mode 100644
index 0000000..9289870
--- /dev/null
+++ b/website/posts/home-symlink.md
@@ -0,0 +1,103 @@
+title: Risk of local privilege escalation in account creation
+date: 2021-04-03 17:30
+author: Maxime Devos
+tags: Security Advisory
+---
+
+A security vulnerability that can lead to local privilege escalation
+has been found in the activation code of user accounts (excluding
+system accounts).  It does not affect users on foreign distros
+and is only exploitable during system reconfiguration.
+
+This exploit is _not_ impossible on machines where the Linux [protected
+symlinks](https://sysctl-explorer.net/fs/protected_symlinks/) feature
+is enabled.  It is believed the attack can also be performed using hard
+links.
+
+# Vulnerability
+
+The attack consists of the user being logged in after the account
+skeletons have been copied to the home directory, but before the
+owner of the account skeletons have been set.  The user then deletes
+a copied account skeleton (e.g. `$HOME/.gdbinit`) and replaces
+it with a symbolic link to a file not owned by the user, such as
+`/etc/shadow`.
+
+The activation code then changes the ownership of the file the symbolic
+link points to instead of the symbolic link itself.  At that point, the
+user has read-write access to the target file.
+
+# Fix
+
+This [bug](https://issues.guix.gnu.org/47584) has been
+<!-- XXX insert the commit id -->
+[fixed](https://git.savannah.gnu.org/cgit/guix.git/commit/?id= XXX).
+See below for upgrade instructions.
+
+The fix consist of initially creating the home directory root-owned and only
+changing the owner of the home directory once all skeletons have been copied
+and their owner has been set.
+
+# Upgrading
+
+To upgrade the Guix System, run something like:
+
+```
+guix pull
+sudo guix system reconfigure /run/current-system/configuration.scm
+sudo reboot
+```
+
+As the user account activation code is run as a shepherd service,
+the last step is required to make sure the fixed activation code
+is run in the future.
+
+To avoid the vulnerability while upgrading the system, only declare
+new user accounts in the configuration file after the Guix System
+has been upgraded.
+
+# Conclusions
+
+The activation code in Guix System originally was written with the
+assumption that no other code was running at the same time in mind.
+However, this is not a reasonable assumption in practice, as this
+vulnerability demonstrates.  Thus, it may be worthwhile to look
+over other activation code for similar issues.
+
+While investigating how to fix the issue, it became apparent GNU Guile,
+the implementation of the Algorithmic Language Scheme GNU Guix is
+written in, is lacking in primitives that usually are used to avoid
+these kind of issues, such `openat` and `O_NOFOLLOW`.
+
+While these primitives turned out not to be necessary to fix the
+issue and a [patch series](<https://lists.gnu.org/archive/html/guile-devel/2021-03/msg00026.html>)
+to GNU Guile has been submitted that adds these primitives, this does
+serve as a remainder that GNU Guile is a critical component of
+Guix System and working around missing primitives will not always be possible.
+
+This issue is tracked as
+[bug #47584](https://issues.guix.gnu.org/47584); you can read the thread
+for more information.
+
+Please report any issues you may have to
+[`guix-devel@gnu.org`](https://guix.gnu.org/en/contact/).  See the
+[security web page](https://guix.gnu.org/en/security/) for information
+on how to report security issues.
+
+#### About GNU Guix
+
+[GNU Guix](https://guix.gnu.org) is a transactional package manager and
+an advanced distribution of the GNU system that [respects user
+freedom](https://www.gnu.org/distros/free-system-distribution-guidelines.html).
+Guix can be used on top of any system running the Hurd or the Linux
+kernel, or it can be used as a standalone operating system distribution
+for i686, x86_64, ARMv7, and AArch64 machines.
+
+In addition to standard package management features, Guix supports
+transactional upgrades and roll-backs, unprivileged package management,
+per-user profiles, and garbage collection.  When used as a standalone
+GNU/Linux distribution, Guix offers a declarative, stateless approach to
+operating system configuration management.  Guix is highly customizable
+and hackable through [Guile](https://www.gnu.org/software/guile)
+programming interfaces and extensions to the
+[Scheme](http://schemers.org) language.
-- 
2.31.1


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

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

[Maxime Devos]

On Sat, 2021-04-03 at 18:22 +0200, Maxime Devos wrote:
> +            ;; 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'.

Oops please add a period after 'copy-account-skeletons';

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

[Ludovic Courtès]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> 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>.

Pushed:

  https://git.savannah.gnu.org/cgit/guix.git/commit/?id=2161820ebbbab62a5ce76c9101ebaec54dc61586

> 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.

I tweaked it to (1) make it clear upfront that only Guix System is
affected, (2) to explicitly recommend an upgrade on Guix System, and (3)
to clarify when the attack can happen.

Thanks for finding the issue, for reporting it at guix-security, and for
preparing these patches!

Ludo’.

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

[Ludovic Courtès]

Note that this issue is about Guix System; users of Guix on other
distros are unaffected.

Maxime Devos <maximedevos@telenet.be> skribis:

> The attack consists of the user being logged in after the account
> skeletons have been copied to the home directory, but before the
> owner of the account skeletons have been set.  The user then deletes
> a copied account skeleton (e.g. @file{$HOME/.gdbinit}) and replaces
> it with a symbolic link to a file not owned by the user, such as
> @file{/etc/shadow}.
>
> The activation code then changes the ownership
> of the file the symbolic link points to instead of the symbolic
> link itself.  At that point, the user has read-write access
> to the target file.

To give a bit more context, account creation on Guix System happens
while ‘guix system reconfigure’ is running.

The user whose account is being created thus needs to be able to log in
right during the time window described above.

Users whose password is uninitialized (i.e., the ‘password’ field of
<user-account> is left unspecified¹) cannot log in at that point, unless
possibly if the OpenSSH configuration specifies an authorized key for
the user account.

Ludo’.

¹ https://guix.gnu.org/manual/en/html_node/User-Accounts.html
² https://guix.gnu.org/manual/en/html_node/Networking-Services.html#index-openssh_002dservice_002dtype

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

[Ludovic Courtès]

Maxime Devos <maximedevos@telenet.be> skribis:

> The attack consists of the user being logged in after the account
> skeletons have been copied to the home directory, but before the
> owner of the account skeletons have been set.  The user then deletes
> a copied account skeleton (e.g. @file{$HOME/.gdbinit}) and replaces
> it with a symbolic link to a file not owned by the user, such as
> @file{/etc/shadow}.
>
> The activation code then changes the ownership
> of the file the symbolic link points to instead of the symbolic
> link itself.  At that point, the user has read-write access
> to the target file.

In the draft blog post, you mention that the attack cannot be carried
out when protected symlinks are enabled.  This is now the case by
default on Guix System¹, so in that case, a system upgraded from a
commit after March 16th is unaffected.

Ludo’.

¹ https://issues.guix.gnu.org/47013#13

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

[Ludovic Courtès]

Maxime Devos <maximedevos@telenet.be> skribis:

> From 7937b9f18085569e5d7cb8a3c4dc08e1088a94a9 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Sat, 3 Apr 2021 18:02:05 +0200
> Subject: [PATCH] =?UTF-8?q?website:=20Add=20post=20about=20vulnerability?=
>  =?UTF-8?q?=20in=20=E2=80=98copy-account-skeletons=E2=80=99.?=
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> * website/posts/home-symlink.md: New post.

It’s unfortunate that this is going out during a week-end, and a
three-day week-end on top of that in some regions of the world, with
many people not seeing the message and not being able to act upon it for
three days.

> +title: Risk of local privilege escalation in account creation
> +date: 2021-04-03 17:30
> +author: Maxime Devos
> +tags: Security Advisory
> +---
> +
> +A security vulnerability that can lead to local privilege escalation
> +has been found in the activation code of user accounts (excluding
> +system accounts).  It does not affect users on foreign distros
> +and is only exploitable during system reconfiguration.

How about this, taken from the news.scm entry I tweaked:

  A security vulnerability that can lead to local privilege
  escalation has been found in the code that creates user accounts on Guix
  System—Guix on other distros is unaffected.  The system is only vulnerable
  during the activation of non-system user accounts that do not already exist.

(This is more upfront about who’s affected and avoids the technical term
“activation code” which makes no sense outside the circle of Guix System
and NixOS hackers.)

> +This exploit is _not_ impossible on machines where the Linux [protected
> +symlinks](https://sysctl-explorer.net/fs/protected_symlinks/) feature
> +is enabled.  It is believed the attack can also be performed using hard
> +links.

Please mention that protected symlinks are enabled by default on Guix
System since a March 16th commit, with a link to
<https://issues.guix.gnu.org/47013#13>.

> +# Conclusions
> +
> +The activation code in Guix System originally was written with the
> +assumption that no other code was running at the same time in mind.
> +However, this is not a reasonable assumption in practice, as this
> +vulnerability demonstrates.  Thus, it may be worthwhile to look
> +over other activation code for similar issues.

That’s an interesting conclusion for us developers, but not necessarily
for the users this is targeting.  It also sounds unnecessarily scary and
casual.

> +While investigating how to fix the issue, it became apparent GNU Guile,
> +the implementation of the Algorithmic Language Scheme GNU Guix is
> +written in, is lacking in primitives that usually are used to avoid
> +these kind of issues, such `openat` and `O_NOFOLLOW`.
> +
> +While these primitives turned out not to be necessary to fix the
> +issue and a [patch series](<https://lists.gnu.org/archive/html/guile-devel/2021-03/msg00026.html>)
> +to GNU Guile has been submitted that adds these primitives, this does
> +serve as a remainder that GNU Guile is a critical component of
> +Guix System and working around missing primitives will not always be possible.

All this is true but also probably too detailed (or not enough,
depending on the reader).  How about just mentioning that work is
ongoing to support the `openat` family of POSIX functions in Guile,
which, when used, while help address this class of vulnerability?

Otherwise LGTM, thanks!

Ludo’.

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

[Ludovic Courtès]

Maxime Devos <maximedevos@telenet.be> skribis:

> +The attack consists of the user being logged in after the account
> +skeletons have been copied to the home directory, but before the
> +owner of the account skeletons have been set.  The user then deletes
> +a copied account skeleton (e.g. `$HOME/.gdbinit`) and replaces
> +it with a symbolic link to a file not owned by the user, such as
> +`/etc/shadow`.

Also…  in this paragraph, it’s not entirely clear which user we’re
talking about it.  In news.scm, I reworded it like so:

  The attack can happen when @command{guix system reconfigure} is running.
  Running @command{guix system reconfigure} can trigger the creation of new user
  accounts if the configuration specifies new accounts.  If a user whose account
  is being created manages to log in after the account has been created but
  before ``skeleton files'' copied to its home directory have the right
  ownership, they may, by creating an appropriately-named symbolic link in the
  home directory pointing to a sensitive file, such as @file{/etc/shadow}, get
  root privileges.

It may also be worth mentioning that the user is likely unable to log in
at all at that point, as I wrote here:

  https://issues.guix.gnu.org/47584#6

WDYT?

Ludo’.

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

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

On Sat, 2021-04-03 at 22:33 +0200, Ludovic Courtès wrote:
> Maxime Devos <maximedevos@telenet.be> skribis:
> 
> > The attack consists of the user being logged in after the account
> > skeletons have been copied to the home directory, but before the
> > owner of the account skeletons have been set.  The user then deletes
> > a copied account skeleton (e.g. @file{$HOME/.gdbinit}) and replaces
> > it with a symbolic link to a file not owned by the user, such as
> > @file{/etc/shadow}.
> > 
> > The activation code then changes the ownership
> > of the file the symbolic link points to instead of the symbolic
> > link itself.  At that point, the user has read-write access
> > to the target file.
> 
> In the draft blog post, you mention that the attack cannot be carried
> out when protected symlinks are enabled.

In the blog post, I thought I wrote the attack can be carried out
*even if* protected symlinks are enabled.  Looking at

https://sysctl-explorer.net/fs/protected_symlinks/,

I don't think the Linux protected symlink feature helps, as home
directories are never sticky and word-writable.

Perhaps I should have written ‘possible’ instead of ‘not impossible’
in the blog post.

> Please mention that protected symlinks are enabled by default on Guix
> System since a March 16th commit, with a link to [...]

See my response above.

I agree with all other comments on this bug report.

Greetings,
Maxime.

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

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

[Maxime Devos]

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

On Sat, 2021-04-03 at 18:26 +0200, Maxime Devos wrote:
> A suggested blog post is attached.
A revised blog post is attached.

The following points are currently _not_ addressed:

Ludovic Courtès wrote:
> Also…  in this paragraph, it’s not entirely clear which user we’re
> talking about it.  In news.scm, I reworded it like so:
>  The attack can happen when @command{guix system reconfigure} is running.
>  Running @command{guix system reconfigure} can trigger the creation of new user
>  accounts if the configuration specifies new accounts.  If a user whose account
>  is being created manages to log in after the account has been created but
>  before ``skeleton files'' copied to its home directory have the right
>  ownership, they may, by creating an appropriately-named symbolic link in the
>  home directory pointing to a sensitive file, such as @file{/etc/shadow}, get
>  root privileges.
>
> It may also be worth mentioning that the user is likely unable to log in
> at all at that point, as I wrote here:

I can't think of something along these lines to write at the moment ...

Greetings,
Maxime.

[-- Attachment #1.2: 0001-website-Add-post-about-vulnerability-in-copy-account.patch --]
[-- Type: text/x-patch, Size: 4313 bytes --]

From 10b4528aac6cd9c0c341634b9f163f0a38ec4c6b Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sat, 3 Apr 2021 18:02:05 +0200
Subject: [PATCH] =?UTF-8?q?website:=20Add=20post=20about=20vulnerability?=
 =?UTF-8?q?=20in=20=E2=80=98copy-account-skeletons=E2=80=99.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* website/posts/home-symlink.md: New post.

Co-authored-by: Ludovic Courtès <ludo@gnu.org>
---
 website/posts/home-symlink.md | 86 +++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 website/posts/home-symlink.md

diff --git a/website/posts/home-symlink.md b/website/posts/home-symlink.md
new file mode 100644
index 0000000..67f3053
--- /dev/null
+++ b/website/posts/home-symlink.md
@@ -0,0 +1,86 @@
+title: Risk of local privilege escalation in account creation
+date: 2021-04-04 15:30
+author: Maxime Devos, Ludovic Courtès
+tags: Security Advisory
+---
+
+A security vulnerability that can lead to local privilege
+escalation has been found in the code that creates user accounts on Guix
+System—Guix on other distros is unaffected.  The system is only vulnerable
+during the activation of non-system user accounts that do not already exist.
+
+This exploit is _not_ prevented by the Linux [protected
+symlinks](https://sysctl-explorer.net/fs/protected_symlinks/) feature.
+
+# Vulnerability
+
+The attack consists of the user being logged in after the account
+skeletons have been copied to the home directory, but before the
+owner of the account skeletons have been set.  The user then deletes
+a copied account skeleton (e.g. `$HOME/.gdbinit`) and replaces
+it with a symbolic link to a file not owned by the user, such as
+`/etc/shadow`.
+
+The activation code then changes the ownership of the file the symbolic
+link points to instead of the symbolic link itself.  At that point, the
+user has read-write access to the target file.
+
+# Fix
+
+This [bug](https://issues.guix.gnu.org/47584) has been
+[fixed](https://git.savannah.gnu.org/cgit/guix.git/commit/?id=2161820ebbbab62a5ce76c9101ebaec54dc61586).
+See below for upgrade instructions.
+
+The fix consist of initially creating the home directory root-owned and only
+changing the owner of the home directory once all skeletons have been copied
+and their owner has been set.
+
+# Upgrading
+
+To upgrade the Guix System, run something like:
+
+```
+guix pull
+sudo guix system reconfigure /run/current-system/configuration.scm
+sudo reboot
+```
+
+As the user account activation code is run as a shepherd service,
+the last step is required to make sure the fixed activation code
+is run in the future.
+
+To avoid the vulnerability while upgrading the system, only declare
+new user accounts in the configuration file after the Guix System
+has been upgraded.
+
+# Conclusions
+
+Work is ongoing to support the `openat` family of POSIX functions in
+Guile, which, when used, help address this class of vulnerabilities.
+
+This issue is tracked as
+[bug #47584](https://issues.guix.gnu.org/47584); you can read the thread
+for more information.
+
+Please report any issues you may have to
+[`guix-devel@gnu.org`](https://guix.gnu.org/en/contact/).  See the
+[security web page](https://guix.gnu.org/en/security/) for information
+on how to report security issues.
+
+#### About GNU Guix
+
+[GNU Guix](https://guix.gnu.org) is a transactional package manager and
+an advanced distribution of the GNU system that [respects user
+freedom](https://www.gnu.org/distros/free-system-distribution-guidelines.html).
+Guix can be used on top of any system running the Hurd or the Linux
+kernel, or it can be used as a standalone operating system distribution
+for i686, x86_64, ARMv7, and AArch64 machines.
+
+In addition to standard package management features, Guix supports
+transactional upgrades and roll-backs, unprivileged package management,
+per-user profiles, and garbage collection.  When used as a standalone
+GNU/Linux distribution, Guix offers a declarative, stateless approach to
+operating system configuration management.  Guix is highly customizable
+and hackable through [Guile](https://www.gnu.org/software/guile)
+programming interfaces and extensions to the
+[Scheme](http://schemers.org) language.
-- 
2.31.1


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

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

[Ludovic Courtès]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> On Sat, 2021-04-03 at 22:33 +0200, Ludovic Courtès wrote:
>> Maxime Devos <maximedevos@telenet.be> skribis:
>> 
>> > The attack consists of the user being logged in after the account
>> > skeletons have been copied to the home directory, but before the
>> > owner of the account skeletons have been set.  The user then deletes
>> > a copied account skeleton (e.g. @file{$HOME/.gdbinit}) and replaces
>> > it with a symbolic link to a file not owned by the user, such as
>> > @file{/etc/shadow}.
>> > 
>> > The activation code then changes the ownership
>> > of the file the symbolic link points to instead of the symbolic
>> > link itself.  At that point, the user has read-write access
>> > to the target file.
>> 
>> In the draft blog post, you mention that the attack cannot be carried
>> out when protected symlinks are enabled.
>
> In the blog post, I thought I wrote the attack can be carried out
> *even if* protected symlinks are enabled.  Looking at
>
> https://sysctl-explorer.net/fs/protected_symlinks/,
>
> I don't think the Linux protected symlink feature helps, as home
> directories are never sticky and word-writable.

Oh right, my bad, I overlooked this.

> Perhaps I should have written ‘possible’ instead of ‘not impossible’
> in the blog post.

Dunno, maybe it’s just me not paying enough attention.

> I agree with all other comments on this bug report.

OK.  It does mean that the bug is hardly exploitable in practice: you
have to be able to log in at all, and if you’re able to log in, you have
to log in precisely within the 1s (or less) that follows account
creation, which sounds challenging (TCP + SSH connection establishment
is likely to take as much time or more, likewise for typing in your
password.)  It’s also one-time chance.

Do I get it right?

Does it warrant as strong messaging as for the recent daemon
‘--keep-failed’ vulnerability?

Thanks,
Ludo’.

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

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Mon, 2021-04-05 at 21:54 +0200, Ludovic Courtès wrote:
> [...]
> 
> OK.  It does mean that the bug is hardly exploitable in practice: you
> have to be able to log in at all,
Yes.

>  and if you’re able to log in, you have
> to log in precisely within the 1s (or less) that follows account
> creation, which sounds challenging (TCP + SSH connection establishment
> is likely to take as much time or more,

Is logging in possible when the home directory doesn't exist?
It isn't possible from the console.  I guess it isn't possible from SSH
either.

If it is possible,
then the window would be somewhat larger I think.  Account creation is done
at activation time, while creating home directories is done as a shepherd
service (see account-service-type in gnu/system/shadow.scm).

>  likewise for typing in your password.)
An attacker could copy and paste, or have used a single-character password,
to save some time.

>   It’s also one-time chance.

Yes.

> Do I get it right?

I think so, except the window might be larger (but still a one-time chance).

> Does it warrant as strong messaging as for the recent daemon
> ‘--keep-failed’ vulnerability?

As it is a one-time chance, with a limited window, and only under specific
circumstances (creating a new user account), I don't think so.  But I would
still recommend to upgrade.  Does the blog post have ‘too strong messaging’? 

Greetings,
Maxime

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

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

[Ludovic Courtès]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> On Mon, 2021-04-05 at 21:54 +0200, Ludovic Courtès wrote:
>> [...]
>> 
>> OK.  It does mean that the bug is hardly exploitable in practice: you
>> have to be able to log in at all,
> Yes.
>
>>  and if you’re able to log in, you have
>> to log in precisely within the 1s (or less) that follows account
>> creation, which sounds challenging (TCP + SSH connection establishment
>> is likely to take as much time or more,
>
> Is logging in possible when the home directory doesn't exist?

I think so.

> An attacker could copy and paste, or have used a single-character password,
> to save some time.

Hmm yes.  It’s a bit a far-fetched though: the attacker would have
passed the sysadmin the output of the ‘crypt’ procedure, such that the
sysadmin cannot know the password length.

>> Does it warrant as strong messaging as for the recent daemon
>> ‘--keep-failed’ vulnerability?
>
> As it is a one-time chance, with a limited window, and only under specific
> circumstances (creating a new user account), I don't think so.  But I would
> still recommend to upgrade.  Does the blog post have ‘too strong messaging’? 

The blog post and info-guix messages are the highest levels of
visibility we can give, roughly.  So I think we have to think twice
before doing that or truly important issues will eventually go
unnoticed.

The risk with this issue seems much lower than that of the keep-failed
issue, it even looks super low.

WDYT?

Ludo’.

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

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

On Tue, 2021-04-06 at 13:57 +0200, Ludovic Courtès wrote:
> [...]
> 
> The blog post and info-guix messages are the highest levels of
> visibility we can give, roughly.  So I think we have to think twice
> before doing that or truly important issues will eventually go
> unnoticed.
> 
> The risk with this issue seems much lower than that of the keep-failed
> issue, it even looks super low.
> 
> WDYT?

That is a good point, but I still wonder if there's *somewhere* this
can be posted.

I was going to start a thread at guix-devel about
blog posts in general (categories, what can be posted as a ‘official’
blog post on guix.gnu.org, any maximal frequencies ...) but I ended up
being busy with other things.

Greetings,
Maxime.

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

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 254 bytes --]

Now openat etc is in Guile, I've looked into adjusting mkdir-p/perms 
appropriately.  TODO: change the Guile used for activation to some 
commit that has openat etc, adjust patch according to test failures. 
(Not tested yet)

Greetings,
Maxime.


[-- Attachment #1.1.2: mkdir-p.diff --]
[-- Type: text/x-patch, Size: 4461 bytes --]

diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index 10c9045740..ee52bb1979 100644
--- a/gnu/build/activation.scm
+++ b/gnu/build/activation.scm
@@ -5,7 +5,7 @@
 ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
-;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2021, 2022 Maxime Devos <maximedevos@telenet.be>
 ;;; Copyright © 2020 Christine Lemmer-Webber <cwebber@dustycloud.org>
 ;;; Copyright © 2021 Brice Waegeneire <brice@waegenei.re>
 ;;;
@@ -65,45 +65,61 @@ (define (dot-or-dot-dot? file)
   (member file '("." "..")))
 
 ;; Based upon mkdir-p from (guix build utils)
-(define (verify-not-symbolic dir)
-  "Verify DIR or its ancestors aren't symbolic links."
+(define (mkdir-p/perms directory owner bits)
+  "Create directory DIRECTORY and all its ancestors.
+
+Additionally, verify no component of DIRECTORY is a symbolic link,
+without TOCTTOU races.  However, if OWNER differs from the the current
+(process) uid/gid, there is a small window in which DIRECTORY is set to the
+current (process) uid/gid instead of OWNER.  This is not expected to be
+a problem in practice.
+
+The permission bits and owner of DIRECTORY are set to BITS and OWNER.
+Anything above DIRECTORY that already exists keeps
+its old owner and bits.  For components that do not exist yet, the owner
+and bits are set according to the default behaviour of 'mkdir'."
   (define absolute?
-    (string-prefix? "/" dir))
+    (string-prefix? "/" directory))
 
   (define not-slash
     (char-set-complement (char-set #\/)))
 
-  (define (verify-component file)
-    (unless (eq? 'directory (stat:type (lstat file)))
-      (error "file name component is not a directory" dir)))
-
-  (let loop ((components (string-tokenize dir not-slash))
-             (root       (if absolute?
-                             ""
-                             ".")))
+  ;; By combining O_NOFOLLOW and O_DIRECTORY, this procedure automatically
+  ;; verifies that no components are symlinks.
+  (define open-flags (logior O_CLOEXEC ; don't pass the port on to subprocesses
+                             O_NOFOLLOW ; don't follow symlinks
+                             O_DIRECTORY ; reject anything not a directory
+                             O_PATH)) ; TODO: Does Hurd have O_PATH?
+  
+  (let loop ((components (string-tokenize directory not-slash))
+             (root (open (if absolute? "/" ".") open-flags)))
     (match components
       ((head tail ...)
-       (let ((file (string-append root "/" head)))
-         (catch 'system-error
-           (lambda ()
-             (verify-component file)
-             (loop tail file))
-           (lambda args
-             (if (= ENOENT (system-error-errno args))
-                 #t
-                 (apply throw args))))))
-      (() #t))))
-
-;; TODO: the TOCTTOU race can be addressed once guile has bindings
-;; for fstatat, openat and friends.
-(define (mkdir-p/perms directory owner bits)
-  "Create the directory DIRECTORY and all its ancestors.
-Verify no component of DIRECTORY is a symbolic link.
-Warning: this is currently suspect to a TOCTTOU race!"
-  (verify-not-symbolic directory)
-  (mkdir-p directory)
-  (chown directory (passwd:uid owner) (passwd:gid owner))
-  (chmod directory bits))
+       (let retry ()
+         ;; In the usual case, we expect HEAD to already exist.
+         (match (catch 'system-error
+                  (lambda ()
+                    (openat root head open-flags))
+                  (lambda args
+                    (if (= ENOENT (system-error-errno args))
+                        #false
+                        (apply throw args))))
+           ((? port? new-root)
+            (close root)
+            (loop tail new-root))
+           (#false
+            ;; If not, create it.
+            (catch 'system-error
+              (lambda _
+                (mkdirat root head))
+              (lambda args
+                ;; Someone else created the directory.  Unexpected but fine.
+                (unless (= EEXIST (system-error-errno args))
+                  (apply throw args))))
+            (retry)))))
+      (()
+       (chown directory (passwd:uid owner) (passwd:gid owner))
+       (chmod directory bits)))))
 
 (define* (copy-account-skeletons home
                                  #:key

[-- Attachment #1.1.3: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#47584: [DRAFT PATCH v2 0/4] Fix race condition in mkdir-p/perms

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 226 bytes --]

 > TODO: change the Guile used for activation to some
 > commit that has openat etc, [...]

This is done now, but "make check-system" now fails due to an openssl 
build failure, see latest patch, so not yet appliable ...

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#47584: [PATCH 1/3] guile-next: Update to 3.0.8-793fb46.

[Maxime Devos]

* gnu/packages/guile.scm (guile-next): Update to 3.0.8, commit 793fb46.
[arguments]: Remove 'skip-failing-tests', as presumably the issues are fixed
in the new version.
---
 gnu/packages/guile.scm | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/gnu/packages/guile.scm b/gnu/packages/guile.scm
index fcdf75051c..936fc8649f 100644
--- a/gnu/packages/guile.scm
+++ b/gnu/packages/guile.scm
@@ -431,11 +431,11 @@ (define-public guile-3.0/fixed
                                                 ;  when heavily loaded)
 
 (define-public guile-next
-  (let ((version "3.0.7")
+  (let ((version "3.0.8")
         (revision "0")
-        (commit "d70c1dbebf9ac0fd45af4578c23983ec4a7da535"))
+        (commit "793fb46a1e69fa2156805e4a97b340cf62e096a6"))
     (package
-      (inherit guile-3.0)
+      (inherit guile-3.0-latest)
       (name "guile-next")
       (version (git-version version revision commit))
       (source (origin
@@ -447,19 +447,10 @@ (define-public guile-next
                 (file-name (git-file-name name version))
                 (sha256
                  (base32
-                  "05rsk9lh5kchbav3lwfwgvgybrykqqjmkkc6689fhb3mjr5m3dqj"))))
-      (arguments
-       (substitute-keyword-arguments (package-arguments guile-3.0)
-         ((#:phases phases '%standard-phases)
-          `(modify-phases ,phases
-             (add-before 'check 'skip-failing-tests
-               (lambda _
-                 (substitute* "test-suite/standalone/test-out-of-memory"
-                   (("!#") "!#\n\n(exit 77)\n"))
-                 (delete-file "test-suite/tests/version.test")
-                 #t))))))
+                  "0x42qhsdgx7mg6ap2zgbpbj3f5yhjapyr3xkpzb1z6f2yc8rdlsw"))))
       (native-inputs
-       (modify-inputs (package-native-inputs guile-3.0)
+       (modify-inputs (package-native-inputs guile-3.0-latest)
+         (replace "guile" this-package) ; for cross-compilation
          (prepend autoconf
                   automake
                   libtool

base-commit: 31a56967e2869c916b7a5e8ee570e8e10f0210a5
prerequisite-patch-id: 2712efb97bf33985fd0658e4dd8e936dc08be5fe
prerequisite-patch-id: 9d2409b480a8bff0fef029b4b095922d4957e06f
prerequisite-patch-id: 51a32abca3efec1ba67ead59b8694c5ea3129ad3
prerequisite-patch-id: 9092927761a340c07a99f5f3ed314a6add04cdee
prerequisite-patch-id: d0af09fbd5ee0ef60bdee53b87d729e46c1db2ca
prerequisite-patch-id: c2b101598fa5b6f93470ae41d51a983dcb931b04
-- 
2.38.0

bug#47584: [PATCH 2/3] WIP gnu: Change the Guile used for activation to one that has 'openat'.

[Maxime Devos]

TODO: when doing "make check-system TESTS=ldap", I get a build failure
of openssl@1.1.1l, I suspect it's a situation like
<https://issues.guix.gnu.org/56137> again, though I haven't investigated yet.

Test Summary Report
-------------------
../test/recipes/80-test_ssl_new.t                (Wstat: 256 Tests: 29 Failed: 1)
  Failed test:  12
  Non-zero exit status: 1
Files=158, Tests=2636, 157 wallclock secs ( 2.29 usr  0.18 sys + 104.74 cusr 28.04 csys = 135.25 CPU)
Result: FAIL
make[1]: *** [Makefile:208: _tests] Error 1
make[1]: Leaving directory '/tmp/guix-build-openssl-1.1.1l.drv-0/openssl-1.1.1l'
make: *** [Makefile:205: tests] Error 2

Test suite failed, dumping logs.
error: in phase 'check': uncaught exception:
%exception #<&invoke-error program: "make" arguments: ("test") exit-status: 2 term-signal: #f stop-signal: #f>
phase `check' failed after 157.1 seconds
command "make" "test" failed with status 2
note: keeping build directory `/tmp/guix-build-openssl-1.1.1l.drv-1'
builder for `/gnu/store/jhijsrxqh586l8ck61ppkhydkb158hj0-openssl-1.1.1l.drv' failed with exit code 1
build of /gnu/store/jhijsrxqh586l8ck61ppkhydkb158hj0-openssl-1.1.1l.drv failed
[...]

This is required by the next patch, in which 'mkdir-p/perms'
uses 'openat'.

* gnu/packages/guile.scm (guile-for-activation): New variable.
* gnu/services.scm (activation-script)[actions]: Set #:guile to
guile-for-activation.
* gnu/packages/make-bootstrap.scm (%guile-static-stripped/initrd):
New variable.
* gnu/system/linux-initrd.scm (expression->initrd): Use
%guile-static-stripped/initrd instead of %guile-static-stripped.
---
 gnu/packages/guile.scm          |  5 +++++
 gnu/packages/make-bootstrap.scm | 15 ++++++++++++---
 gnu/services.scm                |  5 ++++-
 gnu/system/linux-initrd.scm     |  4 ++--
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/gnu/packages/guile.scm b/gnu/packages/guile.scm
index 936fc8649f..1d1b0bd77b 100644
--- a/gnu/packages/guile.scm
+++ b/gnu/packages/guile.scm
@@ -460,6 +460,11 @@ (define-public guile-next
                   gperf)))
       (synopsis "Development version of GNU Guile"))))
 
+;; The important thing here is that this Guile has 'openat' and friends
+;; for (gnu build activation), which at time of writing isn't available
+;; in any release yet.
+(define-public guile-for-activation guile-next)
+
 (define* (make-guile-readline guile #:optional (name "guile-readline"))
   (package
     (name name)
diff --git a/gnu/packages/make-bootstrap.scm b/gnu/packages/make-bootstrap.scm
index 4ea97368a9..8852caa406 100644
--- a/gnu/packages/make-bootstrap.scm
+++ b/gnu/packages/make-bootstrap.scm
@@ -7,6 +7,7 @@
 ;;; Copyright © 2019, 2020 Marius Bakke <mbakke@fastmail.com>
 ;;; Copyright © 2020 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2021 Pierre Langlois <pierre.langlois@gmx.com>
+;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -57,7 +58,8 @@ (define-module (gnu packages make-bootstrap)
             %mes-bootstrap-tarball
             %bootstrap-tarballs
 
-            %guile-static-stripped))
+            %guile-static-stripped
+            %guile-static-stripped/initrd))
 
 ;;; Commentary:
 ;;;
@@ -794,14 +796,21 @@ (define* (make-guile-static-stripped static-guile)
     (synopsis "Minimal statically-linked and relocatable Guile")))
 
 (define %guile-static-stripped
-  ;; A stripped static Guile 3.0 binary, for use in initrds
-  ;; and during bootstrap.
+  ;; A stripped static Guile 3.0 binary, for use during bootstrap.
   (make-guile-static-stripped
    (make-guile-static guile-3.0
                       '("guile-2.2-default-utf8.patch"
                         "guile-3.0-linux-syscalls.patch"
                         "guile-3.0-relocatable.patch"))))
 
+;; Like %guile-static-stripped, but for use in initrds.
+(define %guile-static-stripped/initrd
+  (make-guile-static-stripped
+   (make-guile-static guile-for-activation
+                      '("guile-2.2-default-utf8.patch"
+                        "guile-3.0-linux-syscalls.patch"
+                        "guile-3.0-relocatable.patch"))))
+
 (define (tarball-package pkg)
   "Return a package containing a tarball of PKG."
   (package
diff --git a/gnu/services.scm b/gnu/services.scm
index 2abef557d4..e051f9e821 100644
--- a/gnu/services.scm
+++ b/gnu/services.scm
@@ -6,6 +6,7 @@
 ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
 ;;; Copyright © 2020 Christine Lemmer-Webber <cwebber@dustycloud.org>
 ;;; Copyright © 2020, 2021 Brice Waegeneire <brice@waegenei.re>
+;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -41,6 +42,7 @@ (define-module (gnu services)
   #:use-module (guix utils)
   #:use-module (gnu packages base)
   #:use-module (gnu packages bash)
+  #:use-module ((gnu packages guile) #:select (guile-for-activation))
   #:use-module (gnu packages hurd)
   #:use-module (gnu system setuid)
   #:use-module (srfi srfi-1)
@@ -610,7 +612,8 @@ (define* (activation-service->script service)
 (define (activation-script gexps)
   "Return the system's activation script, which evaluates GEXPS."
   (define actions
-    (map (cut program-file "activate-service.scm" <>) gexps))
+    (map (cut program-file "activate-service.scm" <>
+              #:guile guile-for-activation) gexps))
 
   (program-file "activate.scm"
                 (with-imported-modules (source-module-closure
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 4c4c78e444..b65d830a17 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -36,7 +36,7 @@ (define-module (gnu system linux-initrd)
   #:use-module ((gnu packages xorg)
                 #:select (console-setup xkeyboard-config))
   #:use-module ((gnu packages make-bootstrap)
-                #:select (%guile-static-stripped))
+                #:select (%guile-static-stripped/initrd))
   #:use-module (gnu system file-systems)
   #:use-module (gnu system mapped-devices)
   #:use-module (gnu system keyboard)
@@ -62,7 +62,7 @@ (define-module (gnu system linux-initrd)
 
 (define* (expression->initrd exp
                              #:key
-                             (guile %guile-static-stripped)
+                             (guile %guile-static-stripped/initrd)
                              (gzip gzip)
                              (name "guile-initrd")
                              (system (%current-system)))
-- 
2.38.0

bug#47584: [PATCH 3/3] activation: Fix TOCTTOU in mkdir-p/perms.

[Maxime Devos]

I removed the 'Based upon mkdir-p from (guix build utils)'
comment because it's quite a bit different now.

* gnu/build/activation.scm (verify-not-symbolic): Delete.
(mkdir-p/perms): Rewrite in terms of 'openat'.
---
 gnu/build/activation.scm | 90 +++++++++++++++++++++++++---------------
 1 file changed, 57 insertions(+), 33 deletions(-)

diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index 10c9045740..29c6f2ce4c 100644
--- a/gnu/build/activation.scm
+++ b/gnu/build/activation.scm
@@ -5,7 +5,7 @@
 ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
-;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2021, 2022 Maxime Devos <maximedevos@telenet.be>
 ;;; Copyright © 2020 Christine Lemmer-Webber <cwebber@dustycloud.org>
 ;;; Copyright © 2021 Brice Waegeneire <brice@waegenei.re>
 ;;;
@@ -64,46 +64,70 @@ (define %skeleton-directory
 (define (dot-or-dot-dot? file)
   (member file '("." "..")))
 
-;; Based upon mkdir-p from (guix build utils)
-(define (verify-not-symbolic dir)
-  "Verify DIR or its ancestors aren't symbolic links."
+(define (mkdir-p/perms directory owner bits)
+  "Create directory DIRECTORY and all its ancestors.
+
+Additionally, verify no component of DIRECTORY is a symbolic link,
+without TOCTTOU races.  However, if OWNER differs from the the current
+(process) uid/gid, there is a small window in which DIRECTORY is set to the
+current (process) uid/gid instead of OWNER.  This is not expected to be
+a problem in practice.
+
+The permission bits and owner of DIRECTORY are set to BITS and OWNER.
+Anything above DIRECTORY that already exists keeps
+its old owner and bits.  For components that do not exist yet, the owner
+and bits are set according to the default behaviour of 'mkdir'."
   (define absolute?
-    (string-prefix? "/" dir))
+    (string-prefix? "/" directory))
 
   (define not-slash
     (char-set-complement (char-set #\/)))
 
-  (define (verify-component file)
-    (unless (eq? 'directory (stat:type (lstat file)))
-      (error "file name component is not a directory" dir)))
+  ;; By combining O_NOFOLLOW and O_DIRECTORY, this procedure automatically
+  ;; verifies that no components are symlinks.
+  (define open-flags (logior O_CLOEXEC ; don't pass the port on to subprocesses
+                             O_NOFOLLOW ; don't follow symlinks
+                             O_DIRECTORY)) ; reject anything not a directory
 
-  (let loop ((components (string-tokenize dir not-slash))
-             (root       (if absolute?
-                             ""
-                             ".")))
+  (let loop ((components (string-tokenize directory not-slash))
+             (root (open (if absolute? "/" ".") open-flags)))
     (match components
       ((head tail ...)
-       (let ((file (string-append root "/" head)))
-         (catch 'system-error
-           (lambda ()
-             (verify-component file)
-             (loop tail file))
-           (lambda args
-             (if (= ENOENT (system-error-errno args))
-                 #t
-                 (apply throw args))))))
-      (() #t))))
-
-;; TODO: the TOCTTOU race can be addressed once guile has bindings
-;; for fstatat, openat and friends.
-(define (mkdir-p/perms directory owner bits)
-  "Create the directory DIRECTORY and all its ancestors.
-Verify no component of DIRECTORY is a symbolic link.
-Warning: this is currently suspect to a TOCTTOU race!"
-  (verify-not-symbolic directory)
-  (mkdir-p directory)
-  (chown directory (passwd:uid owner) (passwd:gid owner))
-  (chmod directory bits))
+       (let retry ()
+         ;; In the usual case, we expect HEAD to already exist.
+         (match (catch 'system-error
+                  (lambda ()
+                    (openat root head open-flags))
+                  (lambda args
+                    (if (= ENOENT (system-error-errno args))
+                        #false
+                        (begin
+                          (close-port root)
+                          (apply throw args)))))
+           ((? port? new-root)
+            (close root)
+            (loop tail new-root))
+           (#false
+            ;; If not, create it.
+            (catch 'system-error
+              (lambda _
+                (mkdirat root head))
+              (lambda args
+                ;; Someone else created the directory.  Unexpected but fine.
+                (unless (= EEXIST (system-error-errno args))
+                  (close-port root)
+                  (apply throw args))))
+            (retry)))))
+      (()
+       (catch 'system-error
+         (lambda ()
+           (chown root (passwd:uid owner) (passwd:gid owner))
+           (chmod root bits))
+         (lambda args
+           (close-port root)
+           (apply throw args)))
+       (close-port root)
+       (values)))))
 
 (define* (copy-account-skeletons home
                                  #:key
-- 
2.38.0

bug#47584: [PATCH 1/3] guile-next: Update to 3.0.8-793fb46.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 314 bytes --]

On 28-10-2022 18:04, Maxime Devos wrote:
>         (native-inputs
> -       (modify-inputs (package-native-inputs guile-3.0)
> +       (modify-inputs (package-native-inputs guile-3.0-latest)
> +         (replace "guile" this-package) ; for cross-compilation

I forgot to mention this in the commit message.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Ludovic Courtès]

Hello,

Maxime Devos <maximedevos@telenet.be> skribis:

> I removed the 'Based upon mkdir-p from (guix build utils)'
> comment because it's quite a bit different now.
>
> * gnu/build/activation.scm (verify-not-symbolic): Delete.
> (mkdir-p/perms): Rewrite in terms of 'openat'.

Finally pushed as c1283e203995c8d84584e701b965efe086d1d666, now that
Guile 3.0.9 with the *at family of procedures is the default (and has
been for a while, actually).

Great work both in Guile upstream and in Guix here.

Ludo’.