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

Previous Next

To reply to this bug, email your comments to 47584 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

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

From: Maxime Devos <maximedevos <at> telenet.be>
To: bug-guix <at> gnu.org
Subject: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Sat, 03 Apr 2021 18:09:16 +0200

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

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

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Sat, 03 Apr 2021 18:22:12 +0200

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

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

[0001-activation-Do-not-dereference-symlinks-in-home-direc.patch (text/x-patch, attachment)]

[0002-news-Add-entry-for-user-account-activation-vulnerabi.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Sat, 03 Apr 2021 18:26:53 +0200

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

A suggested blog post is attached.

[0001-website-Add-post-about-vulnerability-in-copy-account.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Sat, 03 Apr 2021 18:32:54 +0200

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';

Message #21 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Sat, 03 Apr 2021 22:15:45 +0200

Hi Maxime,

Maxime Devos <maximedevos <at> telenet.be> skribis:

> From 9672bd37bf50db1e0989d0b84035c4788422bd31 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos <at> 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 <at> 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 <at> 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’.

Message #24 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Sat, 03 Apr 2021 22:27:23 +0200

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

Maxime Devos <maximedevos <at> 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

Message #27 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Sat, 03 Apr 2021 22:33:18 +0200

Maxime Devos <maximedevos <at> 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

Message #30 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Sat, 03 Apr 2021 22:45:51 +0200

Maxime Devos <maximedevos <at> telenet.be> skribis:

> From 7937b9f18085569e5d7cb8a3c4dc08e1088a94a9 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos <at> 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’.

Message #33 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Sat, 03 Apr 2021 22:49:24 +0200

Maxime Devos <maximedevos <at> 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’.

Message #36 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Sun, 04 Apr 2021 09:36:05 +0200

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

On Sat, 2021-04-03 at 22:33 +0200, Ludovic Courtès wrote:
> Maxime Devos <maximedevos <at> 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.

[signature.asc (application/pgp-signature, inline)]

Message #39 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Sun, 04 Apr 2021 15:29:01 +0200

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

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.

[0001-website-Add-post-about-vulnerability-in-copy-account.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #42 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: Leo Famulari <leo <at> famulari.name>, 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Mon, 05 Apr 2021 21:54:56 +0200

Hi Maxime,

Maxime Devos <maximedevos <at> telenet.be> skribis:

> On Sat, 2021-04-03 at 22:33 +0200, Ludovic Courtès wrote:
>> Maxime Devos <maximedevos <at> 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’.

Message #45 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Leo Famulari <leo <at> famulari.name>, 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Tue, 06 Apr 2021 11:56:23 +0200

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

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

[signature.asc (application/pgp-signature, inline)]

Message #48 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: Leo Famulari <leo <at> famulari.name>, 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Tue, 06 Apr 2021 13:57:08 +0200

Hi Maxime,

Maxime Devos <maximedevos <at> 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’.

Message #51 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Leo Famulari <leo <at> famulari.name>, 47584 <at> debbugs.gnu.org
Subject: Re: bug#47584: Race condition in
 ‘copy-account-skeletons’: possible privilege
 escalation.
Date: Wed, 07 Apr 2021 20:28:02 +0200

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

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.

[signature.asc (application/pgp-signature, inline)]

Message #54 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Subject: Re: Race condition in ‘copy-account-skeletons’: possible privilege escalation.
Date: Fri, 21 Oct 2022 11:31:14 +0200

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

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.

[mkdir-p.diff (text/x-patch, attachment)]

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #57 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Subject: [DRAFT PATCH v2 0/4] Fix race condition in mkdir-p/perms
Date: Fri, 28 Oct 2022 18:03:26 +0200

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

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

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #60 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Cc: Maxime Devos <maximedevos <at> telenet.be>
Subject: [PATCH 2/3] WIP gnu: Change the Guile used for activation to one that
 has 'openat'.
Date: Fri, 28 Oct 2022 18:04:08 +0200

TODO: when doing "make check-system TESTS=ldap", I get a build failure
of openssl <at> 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 <at> fastmail.com>
 ;;; Copyright © 2020 Mathieu Othacehe <m.othacehe <at> gmail.com>
 ;;; Copyright © 2021 Pierre Langlois <pierre.langlois <at> gmx.com>
+;;; Copyright © 2022 Maxime Devos <maximedevos <at> 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 <at> protonmail.com>
 ;;; Copyright © 2020 Christine Lemmer-Webber <cwebber <at> dustycloud.org>
 ;;; Copyright © 2020, 2021 Brice Waegeneire <brice <at> waegenei.re>
+;;; Copyright © 2022 Maxime Devos <maximedevos <at> 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

Message #63 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Cc: Maxime Devos <maximedevos <at> telenet.be>
Subject: [PATCH 1/3] guile-next: Update to 3.0.8-793fb46.
Date: Fri, 28 Oct 2022 18:04:07 +0200

* 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

Message #66 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Cc: Maxime Devos <maximedevos <at> telenet.be>
Subject: [PATCH 3/3] activation: Fix TOCTTOU in mkdir-p/perms.
Date: Fri, 28 Oct 2022 18:04:09 +0200

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 <at> netris.org>
 ;;; Copyright © 2018 Arun Isaac <arunisaac <at> systemreboot.net>
 ;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado <at> elephly.net>
-;;; Copyright © 2021 Maxime Devos <maximedevos <at> telenet.be>
+;;; Copyright © 2021, 2022 Maxime Devos <maximedevos <at> telenet.be>
 ;;; Copyright © 2020 Christine Lemmer-Webber <cwebber <at> dustycloud.org>
 ;;; Copyright © 2021 Brice Waegeneire <brice <at> 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

Message #69 received at 47584 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47584 <at> debbugs.gnu.org
Subject: Re: [PATCH 1/3] guile-next: Update to 3.0.8-793fb46.
Date: Fri, 28 Oct 2022 18:05:47 +0200

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

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.

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.