oss-security - Re: CVE-2024-47191: Local root exploit in the PAM module pam

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <878quzt99y.fsf@kaka.sjd.se>
Date: Tue, 08 Oct 2024 08:10:17 +0200
From: Simon Josefsson <simon@...efsson.org>
To: Solar Designer <solar@...nwall.com>
Cc: oss-security@...ts.openwall.com
Subject: Re: CVE-2024-47191: Local root exploit in the PAM module pam_oath.so

Solar Designer <solar@...nwall.com> writes:

> This link requires authentication.  I guess you meant to post:
>
> https://gitlab.com/oath-toolkit/oath-toolkit/-/commit/95ef255e6a401949ce3f67609bf8aac2029db418

Fixed, thank you!  The writeup is available here:

https://www.nongnu.org/oath-toolkit/CVE-2024-47191.html

> I note a few things:
>
> 1. Neither the SUSE nor the upstream patches change the supplementary
> groups.  SUSE patches fork() and then in the child setgid() and
> setuid().  Upstream doesn't fork(), but switches with setegid() and
> seteuid(), and then back.  If the intent is solely to avoid the need for
> fchown(), then that's sufficient.  Hopefully, along with SUSE's openat()
> and flags magic or with upstream's fopen(, "x"), nothing more is needed.
> However, if the intent is to avoid even trying to access files in user's
> directory with potentially excessive privileges, then supplementary
> groups should also be switched or dropped.
>
> I'm sorry I didn't get around to bringing this maybe-issue up in the
> pre-disclosure thread on the distros list (which Johannes Segitz from
> SUSE kindly started on September 27).  I feel it was not essential to
> discuss/address pre-disclosure, and is fine to discuss in public now.

Thanks for review and mentioning this!  I added some comments:

https://gitlab.com/oath-toolkit/oath-toolkit/-/issues/47

I noticed that that there are Linux-PAM helpers to drop privileges:

https://github.com/linux-pam/linux-pam/blob/master/libpam/pam_modutil_priv.c#L52

I have found another implementation of this in yubico-pam:

https://github.com/Yubico/yubico-pam/blob/master/drop_privs.c

> 2. Switching task credentials from library code is tricky, given that
> the program could have threads that don't expect this.  set*id() and
> setgroups() libc calls would typically affect all threads.  On Linux,
> it's possible to affect the current thread only, which e.g. we do in
> tcb[1] by using setfs*id() and direct setgroups() syscall (the latter
> only in our recent git code at this time, previously we used the libc
> function).  I assume Simon is aware of the Linux specific way, but
> deliberately chose not to do this in upstream oath-toolkit for
> portability to non-Linux.
>
> [1] https://www.openwall.com/tcb/ and https://github.com/openwall/tcb

Thanks for the pointer!  Yes, even the mild use of POSIX APIs in liboath
usersfile.c causes portability problems today, so I would like to avoid
adding more and ideally even remove the current usersfile stuff since it
doesn't belong in the core HOTP/TOTP library.

The thread concern is worrying though, but I'm hoping usage of this API
is not that widespread in any threaded applications.

> 4. As Simon also noted:
>
>> SUSE's alternative patch and advisory can be found via:
>> 
>> https://security.opensuse.org/2024/10/04/oath-toolkit-vulnerability.html
>> 
>> It rely on Linux kernel specific features and uses fork() which was
>> determined to be contrary to the liboath design, which aims to be
>> portable to macOS and *BSD and beyond.
>
> I agree fork() from library code is tricky, but not so much because of
> portability concerns.

Making fork() work on Windows from within a library is not that fun.

> Again, the program using the library may not expect it to ever have an
> extra child process.  Sure the library should use waitpid() on this
> specific process, yet the program could receive unexpected SIGCHLD.
> The combination of the program's threads and our fork() could also
> have unexpected consequences.
>
> In tcb, we chose to make usage of fork() a PAM module option, so that by
> enabling it the distro or sysadmin acknowledges that it's acceptable in
> the specific PAM configuration.  Our usage of fork() is for a different
> reason, though: "Using this option one can be sure that after a call to
> pam_end(3) there is no sensitive data left in the process' address
> space."  I wonder if this property would also be relevant in
> oath-toolkit patches if more processing is moved to the child process,
> or if this would be excessive under the relevant threat models.

Nice catch, I've opened an issue about this aspect:

https://gitlab.com/oath-toolkit/oath-toolkit/-/issues/48

Btw, do you have any thoughts on WHICH user to drop privileges to?  The
SUSE patch drops privs to the credential file owner.  My patch drops
privs to the PAM user that is being authenticated.  I think there are
reasonable arguments for both choices, and for all reasonable
configurations that I'm aware of, I don't think the choice matters.

/Simon

Download attachment "signature.asc" of type "application/pgp-signature" (256 bytes)
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.